From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Linus Torvalds pointed out that using trylock here is wrong. This iteration
drops it in favor of unconditional locks but keeps all the fixes that came
later.

I will also not send it for this release but make it part of the updates PR
for v6.2 to give it some time in next.

v7 -> v8:
- don't use down_read_trylock(), just go straight for a full lock

v6 -> v7:
- fix a build issue with CDEV_V1 code disabled (giving credit to Nick Hainke)
- protect the gdev->chip also in gpio_chrdev_open()

v5 -> v6:
- signal an error in poll callbacks instead of returning 0 which would make
  the user-space assume a timeout occurred (which could lead to user-space
  spinning a timeout loop forever)

v4 -> v5:
- try to acquire the semaphore for reading and bail out of syscall callbacks
  immediately in case of lock contention

v3 -> v4:
- use function typedefs to make code cleaner
- add a blank line after down_write()

v2 -> v3:
- drop the helper variable in patch 1/2 as we won't be using it in 2/2
- refactor patch 2/2 to use locking wrappers around the syscall callbacks

v1 -> v2:
- add missing gdev->chip checks in patch 1/2
- add a second patch that protects the structures that can be accessed
  by user-space calls against concurrent removal

Bartosz Golaszewski (2):
  gpiolib: cdev: fix NULL-pointer dereferences
  gpiolib: protect the GPIO device against being dropped while in use by
    user-space

 drivers/gpio/gpiolib-cdev.c | 204 +++++++++++++++++++++++++++++++-----
 drivers/gpio/gpiolib.c      |   4 +
 drivers/gpio/gpiolib.h      |   5 +
 3 files changed, 188 insertions(+), 25 deletions(-)

-- 
2.37.2

On Mon, Dec 5, 2022 at 1:39 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Linus Torvalds pointed out that using trylock here is wrong. This iteration
> drops it in favor of unconditional locks but keeps all the fixes that came
> later.
>
> I will also not send it for this release but make it part of the updates PR
> for v6.2 to give it some time in next.
>
> v7 -> v8:
> - don't use down_read_trylock(), just go straight for a full lock
>
> v6 -> v7:
> - fix a build issue with CDEV_V1 code disabled (giving credit to Nick Hainke)
> - protect the gdev->chip also in gpio_chrdev_open()
>
> v5 -> v6:
> - signal an error in poll callbacks instead of returning 0 which would make
>   the user-space assume a timeout occurred (which could lead to user-space
>   spinning a timeout loop forever)
>
> v4 -> v5:
> - try to acquire the semaphore for reading and bail out of syscall callbacks
>   immediately in case of lock contention
>
> v3 -> v4:
> - use function typedefs to make code cleaner
> - add a blank line after down_write()
>
> v2 -> v3:
> - drop the helper variable in patch 1/2 as we won't be using it in 2/2
> - refactor patch 2/2 to use locking wrappers around the syscall callbacks
>
> v1 -> v2:
> - add missing gdev->chip checks in patch 1/2
> - add a second patch that protects the structures that can be accessed
>   by user-space calls against concurrent removal
>
> Bartosz Golaszewski (2):
>   gpiolib: cdev: fix NULL-pointer dereferences
>   gpiolib: protect the GPIO device against being dropped while in use by
>     user-space
>
>  drivers/gpio/gpiolib-cdev.c | 204 +++++++++++++++++++++++++++++++-----
>  drivers/gpio/gpiolib.c      |   4 +
>  drivers/gpio/gpiolib.h      |   5 +
>  3 files changed, 188 insertions(+), 25 deletions(-)
>
> --
> 2.37.2
>

Reapplied again to get it back into next for testing.

Bart

On Mon, Dec 05, 2022 at 01:39:01PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Linus Torvalds pointed out that using trylock here is wrong. This iteration
> drops it in favor of unconditional locks but keeps all the fixes that came
> later.
> 
> I will also not send it for this release but make it part of the updates PR
> for v6.2 to give it some time in next.
> 
> v7 -> v8:
> - don't use down_read_trylock(), just go straight for a full lock

