[2/3] fs: namei: Allow follow_down() to uncover auto mounts

  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(-)
  

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).