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: <20240131000956.3dbc0fc0@gandalf.local.home>
Date: Wed, 31 Jan 2024 00:09:56 -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 11:03:54 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> +/*
> + * eventfs_inode reference count management.
> + *
> + * NOTE! We count only references from dentries, in the
> + * form 'dentry->d_fsdata'. There are also references from
> + * directory inodes ('ti->private'), but the dentry reference
> + * count is always a superset of the inode reference count.
> + */
> +static void release_ei(struct kref *ref)
> +{
> +	struct eventfs_inode *ei = container_of(ref, struct eventfs_inode, kref);
> +	kfree(ei->entry_attrs);

I did modify this to add back the kfree_const(ei->name);

I would also like to add a:

	WARN_ON_ONCE(!ei->is_freed);

The kref_put() logic should add the protection that this is guaranteed to
be true as the logic in the removal does:

	ei->is_freed = 1;
  [..]
	kref_put(ei);

I would think that the last "put" would always have the "is_freed" set. The
WARN_ON is to catch things doing too many put_ei().

> +	kfree(ei);
> +}
> +

> @@ -951,5 +857,6 @@ void eventfs_remove_events_dir(struct eventfs_inode *ei)
>  	 * sticks around while the other ei->dentry are created
>  	 * and destroyed dynamically.
>  	 */
> +	simple_recursive_removal(dentry, NULL);

Actually, this doesn't work. It expects all the dentries in the list to
have positive ref counts, as it does dput() on them. If there's any cached,
it will bug with a dput() on a dentry of zero.

The only dentries that should have non zero ref counts are ones that are
currently being accessed and wont go away until finished. I think all we
need to do is invalidate the top dentry. Is that true?

Does this work:

	d_invalidate(dentry);

to make the directory no longer accessible. And all dentries within it
should be zero, and hopefully go away on memory reclaim. The last patch
removes it, but if you want to test the deletion, you can do this:

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

But also note that this patch fails with the "ghost" files with the kprobe
test again. When I apply patch 6, that goes away. I'm guessing that's
because this needs the d_revalidate() call. To not break bisection, I think
we need to merge this and patch 6 together.

Also patch 6 removes the simple_recursive_removal() which without at least
the d_invalidate() causes a dput splat. That's because the rmdir calls
tracefs_remove() which calls simple_recursive_removal() that will walk to
the events directory and I'm assuming if it hasn't been invalidated, it
walks into it causing the same issue as a simple_recursive_removal() would
have here.

Try the above mkdir and rmdir to see.

-- Steve

>  	dput(dentry);
>  }