linux-kernel - Re: [PATCH 3/4] gpio: virtuser: lock up configfs that an instantiated device depends on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <pdz63f2y3ivqb3pu6hqvx3ir5gystcy2g77cpd43s7fm75rrgy@eyfkw5vrnx6f>
Date: Fri, 3 Jan 2025 11:37:45 +0900
From: Koichiro Den <koichiro.den@...onical.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: linux-gpio@...r.kernel.org, linus.walleij@...aro.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] gpio: virtuser: lock up configfs that an
 instantiated device depends on

On Thu, Jan 02, 2025 at 02:21:25PM +0100, Bartosz Golaszewski wrote:
> On Tue, Dec 24, 2024 at 7:08 AM Koichiro Den <koichiro.den@...onical.com> wrote:
> >
> > Once a virtuser device is instantiated and actively used, allowing rmdir
> > for its configfs serves no purpose and can be confusing. Userspace
> > interacts with the virtual consumer at arbitrary times, meaning it
> > depends on its existance.
> >
> > Make the subsystem itself depend on the configfs entry for a virtuser
> > device while it is in active use.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@...onical.com>
> > ---
> >  drivers/gpio/gpio-virtuser.c | 49 ++++++++++++++++++++++++++++++------
> >  1 file changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c
> > index c9700c1e4126..45b8f192f860 100644
> > --- a/drivers/gpio/gpio-virtuser.c
> > +++ b/drivers/gpio/gpio-virtuser.c
> > @@ -1533,6 +1533,32 @@ gpio_virtuser_device_deactivate(struct gpio_virtuser_device *dev)
> >         kfree(dev->lookup_table);
> >  }
> >
> > +static void
> > +gpio_virtuser_device_lockup_configfs(struct gpio_virtuser_device *dev, bool lock)
> > +{
> > +       struct gpio_virtuser_lookup_entry *entry;
> > +       struct gpio_virtuser_lookup *lookup;
> > +       struct configfs_subsystem *subsys;
> > +
> > +       subsys = dev->group.cg_subsys;
> 
> If there'll be a v2 (patch 1/4 possibly needs it), can you assign it
> at declaration? Same for 4/4.

That's a good idea, I'll do so in v2.
Let me also fix a minor typo in the commit message ("existance").

-Koichiro

> 
> Bart
> 
> > +
> > +       /*
> > +        * The device only needs to depend on leaf lookup entries. This is
> > +        * sufficient to lock up all the configfs entries that the
> > +        * instantiated, alive device depends on.
> > +        */
> > +       list_for_each_entry(lookup, &dev->lookup_list, siblings) {
> > +               list_for_each_entry(entry, &lookup->entry_list, siblings) {
> > +                       if (lock)
> > +                               WARN_ON(configfs_depend_item_unlocked(
> > +                                               subsys, &entry->group.cg_item));
> > +                       else
> > +                               configfs_undepend_item_unlocked(
> > +                                               &entry->group.cg_item);
> > +               }
> > +       }
> > +}
> > +
> >  static ssize_t
> >  gpio_virtuser_device_config_live_store(struct config_item *item,
> >                                        const char *page, size_t count)
> > @@ -1545,15 +1571,24 @@ gpio_virtuser_device_config_live_store(struct config_item *item,
> >         if (ret)
> >                 return ret;
> >
> > -       guard(mutex)(&dev->lock);
> > +       if (live)
> > +               gpio_virtuser_device_lockup_configfs(dev, true);
> >
> > -       if (live == gpio_virtuser_device_is_live(dev))
> > -               return -EPERM;
> > +       scoped_guard(mutex, &dev->lock) {
> > +               if (live == gpio_virtuser_device_is_live(dev))
> > +                       ret = -EPERM;
> > +               else if (live)
> > +                       ret = gpio_virtuser_device_activate(dev);
> > +               else
> > +                       gpio_virtuser_device_deactivate(dev);
> > +       }
> >
> > -       if (live)
> > -               ret = gpio_virtuser_device_activate(dev);
> > -       else
> > -               gpio_virtuser_device_deactivate(dev);
> > +       /*
> > +        * Undepend is required only if device disablement (live == 0)
> > +        * succeeds or if device enablement (live == 1) fails.
> > +        */
> > +       if (live == !!ret)
> > +               gpio_virtuser_device_lockup_configfs(dev, false);
> >
> >         return ret ?: count;
> >  }
> > --
> > 2.43.0
> >