2008-05-06 19:51:01

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path()

On Tue, May 06, 2008 at 10:35:46AM +1000, Neil Brown wrote:
> On Monday May 5, [email protected] wrote:
> > On Mon, May 05, 2008 at 09:22:49AM +1000, Neil Brown wrote:
> > >
> > > Now it could be argued that this permission test is really a dumb idea
> > > that buys nothing and costs much. And if you were to queue a patch to
> > > get rid of it, I doubt you would get any objections .... certainly not
> > > from me :-)
> >
> > Dumb idea or not, it looks like it's explicitly documented in
> > exports(5):
> >
> > " subtree checking is also used to make sure that files
> > inside directories to which only root has access can only be
> > accessed if the filesystem is exported with no_root_squash
> > (see below), even if the file itself allows more general
> > access."
> >
> > So as much as I'd like to I'm not comfortable silently turning off that
> > check.
>
> Ack.
>
> >
> > I suppose we could choose to acquire those capabilities only in the
> > no_subtree_check case.
>
> If only it were that easy ;-)
>
> reconnect_path potentially requires both 'r' and 'x' permission on
> parent directories. 'r' to be able to read the directory to find the
> name of the object being reconnected, and 'x' to do the lookup which
> effects the reconnect.
>
> To fix the current bug properly, reconnect_path still needs to bypass
> normal permission checks even when subtree_check is in effect, so it
> can be sure of getting read permission on the parent directory.

OK, but why not just forget the subtree_check case? It would be just
another item on the "reasons not to use subtree_check" list.

If a fix for the subtree checking case were easy (or if someone else had
the time to do a very careful job of it), then fine, but maybe we should
just fix the easy case and leave the subtree checking as is for now.

--b.

>
> There is another way .... but it would need careful consideration.
>
> While the dentry returned by exportfs_decode_fh (for a directory) must
> be connected in the dcache tree, it does *not* need to have a correct
> name. All that is needed is that d_parent is correct (this is used,
> as mentioned before, to correctly lock directory renames).
>
> We can leave the dentry unhashed but with a correct d_parent pointer.
> If the directory is ever access by name, d_slice_alias will be called
> and this will update the name in the dentry to be correct.
>
> We could then get rid of exportfs_get_name and the call to
> lookup_one_len, and add some dcache magic after the ->get_parent call
> to make 'pd' an anonymous child of 'ppd'.
>
> Some matching changes to d_splice_alias should finish the task.
>
> Does this seem sane to anyone else? Is it worth a try?
>
>
> NeilBrown