Yep, it was a good wrap-up explanation.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Dec 05, 2022 at 02:59:42PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 05, 2022 at 01:39:01PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > 
> > Linus Torvalds pointed out that using trylock here is wrong. This iteration
> > drops it in favor of unconditional locks but keeps all the fixes that came
> > later.
> > 
> > I will also not send it for this release but make it part of the updates PR
> > for v6.2 to give it some time in next.
> > 
> > v7 -> v8:
> > - don't use down_read_trylock(), just go straight for a full lock
> 
> Yep, it was a good wrap-up explanation.

But he also suggested to fold NULL check into call_*_locked(). Any points why
you decided not to go that way?

-- 
With Best Regards,
Andy Shevchenko

On Mon, Dec 5, 2022 at 2:01 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Dec 05, 2022 at 02:59:42PM +0200, Andy Shevchenko wrote:
> > On Mon, Dec 05, 2022 at 01:39:01PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Linus Torvalds pointed out that using trylock here is wrong. This iteration
> > > drops it in favor of unconditional locks but keeps all the fixes that came
> > > later.
> > >
> > > I will also not send it for this release but make it part of the updates PR
> > > for v6.2 to give it some time in next.
> > >
> > > v7 -> v8:
> > > - don't use down_read_trylock(), just go straight for a full lock
> >
> > Yep, it was a good wrap-up explanation.
>
> But he also suggested to fold NULL check into call_*_locked(). Any points why
> you decided not to go that way?
>

Yes! Every read(), ioctl() and poll() variant extract a different
structure from file->private_data. I'm afraid we need to keep those in
the callbacks down the stack. I too thought it was a good idea but
that won't work.

Bart

On Mon, Dec 05, 2022 at 03:01:13PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 05, 2022 at 02:59:42PM +0200, Andy Shevchenko wrote:
> > On Mon, Dec 05, 2022 at 01:39:01PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > 
> > > Linus Torvalds pointed out that using trylock here is wrong. This iteration
> > > drops it in favor of unconditional locks but keeps all the fixes that came
> > > later.
> > > 
> > > I will also not send it for this release but make it part of the updates PR
> > > for v6.2 to give it some time in next.
> > > 
> > > v7 -> v8:
> > > - don't use down_read_trylock(), just go straight for a full lock
> > 
> > Yep, it was a good wrap-up explanation.
> 
> But he also suggested to fold NULL check into call_*_locked(). Any points why
> you decided not to go that way?
> 

He also mentioned making it more back-portable.
Does that mean splitting out the patches for uAPI v1 and v2, if not for
each of the Fixes? Or does he mean back-porting it to 6.1?

Cheers,
Kent.

On Mon, Dec 5, 2022 at 2:17 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Dec 05, 2022 at 03:01:13PM +0200, Andy Shevchenko wrote:
> > On Mon, Dec 05, 2022 at 02:59:42PM +0200, Andy Shevchenko wrote:
> > > On Mon, Dec 05, 2022 at 01:39:01PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Linus Torvalds pointed out that using trylock here is wrong. This iteration
> > > > drops it in favor of unconditional locks but keeps all the fixes that came
> > > > later.
> > > >
> > > > I will also not send it for this release but make it part of the updates PR
> > > > for v6.2 to give it some time in next.
> > > >
> > > > v7 -> v8:
> > > > - don't use down_read_trylock(), just go straight for a full lock
> > >
> > > Yep, it was a good wrap-up explanation.
> >
> > But he also suggested to fold NULL check into call_*_locked(). Any points why
> > you decided not to go that way?
> >
>
> He also mentioned making it more back-portable.
> Does that mean splitting out the patches for uAPI v1 and v2, if not for
> each of the Fixes? Or does he mean back-porting it to 6.1?

I think he mentioned not sending it for v6.1 that late in the cycle
and instead sending it for v6.2 and backporting it to v6.1. I'm afraid
that backporting of this fix will require some manual labor anyway.
We'll want to backport it to branches that don't have v2 yet after
all, where that code resided elsewhere.

Bartosz