| Message ID | 20221117191151.14262-3-richard@nod.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFS: NFSD: Allow crossing mounts when re-exporting | expand |
On Thu, 2022-11-17 at 20:11 +0100, Richard Weinberger wrote: > This function is only used by NFSD to cross mount points. > If a mount point is of type auto mount, follow_down() will > not uncover it. Add LOOKUP_AUTOMOUNT to the lookup flags > to have ->d_automount() called when NFSD walks down the > mount tree. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > fs/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/namei.c b/fs/namei.c > index 578c2110df02..000c4b84e6be 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -1462,7 +1462,7 @@ int follow_down(struct path *path) > { > struct vfsmount *mnt = path->mnt; > bool jumped; > - int ret = traverse_mounts(path, &jumped, NULL, 0); > + int ret = traverse_mounts(path, &jumped, NULL, LOOKUP_AUTOMOUNT); > > if (path->mnt != mnt) > mntput(mnt); What happens when CROSSMOUNT isn't enabled and someone tries to stroll into an automount point? I'm guessing the automount happens but the export is denied? It seems like LOOKUP_AUTOMOUNT ought to be conditional on the parent export having CROSSMOUNT set. There's also another caller of follow_down too, the UNIX98 pty code. This may be harmless for it, but it'd be best not to perturb that if we can help it. Maybe follow_down can grow a lookupflags argument?
----- Ursprüngliche Mail ----- > Von: "Jeff Layton" <jlayton@kernel.org> > What happens when CROSSMOUNT isn't enabled and someone tries to stroll > into an automount point? I'm guessing the automount happens but the > export is denied? Exactly. On the other hand, why should knfsd not trigger automounts? Almost any userspace interaction would also do so. > It seems like LOOKUP_AUTOMOUNT ought to be conditional > on the parent export having CROSSMOUNT set. > > There's also another caller of follow_down too, the UNIX98 pty code. > This may be harmless for it, but it'd be best not to perturb that if we > can help it. > > Maybe follow_down can grow a lookupflags argument? So, in nfsd_cross_mnt() the follow_down() helper should use LOOKUP_AUTOMOUNT only if exp->ex_flags & NFSEXP_CROSSMOUNT is true? Sounds sane, thanks for the pointer. Thanks, //richard
On Thu, 2022-11-17 at 22:12 +0100, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Jeff Layton" <jlayton@kernel.org> > > What happens when CROSSMOUNT isn't enabled and someone tries to stroll > > into an automount point? I'm guessing the automount happens but the > > export is denied? > > Exactly. > > On the other hand, why should knfsd not trigger automounts? > Almost any userspace interaction would also do so. > I have no issue with knfsd activity triggering an automount, but I think it'd be best if we don't do that when knfsd can't do anything with the resulting filesystem. Automounts can be expensive. > > It seems like LOOKUP_AUTOMOUNT ought to be conditional > > on the parent export having CROSSMOUNT set. > > > > There's also another caller of follow_down too, the UNIX98 pty code. > > This may be harmless for it, but it'd be best not to perturb that if we > > can help it. > > > > Maybe follow_down can grow a lookupflags argument? > > So, in nfsd_cross_mnt() the follow_down() helper should use LOOKUP_AUTOMOUNT only > if exp->ex_flags & NFSEXP_CROSSMOUNT is true? > Sounds sane, thanks for the pointer. > Yeah, I think so. I do wonder if we ought to make any provision for "nohide" exports, but since you have to enumerate those explicitly, it shouldn't be a huge problem for someone to just ensure that they're mounted beforehand.
On 18/11/22 05:01, Jeff Layton wrote: > On Thu, 2022-11-17 at 20:11 +0100, Richard Weinberger wrote: >> This function is only used by NFSD to cross mount points. >> If a mount point is of type auto mount, follow_down() will >> not uncover it. Add LOOKUP_AUTOMOUNT to the lookup flags >> to have ->d_automount() called when NFSD walks down the >> mount tree. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> fs/namei.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/namei.c b/fs/namei.c >> index 578c2110df02..000c4b84e6be 100644 >> --- a/fs/namei.c >> +++ b/fs/namei.c >> @@ -1462,7 +1462,7 @@ int follow_down(struct path *path) >> { >> struct vfsmount *mnt = path->mnt; >> bool jumped; >> - int ret = traverse_mounts(path, &jumped, NULL, 0); >> + int ret = traverse_mounts(path, &jumped, NULL, LOOKUP_AUTOMOUNT); >> >> if (path->mnt != mnt) >> mntput(mnt); > > What happens when CROSSMOUNT isn't enabled and someone tries to stroll > into an automount point? I'm guessing the automount happens but the > export is denied? It seems like LOOKUP_AUTOMOUNT ought to be conditional > on the parent export having CROSSMOUNT set. > > There's also another caller of follow_down too, the UNIX98 pty code. > This may be harmless for it, but it'd be best not to perturb that if we > can help it. > > Maybe follow_down can grow a lookupflags argument?es, I think that's needed too. Changing the core VFS unconditionally ricks breaking things. For example this: if (!(lookup_flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY | LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT)) && dentry->d_inode) will never be true now so that, at the least, the handling of this case will change for automount(8). I don't remember now the reasons behind doing this but I do remember there was a special case that needed to be handled by it. Ian
----- Ursprüngliche Mail ----- > Von: "Jeff Layton" <jlayton@kernel.org> >> So, in nfsd_cross_mnt() the follow_down() helper should use LOOKUP_AUTOMOUNT >> only >> if exp->ex_flags & NFSEXP_CROSSMOUNT is true? >> Sounds sane, thanks for the pointer. >> > > Yeah, I think so. I do wonder if we ought to make any provision for > "nohide" exports, but since you have to enumerate those explicitly, it > shouldn't be a huge problem for someone to just ensure that they're > mounted beforehand. TBH, I didn't invest much into the nohide feature wrt. NFS re-exporting. What problem do you have in mind? I wonder also what NFS client folks think about my changes before I send the next revision (with Jeff's comments addressed). Thanks, //richard
On Sun, 2022-11-27 at 22:29 +0100, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Jeff Layton" <jlayton@kernel.org> > > > So, in nfsd_cross_mnt() the follow_down() helper should use LOOKUP_AUTOMOUNT > > > only > > > if exp->ex_flags & NFSEXP_CROSSMOUNT is true? > > > Sounds sane, thanks for the pointer. > > > > > > > Yeah, I think so. I do wonder if we ought to make any provision for > > "nohide" exports, but since you have to enumerate those explicitly, it > > shouldn't be a huge problem for someone to just ensure that they're > > mounted beforehand. > > TBH, I didn't invest much into the nohide feature wrt. NFS re-exporting. > What problem do you have in mind? > nohide is sort of complimentary to crossmnt. You can achieve the same effect as crossmnt by adding explicit exports for all the children and marking them "nohide". The point here is that you have to explicitly create exports for the child mounts in that case, and if you're doing that then it's not a burden for the admin to make sure they're mounted before exporting. So, I don't think we need to worry about nohide here after all. > I wonder also what NFS client folks think about my changes before I send > the next revision (with Jeff's comments addressed).
diff --git a/fs/namei.c b/fs/namei.c index 578c2110df02..000c4b84e6be 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1462,7 +1462,7 @@ int follow_down(struct path *path) { struct vfsmount *mnt = path->mnt; bool jumped; - int ret = traverse_mounts(path, &jumped, NULL, 0); + int ret = traverse_mounts(path, &jumped, NULL, LOOKUP_AUTOMOUNT); if (path->mnt != mnt) mntput(mnt);
This function is only used by NFSD to cross mount points. If a mount point is of type auto mount, follow_down() will not uncover it. Add LOOKUP_AUTOMOUNT to the lookup flags to have ->d_automount() called when NFSD walks down the mount tree. Signed-off-by: Richard Weinberger <richard@nod.at> --- fs/namei.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)