linux-kernel - Re: [PATCH 01/10] gpiolib: provide gpiochip_dup_line

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=Mc6ce_gThRfZ78DzHGWdTAO-abY=Ythbd4KRHQ=Yfd_mw@mail.gmail.com>
Date:   Wed, 29 Nov 2023 16:45:11 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 01/10] gpiolib: provide gpiochip_dup_line_label()

On Wed, Nov 29, 2023 at 3:57 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Wed, Nov 29, 2023 at 03:24:02PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> >
> > gpiochip_is_requested() not only has a misleading name but it returns
> > a pointer to a string that is freed when the descriptor is released.
> >
> > Provide a new helper meant to replace it, which returns a copy of the
> > label string instead.
>
> ...
>
> > +/**
> > + * gpiochip_dup_line_label - Get a copy of the consumer label.
> > + * @gc: GPIO chip controlling this line.
> > + * @offset: Hardware offset of the line.
> > + *
> > + * Returns:
> > + * Pointer to a copy of the consumer label if the line is requested or NULL
> > + * if it's not. If a valid pointer was returned, it must be freed using
> > + * kfree(). In case of a memory allocation error, the function returns %ENOMEM.
>
> kfree_const() ? (see below)
>
> > + * Must not be called from atomic context.
> > + */
> > +char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +     const char *label;
> > +     char *cpy;
>
> Why not "copy"?
>
> > +
> > +     label = gpiochip_is_requested(gc, offset);
> > +     if (!label)
> > +             return NULL;
>
> > +     cpy = kstrdup(label, GFP_KERNEL);
>
> You probably want to have kstrdup_const(). However, I haven't checked
> if we have such use cases.

I thought about it but I'm thinking that it would be confusing to
users and lead to bugs. This is not used very much and only for
debugfs output. Let's keep it simple.

>
> > +     if (!cpy)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     return cpy;
> > +}
>
> So, how does this differ from the previous one? You need to hold a reference
> to the descriptor before copying and release it after.
>

The last patch reworks it to hold the obsolete gpio_lock and the
upcoming changes will make this perform the copy under the descriptor
lock and return it once it's released.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>