linux-kernel - Re: [PATCH 5/6] eventfs: get rid of dentry pointers without refcounts

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: <20240131003317.7a63e799@gandalf.local.home>
Date: Wed, 31 Jan 2024 00:33:17 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] eventfs: get rid of dentry pointers without
 refcounts

On Tue, 30 Jan 2024 21:25:30 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> > Does this work:
> >
> >         d_invalidate(dentry);  
> 
> It does, but it's basically irrelevant with the d_revalidate approach.
> 
> Basically, once you have d_revalidate(), the unhashing happens there,
> and it's just extra work and pointless to do it elsewhere.
> 
> So if you look at the "clean up dentry ops and add revalidate
> function" patch, you'll see that it just does
> 
> -       simple_recursive_removal(dentry, NULL);
> 
> and the thing is just history.

With even the last patch included, without the d_invalidate() I get errors
with simply doing:

 # cd /sys/kernel/tracing
 # mkdir instances/foo
 # ls instances/foo/events
 # rmdir instances/foo

As the rmdir calls tracefs_remove() that calls simple_recursive_removal()
that then walks into the "events" directory. Without that d_invalidate, it
walks beyond just the top directory and then splats on the dentries that
are cached.


> 
> So really, that final patch is the one that fixes the whole eventfs
> mess for good (knock wood). But you can't do it first, because it
> basically depends on all the refcount fixes.

I'm running my full suite with the final patch included, plus some of the
updates I mentioned in replies to other patches, as well as including this
"d_invalidate()" as it doesn't pass without it.

> 
> It might be possible to re-organize the patches so that the refcount
> changes go first, then the d_revalidate(), and then the rest. But I
> suspect they all really end up depending on each other some way,
> because the basic issue was that the whole "keep unrefcounted dentry
> pointers around" was just wrong.  So it doesn't end up right until
> it's _all_ fixed, because every step of the way exposes some problem.
> 
> At least that was my experience. Fix one thing, and it exposes the
> hack that another thing depended on.
> 
> This is actually something that Al is a master at. You sometimes see
> him send one big complicated patch where he talks about all the
> problems in some area and it's one huge "fix up everything patch" that
> looks very scary.
> 
> And then a week later he sends a series of 19 patches that all make
> sense and all look "obvious" and all make small progress.
> 
> And magically they end up matching that big cleanup patch in the end.
> And you just *know* that it didn't start out as that beautiful logical
> series, because you saw the big messy patch first...

I'll take a look at breaking the patches up further, as I now have a much
better understanding of dentries then I did before this discussion.

-- Steve