Message-ID: <CAMRc=MfyD5iyfB5f5hx_Kq4p7NZv+0o8HVOysiy6DaKANpGNKQ@mail.gmail.com>
Date:   Tue, 13 Sep 2022 17:58:32 +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:55 PM Kent Gibson <warthog618@...il.com> wrote:
>
> On Tue, Sep 13, 2022 at 04:35:08PM +0200, Bartosz Golaszewski wrote:
> > 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. :)
> >
>
> Forgot that we don't have the path readily available in the kernel -
> would device name suffice?

Maybe. I'm looking at what fdinfo shows in my vm and see things like:

$ cat /proc/2353/fdinfo/10
pos: 0
flags: 02004000
mnt_id: 15
ino: 1052
inotify wd:1 ino:1 sdev:3c mask:fce ignored_mask:0 fhandle-bytes:c
fhandle-type:1 f_handle:7f0dd9400100000000000000

I don't see any tools/libs readily available for parsing these. In
libgpiod, if the user wanted to read the PID of the owner of the line,
we'd need to manually go through the list of all fdinfo entries we
have permissions to access and compare those against the line w'ere
checking.

We'd need of course first expose that info like:

gpio chip:gpiochip2 lines:0,3,4,7

Does that make sense?

Bart

Powered by blists - more mailing lists