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

Christian Brauner <[email protected]> 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 optimi=
ses
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 a=
ll.

Hence the unlikely().

> > +		struct dentry *parent;
> > +		struct inode *dir;
> > +		int ret;
> > +
> > +		if (inode !=3D nd->inode) {
> > +			dir =3D nd->inode;
> > +			ret =3D inode->i_op->have_same_owner(idmap, inode, dir);
> > +		} else if (nd->flags & LOOKUP_RCU) {
> > +			parent =3D READ_ONCE(nd->path.dentry);
> > +			dir =3D READ_ONCE(parent->d_inode);
> > +			if (!dir)
> > +				return -ECHILD;
> > +			ret =3D inode->i_op->have_same_owner(idmap, inode, dir);
> > +		} else {
> > +			parent =3D dget_parent(nd->path.dentry);
> > +			dir =3D parent->d_inode;
> > +			ret =3D 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 th=
e
actual code generated.

> > +	ret =3D vfs_inode_and_dir_have_same_owner(idmap, inode, nd);
> > +	if (ret <=3D 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 err=
or
cases, and potentially any error you pick to represent, say, "no" could al=
so
be returned by the underlying filesystem.

David