Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210502193216.24872-1-brgl@bgdev.pl> <CAHk-=whSWp3exv8tZ2th5im_P7HF=c6iuOOVb9iSrNrd6405WA@mail.gmail.com>
 <YJBA1iYK7npit9vn@zeniv-ca.linux.org.uk> <YJCpnvKUNx+Tc+vg@zeniv-ca.linux.org.uk>
 <CAMRc=Mdh9LvUQCxcyt7ZBjitDB2noVnOptft_VORDhffxJaeCA@mail.gmail.com> <YJGFsrPBoQsKj+JZ@zeniv-ca.linux.org.uk>
In-Reply-To: <YJGFsrPBoQsKj+JZ@zeniv-ca.linux.org.uk>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Wed, 5 May 2021 16:19:52 +0200
Message-ID: <CAMRc=Mf1N1iL4b5KvVFbPsvKDMy_geXkd8LjmCu5r=bih-N7_w@mail.gmail.com>
Subject: Re: [GIT PULL] gpio: updates for v5.13
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 4, 2021 at 7:34 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, May 04, 2021 at 04:17:02PM +0200, Bartosz Golaszewski wrote:
> > >     Incidentally, if your code critically depends upon some field
> > > being first in such-and-such structure, you should either get rid of
> > > the dependency or at least bother to document that.
> > > That
> > > +               /*
> > > +                * Free memory allocated for the pending and live
> > > directories
> > > +                * of committable groups.
> > > +                */
> > > +               if (sd->s_type & (CONFIGFS_GROUP_PENDING |
> > > CONFIGFS_GROUP_LIVE))
> > > +                       kfree(sd->s_element);
> > > +
> > > is asking for trouble down the road.
> > >
> >
> > I'm not sure if this is a hard NAK for these changes or if you
> > consider this something that can be ironed out post v5.13-rc1?
>
> Rename implementation is simply bogus.  You are, for some reason, attaching
> stuff to *destination*, which won't be seen by anyone not currently using
> it.  It's the old_dentry that will be seen from that point on - you are
> moving it to new location by that d_move().  So I rather wonder how much
> had that thing been tested.  And I'm pretty much certain that you are
> mishandling the refcounts on configfs-internal objects, with everything
> that entails in terms of UAF and leaks.
>

The interface's stability in user-space has been tested a lot with the
test-suite for libgpiod[1] but I didn't look for leaks indeed.

> FWIW, I'm not happy about the userland API of that thing (what is supposed
> to happen if you create, move to live, then create another with the same
> name and try to move it to live or original back from live?), but
> Documentation/filesystems/configfs.rst is too sparse on such details.
> So I would like to see the specifics on that as well.  _Before_ signing
> up on anything, including "we can fix it up after merge".

Understood. I've sent out a new PR without these changes. I'll start
another thread asking for your help on the correct approach and maybe
some better ideas for the user interface.

Thanks,
Bartosz

[1] https://patchwork.ozlabs.org/project/linux-gpio/patch/20210429094734.9585-3-brgl@bgdev.pl/