linux-kernel - Re: [GIT PULL] gpio: updates for v5.13

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=Mdh9LvUQCxcyt7ZBjitDB2noVnOptft_VORDhffxJaeCA@mail.gmail.com>
Date:   Tue, 4 May 2021 16:17:02 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] gpio: updates for v5.13

On 5/4/21, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Mon, May 03, 2021 at 06:28:38PM +0000, Al Viro wrote:
>
>> > So Al, do you see anything horrendous in how that configfs thing uses
>> > a rename to do kind of an "atomic swap" of configfs state?
>>
>> Give me a few hours; configfs is playing silly buggers with a lot of
>> structures when creating/tearing down subtrees, and I'd actually
>> expect more trouble with configfs data structures than with VFS ones.
>>
>> I'll take a look.
>

Hi Al and thanks for the comments!

> FWIW, one obviously bogus thing is this:
>
> +       spin_lock(&configfs_dirent_lock);
> +       new_dentry->d_fsdata = sd;
> +       list_move(&sd->s_sibling, &new_parent_sd->s_children);
> +       item->ci_parent = new_parent_item;
> +       d_move(old_dentry, new_dentry);
> +       spin_unlock(&configfs_dirent_lock);
> on successful ->rename().  sd here comes from
> +       sd = old_dentry->d_fsdata;
>
> 	Now, take a look at configfs_d_iput().  ->d_fsdata contributes
> to refcount of sd, and I don't see anything here that would grab the
> reference.
>
> 	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?

> 	I dislike (for the lack of adequate printable terms) the way configfs
> deals with subtree creation and, especially, removal.  It's kept attached
> to dentry tree (all the way to the root) as we build it and, in case we
> fail halfway through, as we are trying to take it apart.
>
> 	There is convoluted code trying to prevent breakage in such cases,
> but it's complex, brittle and I don't remember how critical the lack of
> renames had been in its analysis.  I can try to redo that, but that would
> take some time - IIRC, the last time I did it, it took several days
> of work (including arseloads of grepping through configfs users and
> doing RTFS in those)
>
> 	IMO we should attach the subtree we'd built only when it's
> fully set up.  I can dig out the notes (from 2 years ago) on how to massage
> the damn thing in that direction, but again, it'll take a day or two
> to verify that analysis still applies.  OTOH, that would simplify the code
> considerably, so the next time we want to change something it wouldn't
> be so unpleasant.
>

This seems to address fundamental issues with configfs. I probably
don't have enough deep understanding of the VFS to even try to take on
this. My question again is: should this block the committable items
from getting merged or is this a plan for future improvement?

Can we proceed with merging it to see if it causes any regressions
later in the release cycle?

IMO this isn't a case where we could corrupt someone's files if we
make a mistake but I also acknowledge that I'm biased because I'm the
one who wants this functionality to improve our user-space tests.

Best Regards
Bartosz Golaszewski