linux-kernel - Re: [PATCH 00/20] afs: Fixes and development

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: <20180407171906.GB30522@ZenIV.linux.org.uk>
Date:   Sat, 7 Apr 2018 18:19:06 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     David Howells <dhowells@...hat.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-afs@...ts.infradead.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 00/20] afs: Fixes and development

On Sat, Apr 07, 2018 at 09:50:05AM -0700, Linus Torvalds wrote:
> On Thu, Apr 5, 2018 at 1:29 PM, David Howells <dhowells@...hat.com> wrote:
> >>
> > Here are a set of AFS patches, a few fixes, but mostly development.  The fixes
> > are:
> 
> So I pulled this after your updated fscache pull request, and I notice
> that these three commits are duplicate (not shared):
> 
>       fscache: Attach the index key and aux data to the cookie
>       fscache: Pass object size in rather than calling back for it
>       fscache: Maintain a catalogue of allocated cookies
> 
> and partly as a result I get some trivial conflicts.
> 
> Now, the conflicts really do look entirely trivial, and that's not the
> problem, but the fact that you *didn't* re-send the AFS pull request
> makes me wonder if you perhaps didn't want me to pull it after all?
> 
> So I decided to not do the resolution, and instead just verify with
> you that you still want this pulled?
> 
> No need to rebase, no need to do anything at all, really, except reply
> with "yes I want you to pull this" or "no, the fscache pull updates
> meant that I want you to do something else, hold off".
> 
> Pls advice.
> 
> (I may decide later to pull anyway, because I *think* you want me to
> pull, but thought to ask in case you're online and answer quickly).

FWIW, the main problem I see in there is the use of lookup_one_len()
with parent locked only shared.  As it is, that's simply broken -
lookup of full name happening at the same time will bugger the
things badly.

I have a series that lowers requirements to "parent must be held at
least shared" (see vfs.git#work.dcache) and it seems to be working.
With that series the locking problem goes away; however, the use of
dget_parent() around that lookup_one_len() call is pointless -
->lookup() is guaranteed that
	* dentry->d_parent is stable at least until dentry becomes
positive.  Dentry it originally pointed to remains pinned and positive
through the entire call of ->lookup(); 'dir' argument of ->lookup()
is the inode of that dentry.
	* dentry->d_name is stable at least until it becomes positive.
	* dir remains locked at least shared through the entire call
of ->lookup().

All ->lookup() instances rely upon that and there's no need to
play silly buggers with careful grabbing a reference to dentry->d_parent.
That, of course, can be dealt with after merge, but since that commit
has to be at least rebased to avoid bisection hazard... might as well
get rid of dget_parent() there at the same time.