Re: [PATCH v2 1/8] liveupdate: Protect file handler list with rwsem

[Mike Rapoport]

Hi Pasha,

On Wed, Mar 18, 2026 at 10:16:39AM -0400, Pasha Tatashin wrote:
> Because liveupdate file handlers will no longer hold a
> module reference when registered, we must ensure that the access to
> the handler list is protected against concurrent module unloading.
> 
> Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
> ---
>  kernel/liveupdate/luo_file.c | 61 +++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 26 deletions(-)
> 
> diff --git a/kernel/liveupdate/luo_file.c b/kernel/liveupdate/luo_file.c
> index 5acee4174bf0..6a0ae29c6a24 100644
> --- a/kernel/liveupdate/luo_file.c
> +++ b/kernel/liveupdate/luo_file.c
> @@ -112,6 +112,7 @@
>  #include <linux/string.h>
>  #include "luo_internal.h"
>  
> +static DECLARE_RWSEM(luo_file_handler_lock);
>  static LIST_HEAD(luo_file_handler_list);
>  
>  /* 2 4K pages, give space for 128 files per file_set */
> @@ -277,10 +278,12 @@ int luo_preserve_file(struct luo_file_set *file_set, u64 token, int fd)
>  		goto  err_fput;
>  
>  	err = -ENOENT;
> -	list_private_for_each_entry(fh, &luo_file_handler_list, list) {
> -		if (fh->ops->can_preserve(fh, file)) {
> -			err = 0;
> -			break;
> +	scoped_guard(rwsem_read, &luo_file_handler_lock) {

I'm not sure scoped_guard() here is better that lock before the loop and
unlock after. scoped_guard() would be useful if we were returning from the
guarded scope.

> +		list_private_for_each_entry(fh, &luo_file_handler_list, list) {
> +			if (fh->ops->can_preserve(fh, file)) {
> +				err = 0;
> +				break;
> +			}
>  		}
>  	}
>  
> @@ -777,10 +780,12 @@ int luo_file_deserialize(struct luo_file_set *file_set,
>  		bool handler_found = false;
>  		struct luo_file *luo_file;
>  
> -		list_private_for_each_entry(fh, &luo_file_handler_list, list) {
> -			if (!strcmp(fh->compatible, file_ser[i].compatible)) {
> -				handler_found = true;
> -				break;
> +		scoped_guard(rwsem_read, &luo_file_handler_lock) {
> +			list_private_for_each_entry(fh, &luo_file_handler_list, list) {
> +				if (!strcmp(fh->compatible, file_ser[i].compatible)) {
> +					handler_found = true;
> +					break;
> +				}

Ditto.

>  			}
>  		}
>  
> @@ -850,25 +855,27 @@ int liveupdate_register_file_handler(struct liveupdate_file_handler *fh)
>  	if (!luo_session_quiesce())
>  		return -EBUSY;
>  
> -	/* Check for duplicate compatible strings */
> -	list_private_for_each_entry(fh_iter, &luo_file_handler_list, list) {
> -		if (!strcmp(fh_iter->compatible, fh->compatible)) {
> -			pr_err("File handler registration failed: Compatible string '%s' already registered.\n",
> -			       fh->compatible);
> -			err = -EEXIST;
> +	scoped_guard(rwsem_write, &luo_file_handler_lock) {
> +		/* Check for duplicate compatible strings */
> +		list_private_for_each_entry(fh_iter, &luo_file_handler_list, list) {
> +			if (!strcmp(fh_iter->compatible, fh->compatible)) {
> +				pr_err("File handler registration failed: Compatible string '%s' already registered.\n",
> +				       fh->compatible);
> +				err = -EEXIST;
> +				goto err_resume;

PeterZ advised against mixing guards() with goto, but unfortunately I can't
find lore link right now.

> +			}
> +		}
> +

-- 
Sincerely yours,
Mike.