linux-kernel - Re: [PATCH v1 1/1] gpiolib: Make gpiod

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMRc=MdPiz_YD451Arrm4mT-SwU_OdK1U-WozPxsvt11mHsLZQ@mail.gmail.com>
Date: Thu, 3 Apr 2025 14:23:24 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, linux-gpio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>, 
	Florian Fainelli <florian.fainelli@...adcom.com>, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

On Thu, Apr 3, 2025 at 1:17 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
> > >
> > > > But this encourages people to get sloppy and just ignore
> > > > error pointers returned from gpiod_get()?
> > >
> > > From where did you come to this conclusion, please? We have many subsystems
> > > that ignore invalid resource on the release stage, starting from platform
> > > device driver core.
> >
> > The fact that many people do something does not mean it's correct.
>
> And it doesn't tell it is incorrect either. We are going to conclude that there
> are pros and cons on each of the approaches, but I don't see much a point in
> yours, sorry.
>

My point is:

You get a descriptor with gpiod_get_optional(). You can get three
types of a pointer back:

1. Valid descriptor: you can use it in all GPIO consumer functions.
2. NULL-pointer: you can still use it in all GPIO consumer functions.
They will act as if they succeeded. This is expected as this is how
the "optional" functionality is implemented. Had it been written in
rust, we'd do it better but we use the tools we have. It's very much a
"valid" value to pass to gpiod routines.
3. IS_ERR() value. If you try to pass it to any of the GPIO consumer
functions, they will return it back to you and print a warning. Why?
Because it's an invalid object. And there's no reason to make
gpiod_put() an exemption. You never could have used an IS_ERR()
correctly. Look at what devres does - if it got an IS_ERR(), it never
schedules a release action.

> > Many other subsystem scream loudly when that happens, so I would be ok
> > with adding a big WARN_ON(IS_ERR(desc)).
>
> I disagree. This is not that case where passing an error pointer should be
> an issue.
>
> > > > Also: all other calls error out on IS_ERR(desc) so why would we make it an
> > > > exception?
> > >
> > > Because it's _release_ stage that participates in the cleaning up of
> > > the allocated resources in error paths. It's a common approach in
> > > the kernel. I would rather ask what makes GPIOLIB so special about it?
> >
> > Just because it's the release stage, does not mean you shouldn't care
> > about the correctness of the consumer code. Passing an IS_ERR(descr)
> > to any of the GPIO APIs can happen if the user ignores an error
> > returned by gpiod_get(). That's not alright.
>
> Have you ever seen such a code in the cases when it's okay (like in platform
> device driver users)? I do not. So, the above is based on the hypothetical
> assumption that somebody will make silly things. If you _really_ care about
> checking the error, add __must_check to the respective functions.
>

They already have but people do the following (like in the affected SPI driver):

struct driver_data *data = kzalloc();

Then elsewhere:

data->gpiod = gpiod_get();
if (IS_ERR(data->gpiod))
    return PTR_ERR(data->gpiod);

The data struct now contains the IS_ERR() value. I don't think it
makes any sense for it to carry it around and I don't want to enable
it.

Bart