linux-kernel - Re: [PATCH 1/2] vfs: Allow filesystems with foreign owner IDs to override UID checks

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] [day] [month] [year] [list]

Message-ID: <1523597.1761052821@warthog.procyon.org.uk>
Date: Tue, 21 Oct 2025 14:20:21 +0100
From: David Howells <dhowells@...hat.com>
To: Christian Brauner <brauner@...nel.org>
Cc: dhowells@...hat.com, Al Viro <viro@...iv.linux.org.uk>,
    Christian Brauner <christian@...uner.io>,
    Marc Dionne <marc.dionne@...istor.com>,
    Jeffrey Altman <jaltman@...istor.com>,
    Steve French <sfrench@...ba.org>, linux-afs@...ts.infradead.org,
    openafs-devel@...nafs.org, linux-cifs@...r.kernel.org,
    linux-nfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
    linux-kernel@...r.kernel.org,
    Etienne Champetier <champetier.etienne@...il.com>,
    Chet Ramey <chet.ramey@...e.edu>,
    Cheyenne Wills <cwills@...enomine.net>,
    Mimi Zohar <zohar@...ux.ibm.com>, linux-integrity@...r.kernel.org
Subject: Re: [PATCH 1/2] vfs: Allow filesystems with foreign owner IDs to override UID checks

Christian Brauner <brauner@...nel.org> wrote:

> > +	if (unlikely(inode->i_op->have_same_owner)) {
> 
> Same, as above: similar to IOP_FASTPERM this should use a flag to avoid pointer derefs.

Can we do these IOP_* flags better?  Surely we can determine at the point the
inode has its ->i_op assigned that these things are provided?  This optimises
the case where they don't exist at the expense of the case where they do (we
still have to check the pointer every time).

> > +	if (unlikely(inode->i_op->have_same_owner)) {
> 
> Same, as above: similar to IOP_FASTPERM this should use a flag to avoid pointer derefs.
> 
> Really, we should very properly bias this towards the common case where
> the filesystem will not have a custom ownership comparison callback at all.

Hence the unlikely().

> > +		struct dentry *parent;
> > +		struct inode *dir;
> > +		int ret;
> > +
> > +		if (inode != nd->inode) {
> > +			dir = nd->inode;
> > +			ret = inode->i_op->have_same_owner(idmap, inode, dir);
> > +		} else if (nd->flags & LOOKUP_RCU) {
> > +			parent = READ_ONCE(nd->path.dentry);
> > +			dir = READ_ONCE(parent->d_inode);
> > +			if (!dir)
> > +				return -ECHILD;
> > +			ret = inode->i_op->have_same_owner(idmap, inode, dir);
> > +		} else {
> > +			parent = dget_parent(nd->path.dentry);
> > +			dir = parent->d_inode;
> > +			ret = inode->i_op->have_same_owner(idmap, inode, dir);
> > +			dput(parent);
> > +		}
> > +		return ret;
> > +	}
> 
> This about as ugly as it can get and costly...

I can break this out into a helper, but it should make no difference to the
actual code generated.

> > +	ret = vfs_inode_and_dir_have_same_owner(idmap, inode, nd);
> > +	if (ret <= 0)
> > +		return ret;
> 
> Ok, so while that doesn't exactly surface the error it's still weird.
> Please make that consistent. Either have those two new helper functions
> return negative error codes and zero on success or have it be a proper
> boolean instead so there's no possible confusion. This is just begging
> for someone to do if (ret) return ret and bubble up that positive return
> value.

The problem is that you have three available returns: Yes they do, no they
don't and some arbitrary error was encountered.  The first two are not error
cases, and potentially any error you pick to represent, say, "no" could also
be returned by the underlying filesystem.

David