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: <d682e7c2749f8e8c74ea43b8893a17bd6e9a0007.camel@kernel.org>
Date: Wed, 07 Aug 2024 10:36:58 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Christian Brauner <brauner@...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

On Wed, 2024-08-07 at 16:26 +0200, Christian Brauner wrote:
> > +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.

Yeah, that's a lot simpler. That said, my experience when I've worked
with audit in the past is that people who are using it are _very_
sensitive to changes of when records get emitted or not. I don't like
this, because I think the rules here are ad-hoc and somewhat arbitrary,
but keeping everything working exactly the same has been my MO whenever
I have to work in there.

If a certain access pattern suddenly generates a different set of
records (or some are missing, as would be in this case), we might get
bug reports about this. I'm ok with simplifying this code in the way
you suggest, but we may want to do it in a patch on top of mine, to
make it simple to revert later if that becomes necessary.
-- 
Jeff Layton <jlayton@...nel.org>