linux-kernel - Re: [PATCH 2/2] gpiolib: cdev: export the consumer's PID

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=MdBRGW0skXOgPbZy=w4EiWcyKmKwKSZuAj+k4EtbPmvdg@mail.gmail.com>
Date:   Tue, 13 Sep 2022 16:35:08 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Kent Gibson <warthog618@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] gpiolib: cdev: export the consumer's PID

On Tue, Sep 13, 2022 at 4:28 PM Kent Gibson <warthog618@...il.com> wrote:
>
> On Tue, Sep 13, 2022 at 10:54:26AM +0200, Bartosz Golaszewski wrote:
> > On Tue, Sep 13, 2022 at 4:12 AM Kent Gibson <warthog618@...il.com> wrote:
> > >
> > > On Mon, Sep 12, 2022 at 11:56:17AM +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@...il.com> wrote:
> > > > >
> > > >
> > > > [snip]
> > > >
> > > > > >
> > > > > > Using -1 sounds good but I've just realized there's a different
> > > > > > problem. A process holding a file descriptor may fork and both the
> > > > > > parent and the child will keep the same file descriptors open. Now
> > > > > > we'll have two processes (with different PIDs) holding the same GPIO
> > > > > > lines (specifically holding a file descriptor to the same anonymous
> > > > > > inode).
> > > > > >
> > > > > > This already poses a problem for this patch as we'd need to return an
> > > > > > array of PIDs which we don't have the space for but also is a
> > > > > > situation which we haven't discussed previously IIRC - two processes
> > > > > > keeping the same GPIO lines requested.
> > > > > >
> > > > > > I don't have any good idea on how to address this yet. One thing off
> > > > > > the top of my head is: close the parent's file descriptor from kernel
> > > > > > space (is it even possible?) on fork() (kind of like the close() on
> > > > > > exec flag).
> > > > > >
> > > > > > I need to think about it more.
> > > > > >
> > > > >
> > > > > I thought the O_CLOEXEC was set on the request fds exactly to prevent this
> > > > > case - only one process can hold the request fd.
> > > > >
> > > >
> > > > O_CLOEXEC means "close on exec" not "close on fork". When you fork,
> > > > you inherit all file descriptors from your parent. Only once you call
> > > > execve() are the fds with this flag closed *in the child*.
> > > >
> > >
> > > Ah, ok.
> > > You want to pass request fd ownership from parent to child??
> > > Why not lock ownership to the parent, so O_CLOFORK, were that
> > > available?
> > >
> >
> > Because what if we want to request a line and then daemonize i.e. fork
> > and exit in parent? It makes much more sense to keep the lines
> > requested in the child IMO.
> >
>
> Then you are doing it backwards - daemonize first ;-).
>
> Generally speaking, doesn't transfer of resource ownership to the forked
> child create havoc in multi-threaded apps? i.e. one thread requests a
> resource, another forks.  The parent thread unknowingly loses ownership,
> and the forked child process only starts with a replica of the forking
> thread.
>

Yeah, sounds like a bad idea.

> > During the BoF at Linux Plumbers it was suggested to use
> > /proc/$PID/fdinfo to expose the information about which lines are
> > requested but I can't figure out a way to do it elegantly.
> >
>
> Yeah, missed that :-(.
>
> Makes sense.
>
> As each request fd can contain multiple lines on a particular chip,
> you would need to identify the gpiochip and the offsets for that request.
> So two fields - the gpiochip path, and the list of offsets.
>
> Is that already too clunky or am I missing something?
>

It's worse than that - we don't know the character device's filesystem
path in gpiolib. Nor should we, as we can be in a different fs
namespace when checking it than in which we were when we opened the
device (which is also another concern for storing the path to the
character device in struct gpiod_chip - unless we specify explicitly
that it's the path that was used to open it). Since we don't know it -
we can only get it from the file descriptor that the requesting
process got after calling open() on the GPIO device. But this fd may
have been closed in the meantime. I think I opened a can of worms with
this one. :)

Bartosz