Re: [PATCH RFC v2] mtd: block2mtd: fix circular locking dependency in block2mtd_setup

On Fri, 29 May 2026 at 14:24, Marco Elver <[email protected]> wrote:
>
> On Sat, 16 May 2026 at 17:41, 'syzbot' via
> syzkaller-upstream-moderation
> <[email protected]> wrote:
> >
> > syzkaller reported a circular locking dependency involving the global
> > param_lock and VFS locks (i_rwsem). The block2mtd driver performs a
> > synchronous VFS path lookup (kern_path via bdev_file_open_by_path)
> > inside its module parameter set callback (block2mtd_setup). This
> > callback is invoked by the sysfs core with param_lock held.
> >
> > The dependency chain is:
> > 1. i_rwsem -> cgroup_mutex (via cgroup_rmdir)
> > 2. cgroup_mutex -> rtnl_mutex (via cgroup_mkdir -> cgrp_css_online)
> > 3. rtnl_mutex -> param_lock (via mac80211_hwsim_new_radio ->
> >    ieee80211_rate_control_ops_get)
> > 4. param_lock -> i_rwsem (via block2mtd_setup -> kern_path)
> >
> > To fix this without breaking existing userspace scripts that expect
> > synchronous device creation, temporarily drop the param_lock during
> > the device creation process. This breaks the circular locking
> > dependency while preserving the synchronous nature of the operation.
> >
> > To implement this safely, introduce a local mutex (block2mtd_mutex)
> > to protect the module's internal state (blkmtd_device_list,
> > block2mtd_paramline, and block2mtd_init_called) from concurrent sysfs
> > writes, since param_lock will no longer provide this protection during
> > the critical section.
> >
> > Summary of changes:
> > 1. Introduce block2mtd_mutex to protect the global blkmtd_device_list
> >    and early-boot state variables.
> > 2. Drop param_lock in block2mtd_setup using kernel_param_unlock(kp->mod)
> >    and kernel_param_lock(kp->mod). For built-in modules, kp->mod is
> >    NULL,
> >    and these functions correctly unlock/lock the global param_lock.
> > 3. Protect block2mtd_setup with block2mtd_mutex to ensure that
> > concurrent
> >    sysfs writes are serialized and the device list is safely updated.
> > 4. Protect block2mtd_init and block2mtd_exit with block2mtd_mutex to
> >    ensure safe access to blkmtd_device_list and block2mtd_paramline.
> >
> > The lock ordering is now block2mtd_mutex -> VFS locks (i_rwsem) ->
> > mtd_table_mutex, which is strictly hierarchical and free of circular
> > dependencies.
> >
> > Fixes: f9d8c3c4236ef2a1c15b01200b9d164e2c5a54c7 ("block2mtd: port device access to files")
> > Assisted-by: Gemini:gemini-3.1-pro-preview Gemini:gemini-3-flash-preview
> > Reported-by: [email protected]
> > Link: https://syzkaller.appspot.com/bug?extid=65459fd3b61877d717a3
> > Link: https://syzkaller.appspot.com/ai_job?id=c3c46711-9492-41e5-bcb4-4fb68a746c23
> > To: <[email protected]>
> > To: <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
> >
> > ---
> > v2:
> > - Added comments in block2mtd_init() and block2mtd_exit() explaining the need for block2mtd_mutex to protect against concurrent sysfs writes.
> >
> > v1:
> > https://lore.kernel.org/all/[email protected]/T/
> > ---
> > diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> > index 03e80b2c4..42da6b9a5 100644
> > --- a/drivers/mtd/devices/block2mtd.c
> > +++ b/drivers/mtd/devices/block2mtd.c
> > @@ -45,7 +45,7 @@ struct block2mtd_dev {
> >
> >  /* Static info about the MTD, used in cleanup_module */
> >  static LIST_HEAD(blkmtd_device_list);
> > -
> > +static DEFINE_MUTEX(block2mtd_mutex);
> >
> >  static struct page *page_read(struct address_space *mapping, pgoff_t index)
> >  {
> > @@ -464,28 +464,43 @@ static int block2mtd_setup2(const char *val)
> >
> >  static int block2mtd_setup(const char *val, const struct kernel_param *kp)
> >  {
> > +       int ret = 0;
> > +
> > +       /*
> > +        * Drop the param_lock to avoid circular locking dependency
> > +        * with VFS locks during device lookup.
> > +        */
> > +       kernel_param_unlock(kp->mod);
> > +
> > +       mutex_lock(&block2mtd_mutex);
> > +
> >  #ifdef MODULE
> > -       return block2mtd_setup2(val);
> > +       ret = block2mtd_setup2(val);
> >  #else
> >         /* If more parameters are later passed in via
> >            /sys/module/block2mtd/parameters/block2mtd
> >            and block2mtd_init() has already been called,
> >            we can parse the argument now. */
> >
> > -       if (block2mtd_init_called)
> > -               return block2mtd_setup2(val);
> > +       if (block2mtd_init_called) {
> > +               ret = block2mtd_setup2(val);
> > +       } else {
> > +               /* During early boot stage, we only save the parameters
> > +                  here. We must parse them later: if the param passed
> > +                  from kernel boot command line, block2mtd_setup() is
> > +                  called so early that it is not possible to resolve
> > +                  the device (even kmalloc() fails). Deter that work to
> > +                  block2mtd_setup2(). */
> > +
> > +               strscpy(block2mtd_paramline, val, sizeof(block2mtd_paramline));
> > +       }
> > +#endif
> >
> > -       /* During early boot stage, we only save the parameters
> > -          here. We must parse them later: if the param passed
> > -          from kernel boot command line, block2mtd_setup() is
> > -          called so early that it is not possible to resolve
> > -          the device (even kmalloc() fails). Deter that work to
> > -          block2mtd_setup2(). */
> > +       mutex_unlock(&block2mtd_mutex);
> >
> > -       strscpy(block2mtd_paramline, val, sizeof(block2mtd_paramline));
> > +       kernel_param_lock(kp->mod);
> >
> > -       return 0;
> > -#endif
> > +       return ret;
> >  }
> >
> >
> > @@ -497,9 +512,15 @@ static int __init block2mtd_init(void)
> >         int ret = 0;
> >
> >  #ifndef MODULE
> > +       /*
> > +        * block2mtd_mutex protects block2mtd_init_called and
> > +        * block2mtd_paramline against concurrent sysfs writes.
> > +        */
> > +       mutex_lock(&block2mtd_mutex);
> >         if (strlen(block2mtd_paramline))
> >                 ret = block2mtd_setup2(block2mtd_paramline);
> >         block2mtd_init_called = 1;
> > +       mutex_unlock(&block2mtd_mutex);
> >  #endif
> >
> >         return ret;
> > @@ -511,6 +532,11 @@ static void block2mtd_exit(void)
> >         struct list_head *pos, *next;
> >
> >         /* Remove the MTD devices */
> > +       /*
> > +        * block2mtd_mutex protects blkmtd_device_list against
> > +        * concurrent sysfs writes.
> > +        */
> > +       mutex_lock(&block2mtd_mutex);
> >         list_for_each_safe(pos, next, &blkmtd_device_list) {
> >                 struct block2mtd_dev *dev = list_entry(pos, typeof(*dev), list);
> >                 block2mtd_sync(&dev->mtd);
> > @@ -522,6 +548,7 @@ static void block2mtd_exit(void)
> >                 list_del(&dev->list);
> >                 block2mtd_free_device(dev);
> >         }
> > +       mutex_unlock(&block2mtd_mutex);
> >  }
> >
> >  late_initcall(block2mtd_init);
>
>   The deadlock resolution is correct, but dropping  param_lock
> exposes a critical Use-After-Free (UAF) race condition during module
> unloading.
>
>   Because sysfs writes do not hold a module reference,  delete_module
> (rmmod) can proceed concurrently with an active sysfs parameter write.
>
>   1.  rmmod  starts, sets module state to  GOING , and calls  block2mtd_exit() .
>   2.  block2mtd_exit()  acquires  block2mtd_mutex , unregisters all
> active devices, clears  blkmtd_device_list , and releases
> block2mtd_mutex .
>   3. A concurrent sysfs write (which dropped  param_lock  to avoid the
> deadlock and was blocked on  block2mtd_mutex ) now acquires the mutex.
>   4. It calls  block2mtd_setup2()  $\rightarrow$  add_device() ,
> registers a new MTD device, and adds it to  blkmtd_device_list .
>   5. The sysfs write releases  block2mtd_mutex  and completes.
>   6.  delete_module  completes and frees the module memory.
>
>   This leaves an active MTD device registered in the kernel pointing
> to freed module code ( dev->mtd._read , etc.), leading to an immediate
> kernel panic on next access.
>
>   To prevent this, we must serialize the module exit state. Introduce
> a  block2mtd_exiting  flag synchronized by  block2mtd_mutex  to reject
> new registrations during unload.
>
>     diff --git a/drivers/mtd/devices/block2mtd.c
> b/drivers/mtd/devices/block2mtd.c
>     index f813272dad88..b12a84cf0e12 100644
>     --- a/drivers/mtd/devices/block2mtd.c
>     +++ b/drivers/mtd/devices/block2mtd.c
>     @@ -46,6 +46,7 @@ struct block2mtd_dev {
>      /* Static info about the MTD, used in cleanup_module */
>      static LIST_HEAD(blkmtd_device_list);
>      static DEFINE_MUTEX(block2mtd_mutex);
>     +static bool block2mtd_exiting;
>
>      static struct page *page_read(struct address_space *mapping, pgoff_t index)
>      {
>     @@ -477,6 +478,11 @@ static int block2mtd_setup(const char *val,
> const struct kernel_param *kp)
>
>          mutex_lock(&block2mtd_mutex);
>
>     +    if (block2mtd_exiting) {
>     +        ret = -ENODEV;
>     +        goto out_unlock;
>     +    }
>     +
>      #ifdef MODULE
>          ret = block2mtd_setup2(val);
>      #else
>     @@ -498,6 +504,7 @@ static int block2mtd_setup(const char *val,
> const struct kernel_param *kp)
>          }
>      #endif
>
>     +out_unlock:
>          mutex_unlock(&block2mtd_mutex);
>
>          kernel_param_lock(kp->mod);
>     @@ -529,6 +536,7 @@ static void block2mtd_exit(void)
>
>          /* Remove the MTD devices */
>          mutex_lock(&block2mtd_mutex);
>     +    block2mtd_exiting = true;
>          list_for_each_safe(pos, next, &blkmtd_device_list) {
>              struct block2mtd_dev *dev = list_entry(pos, typeof(*dev), list);
>              block2mtd_sync(&dev->mtd);

Another alternative is refcounting:

    static int block2mtd_setup(const char *val, const struct kernel_param *kp) {
           ...
           // Try to pin the module before dropping param_lock
           if (kp->mod && !try_module_get(kp->mod))
                  return -ENODEV; // Module is already unloading

           kernel_param_unlock(kp->mod);
           mutex_lock(&block2mtd_mutex);
           // critical section (VFS lookup & add_device)
           mutex_unlock(&block2mtd_mutex);
           kernel_param_lock(kp->mod);

           if (kp->mod)
                  module_put(kp->mod);
           ...
    }