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

[David Howells <dhowells@redhat.com>]

Christian Brauner <brauner@kernel.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