linux-kernel - Re: [PATCH v2] fs: try an opportunistic lookup for O

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: <20240807-erledigen-antworten-6219caebedc0@brauner>
Date: Wed, 7 Aug 2024 16:26:46 +0200
From: Christian Brauner <brauner@...nel.org>
To: Jeff Layton <jlayton@...nel.org>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>, 
	Andrew Morton <akpm@...ux-foundation.org>, Mateusz Guzik <mjguzik@...il.com>, 
	Josef Bacik <josef@...icpanda.com>, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] fs: try an opportunistic lookup for O_CREAT opens too

> +static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
> +{
> +	struct dentry *dentry;
> +
> +	if (open_flag & O_CREAT) {
> +		/* Don't bother on an O_EXCL create */
> +		if (open_flag & O_EXCL)
> +			return NULL;
> +
> +		/*
> +		 * FIXME: If auditing is enabled, then we'll have to unlazy to
> +		 * use the dentry. For now, don't do this, since it shifts
> +		 * contention from parent's i_rwsem to its d_lockref spinlock.
> +		 * Reconsider this once dentry refcounting handles heavy
> +		 * contention better.
> +		 */
> +		if ((nd->flags & LOOKUP_RCU) && !audit_dummy_context())
> +			return NULL;

Hm, the audit_inode() on the parent is done independent of whether the
file was actually created or not. But the audit_inode() on the file
itself is only done when it was actually created. Imho, there's no need
to do audit_inode() on the parent when we immediately find that file
already existed. If we accept that then this makes the change a lot
simpler.

The inconsistency would partially remain though. When the file doesn't
exist audit_inode() on the parent is called but by the time we've
grabbed the inode lock someone else might already have created the file
and then again we wouldn't audit_inode() on the file but we would have
on the parent.

I think that's fine. But if that's bothersome the more aggressive thing
to do would be to pull that audit_inode() on the parent further down
after we created the file. Imho, that should be fine?...

See https://gitlab.com/brauner/linux/-/commits/vfs.misc.jeff/?ref_type=heads
for a completely untested draft of what I mean.