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

[Pasha Tatashin]

On Sun, Mar 22, 2026 at 9:13 AM Mike Rapoport <rppt@xxxxxxxxxx> wrote:
>
> 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.

Yeah, I was looking into this yesterday:
https://lore.kernel.org/all/CA+CK2bD2aNu-NByET4KkBP0n2j8WQE2zjxb4g-1cv2hYfjaRZQ@xxxxxxxxxxxxxx

Also, please wait for the next version before reviewing it. I am going
to use what Sami suggested:
https://lore.kernel.org/all/CA+CK2bDONAnry0n9qOsR7GSDTw4Xw3g4ZugRPwXD+NdMx7rpsQ@xxxxxxxxxxxxxx

Pasha

>
> > +                     }
> > +             }
> > +
>
> --
> Sincerely yours,
> Mike.