diff mbox

Do we really need d_weak_revalidate???

Message ID 87r2w3jdn5.fsf@notabene.neil.brown.name (mailing list archive)
State New, archived
Headers show

Commit Message

NeilBrown Aug. 23, 2017, 1:06 a.m. UTC
On Mon, Aug 21 2017, Ian Kent wrote:

>> 
>> A mount isn't triggered by kern_path(pathname, 0, &path).
>> That '0' would need to include one of
>>   LOOKUP_PARENT | LOOKUP_DIRECTORY |
>>   LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT
>> 
>> to trigger an automount (otherwise you just get -EISDIR).
>
> It's perfectly sensible to think that but there is a case where a
> a mount is triggered when using kern_path().
>
> The EISDIR return occurs for positive dentrys, negative dentrys
> will still trigger an automount (which is autofs specific,
> indirect mount map using nobrowse option, the install default).

Ok, I understand this better now.  This difference between direct and
indirect mounts is slightly awkward. It is visible from user-space, but
not elegant to document.
When you use O_PATH to open a direct automount that has not already been
triggered, the open returns the underlying directory (and fstatfs
confirms that it is AUTOFS_SUPER_MAGIC).  When you use O_PATH on
an indirect automount, it *will* trigger the automount when "nobrowse" is
in effect, but it won't when "browse" is in effect.

So we cannot just say "O_PATH doesn't trigger automounts", which is
essentially what I said in

https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=97a45d02e6671482e8b2cdcce3951930bf6bdb94

It might be possible to modify automount so that it was more consistent
- i.e. if the point is triggered by a mkdir has been done, just to the
mkdir.  If it is triggered after a mkdir has been done, do the mount.  I
guess that might be racy, and in any case is hard to justify.

Maybe I should change it to be about "direct automounts", and add a note
that indirect automounts aren't so predictable.

But back to my original issue of wanting to discard
kern_path_mountpoint, what would you think of the following approach -
slight revised from before.

Thanks,
NeilBrown

Comments

Ian Kent Aug. 23, 2017, 2:32 a.m. UTC | #1
On 23/08/17 09:06, NeilBrown wrote:
> On Mon, Aug 21 2017, Ian Kent wrote:
> 
>>>
>>> A mount isn't triggered by kern_path(pathname, 0, &path).
>>> That '0' would need to include one of
>>>   LOOKUP_PARENT | LOOKUP_DIRECTORY |
>>>   LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT
>>>
>>> to trigger an automount (otherwise you just get -EISDIR).
>>
>> It's perfectly sensible to think that but there is a case where a
>> a mount is triggered when using kern_path().
>>
>> The EISDIR return occurs for positive dentrys, negative dentrys
>> will still trigger an automount (which is autofs specific,
>> indirect mount map using nobrowse option, the install default).
> 
> Ok, I understand this better now.  This difference between direct and
> indirect mounts is slightly awkward. It is visible from user-space, but
> not elegant to document.
> When you use O_PATH to open a direct automount that has not already been
> triggered, the open returns the underlying directory (and fstatfs
> confirms that it is AUTOFS_SUPER_MAGIC).  When you use O_PATH on
> an indirect automount, it *will* trigger the automount when "nobrowse" is
> in effect, but it won't when "browse" is in effect.

That inconsistency has bothered me for quite a while now.

It was carried over from the autofs module behavior when automounting
support was added to the VFS. What's worse is it prevents the use of
the AT_NO_AUTOMOUNT flag from working properly with fstatat(2) and with
statx().

There is some risk in changing that so it does work but it really does
need to work to enable userspace to not trigger an automount by using
this flag.

So that's (hopefully) going to change soonish, see:
http://ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-at_no_automount-not-being-honored.patch

The result should be that stat family calls don't trigger automounts except
for fstatat(2) and statx() which will require the AT_NO_AUTOMOUNT flag.

> 
> So we cannot just say "O_PATH doesn't trigger automounts", which is
> essentially what I said in
> 
> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=97a45d02e6671482e8b2cdcce3951930bf6bdb94
> 
> It might be possible to modify automount so that it was more consistent
> - i.e. if the point is triggered by a mkdir has been done, just to the
> mkdir.  If it is triggered after a mkdir has been done, do the mount.  I
> guess that might be racy, and in any case is hard to justify.
> 
> Maybe I should change it to be about "direct automounts", and add a note
> that indirect automounts aren't so predictable.

Right and the semantics should be much more consistent in the near future.
I hope (and expect) this semantic change won't cause problems.

> 
> But back to my original issue of wanting to discard
> kern_path_mountpoint, what would you think of the following approach -
> slight revised from before.
> 
> Thanks,
> NeilBrown
> 
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index beef981aa54f..7663ea82e68d 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -135,10 +135,13 @@ static inline struct autofs_info *autofs4_dentry_ino(struct dentry *dentry)
>  /* autofs4_oz_mode(): do we see the man behind the curtain?  (The
>   * processes which do manipulations for us in user space sees the raw
>   * filesystem without "magic".)
> + * A process performing certain ioctls can get temporary oz status.
>   */
> +extern struct task_struct *autofs_tmp_oz;
>  static inline int autofs4_oz_mode(struct autofs_sb_info *sbi)
>  {
> -	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
> +	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp ||
> +		autofs_tmp_oz == current;
>  }
>  
>  struct inode *autofs4_get_inode(struct super_block *, umode_t);
> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> index dd9f1bebb5a3..d76401669a20 100644
> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -200,6 +200,20 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
>  	return 0;
>  }
>  
> +struct task_struct *autofs_tmp_oz;
> +int kern_path_oz(const char *pathname, int flags, struct path *path)
> +{
> +	static DEFINE_MUTEX(autofs_oz);
> +	int err;
> +
> +	mutex_lock(&autofs_oz);
> +	autofs_tmp_oz = current;
> +	err = kern_path(pathname, flags, path);
> +	autofs_tmp_oz = NULL;
> +	mutex_unlock(&autofs_oz);
> +	return err;
> +}
> +

It's simple enough but does look like it will attract criticism as being
a hack!

The kern_path_locked() function is very similar to what was originally
done, along with code to look down the mount stack (rather than up the
way it does now) to get the mount point. In this case, to be valid the
dentry can't be a symlink so that fits kern_path_locked() too.

So maybe it is worth going back to the way it was in the beginning and
be done with it .... OTOH Al must have had a reason for changing the
way it was done that I didn't get.

>  /* Find the topmost mount satisfying test() */
>  static int find_autofs_mount(const char *pathname,
>  			     struct path *res,
> @@ -209,7 +223,8 @@ static int find_autofs_mount(const char *pathname,
>  	struct path path;
>  	int err;
>  
> -	err = kern_path_mountpoint(AT_FDCWD, pathname, &path, 0);
> +	err = kern_path_oz(pathname, 0, &path);
> +
>  	if (err)
>  		return err;
>  	err = -ENOENT;
> @@ -552,8 +567,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
>  
>  	if (!fp || param->ioctlfd == -1) {
>  		if (autofs_type_any(type))
> -			err = kern_path_mountpoint(AT_FDCWD,
> -						   name, &path, LOOKUP_FOLLOW);
> +			err = kern_path_oz(name, LOOKUP_FOLLOW, &path);
>  		else
>  			err = find_autofs_mount(name, &path,
>  						test_by_type, &type);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Kent Aug. 23, 2017, 2:40 a.m. UTC | #2
On 23/08/17 10:32, Ian Kent wrote:
> On 23/08/17 09:06, NeilBrown wrote:
>> On Mon, Aug 21 2017, Ian Kent wrote:
>>
>>>>
>>>> A mount isn't triggered by kern_path(pathname, 0, &path).
>>>> That '0' would need to include one of
>>>>   LOOKUP_PARENT | LOOKUP_DIRECTORY |
>>>>   LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT
>>>>
>>>> to trigger an automount (otherwise you just get -EISDIR).
>>>
>>> It's perfectly sensible to think that but there is a case where a
>>> a mount is triggered when using kern_path().
>>>
>>> The EISDIR return occurs for positive dentrys, negative dentrys
>>> will still trigger an automount (which is autofs specific,
>>> indirect mount map using nobrowse option, the install default).
>>
>> Ok, I understand this better now.  This difference between direct and
>> indirect mounts is slightly awkward. It is visible from user-space, but
>> not elegant to document.
>> When you use O_PATH to open a direct automount that has not already been
>> triggered, the open returns the underlying directory (and fstatfs
>> confirms that it is AUTOFS_SUPER_MAGIC).  When you use O_PATH on
>> an indirect automount, it *will* trigger the automount when "nobrowse" is
>> in effect, but it won't when "browse" is in effect.
> 
> That inconsistency has bothered me for quite a while now.
> 
> It was carried over from the autofs module behavior when automounting
> support was added to the VFS. What's worse is it prevents the use of
> the AT_NO_AUTOMOUNT flag from working properly with fstatat(2) and with
> statx().
> 
> There is some risk in changing that so it does work but it really does
> need to work to enable userspace to not trigger an automount by using
> this flag.
> 
> So that's (hopefully) going to change soonish, see:
> http://ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-at_no_automount-not-being-honored.patch
> 
> The result should be that stat family calls don't trigger automounts except
> for fstatat(2) and statx() which will require the AT_NO_AUTOMOUNT flag.
> 
>>
>> So we cannot just say "O_PATH doesn't trigger automounts", which is
>> essentially what I said in
>>
>> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=97a45d02e6671482e8b2cdcce3951930bf6bdb94
>>
>> It might be possible to modify automount so that it was more consistent
>> - i.e. if the point is triggered by a mkdir has been done, just to the
>> mkdir.  If it is triggered after a mkdir has been done, do the mount.  I
>> guess that might be racy, and in any case is hard to justify.
>>
>> Maybe I should change it to be about "direct automounts", and add a note
>> that indirect automounts aren't so predictable.
> 
> Right and the semantics should be much more consistent in the near future.
> I hope (and expect) this semantic change won't cause problems.
> 
>>
>> But back to my original issue of wanting to discard
>> kern_path_mountpoint, what would you think of the following approach -
>> slight revised from before.
>>
>> Thanks,
>> NeilBrown
>>
>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>> index beef981aa54f..7663ea82e68d 100644
>> --- a/fs/autofs4/autofs_i.h
>> +++ b/fs/autofs4/autofs_i.h
>> @@ -135,10 +135,13 @@ static inline struct autofs_info *autofs4_dentry_ino(struct dentry *dentry)
>>  /* autofs4_oz_mode(): do we see the man behind the curtain?  (The
>>   * processes which do manipulations for us in user space sees the raw
>>   * filesystem without "magic".)
>> + * A process performing certain ioctls can get temporary oz status.
>>   */
>> +extern struct task_struct *autofs_tmp_oz;
>>  static inline int autofs4_oz_mode(struct autofs_sb_info *sbi)
>>  {
>> -	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
>> +	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp ||
>> +		autofs_tmp_oz == current;
>>  }
>>  
>>  struct inode *autofs4_get_inode(struct super_block *, umode_t);
>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
>> index dd9f1bebb5a3..d76401669a20 100644
>> --- a/fs/autofs4/dev-ioctl.c
>> +++ b/fs/autofs4/dev-ioctl.c
>> @@ -200,6 +200,20 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
>>  	return 0;
>>  }
>>  
>> +struct task_struct *autofs_tmp_oz;
>> +int kern_path_oz(const char *pathname, int flags, struct path *path)
>> +{
>> +	static DEFINE_MUTEX(autofs_oz);
>> +	int err;
>> +
>> +	mutex_lock(&autofs_oz);
>> +	autofs_tmp_oz = current;
>> +	err = kern_path(pathname, flags, path);
>> +	autofs_tmp_oz = NULL;
>> +	mutex_unlock(&autofs_oz);
>> +	return err;
>> +}
>> +
> 
> It's simple enough but does look like it will attract criticism as being
> a hack!
> 
> The kern_path_locked() function is very similar to what was originally
> done, along with code to look down the mount stack (rather than up the
> way it does now) to get the mount point. In this case, to be valid the
> dentry can't be a symlink so that fits kern_path_locked() too.

Oh wait, that __lookup_hash() tries too hard to resolve the dentry,
that won't quite work, and maybe d_lookup() can't be used safely in
this context either ....

> 
> So maybe it is worth going back to the way it was in the beginning and
> be done with it .... OTOH Al must have had a reason for changing the
> way it was done that I didn't get.
> 
>>  /* Find the topmost mount satisfying test() */
>>  static int find_autofs_mount(const char *pathname,
>>  			     struct path *res,
>> @@ -209,7 +223,8 @@ static int find_autofs_mount(const char *pathname,
>>  	struct path path;
>>  	int err;
>>  
>> -	err = kern_path_mountpoint(AT_FDCWD, pathname, &path, 0);
>> +	err = kern_path_oz(pathname, 0, &path);
>> +
>>  	if (err)
>>  		return err;
>>  	err = -ENOENT;
>> @@ -552,8 +567,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
>>  
>>  	if (!fp || param->ioctlfd == -1) {
>>  		if (autofs_type_any(type))
>> -			err = kern_path_mountpoint(AT_FDCWD,
>> -						   name, &path, LOOKUP_FOLLOW);
>> +			err = kern_path_oz(name, LOOKUP_FOLLOW, &path);
>>  		else
>>  			err = find_autofs_mount(name, &path,
>>  						test_by_type, &type);
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Kent Aug. 23, 2017, 2:54 a.m. UTC | #3
On 23/08/17 10:40, Ian Kent wrote:
> On 23/08/17 10:32, Ian Kent wrote:
>> On 23/08/17 09:06, NeilBrown wrote:
>>> On Mon, Aug 21 2017, Ian Kent wrote:
>>>
>>>>>
>>>>> A mount isn't triggered by kern_path(pathname, 0, &path).
>>>>> That '0' would need to include one of
>>>>>   LOOKUP_PARENT | LOOKUP_DIRECTORY |
>>>>>   LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT
>>>>>
>>>>> to trigger an automount (otherwise you just get -EISDIR).
>>>>
>>>> It's perfectly sensible to think that but there is a case where a
>>>> a mount is triggered when using kern_path().
>>>>
>>>> The EISDIR return occurs for positive dentrys, negative dentrys
>>>> will still trigger an automount (which is autofs specific,
>>>> indirect mount map using nobrowse option, the install default).
>>>
>>> Ok, I understand this better now.  This difference between direct and
>>> indirect mounts is slightly awkward. It is visible from user-space, but
>>> not elegant to document.
>>> When you use O_PATH to open a direct automount that has not already been
>>> triggered, the open returns the underlying directory (and fstatfs
>>> confirms that it is AUTOFS_SUPER_MAGIC).  When you use O_PATH on
>>> an indirect automount, it *will* trigger the automount when "nobrowse" is
>>> in effect, but it won't when "browse" is in effect.
>>
>> That inconsistency has bothered me for quite a while now.
>>
>> It was carried over from the autofs module behavior when automounting
>> support was added to the VFS. What's worse is it prevents the use of
>> the AT_NO_AUTOMOUNT flag from working properly with fstatat(2) and with
>> statx().
>>
>> There is some risk in changing that so it does work but it really does
>> need to work to enable userspace to not trigger an automount by using
>> this flag.
>>
>> So that's (hopefully) going to change soonish, see:
>> http://ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-at_no_automount-not-being-honored.patch
>>
>> The result should be that stat family calls don't trigger automounts except
>> for fstatat(2) and statx() which will require the AT_NO_AUTOMOUNT flag.
>>
>>>
>>> So we cannot just say "O_PATH doesn't trigger automounts", which is
>>> essentially what I said in
>>>
>>> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=97a45d02e6671482e8b2cdcce3951930bf6bdb94
>>>
>>> It might be possible to modify automount so that it was more consistent
>>> - i.e. if the point is triggered by a mkdir has been done, just to the
>>> mkdir.  If it is triggered after a mkdir has been done, do the mount.  I
>>> guess that might be racy, and in any case is hard to justify.
>>>
>>> Maybe I should change it to be about "direct automounts", and add a note
>>> that indirect automounts aren't so predictable.
>>
>> Right and the semantics should be much more consistent in the near future.
>> I hope (and expect) this semantic change won't cause problems.
>>
>>>
>>> But back to my original issue of wanting to discard
>>> kern_path_mountpoint, what would you think of the following approach -
>>> slight revised from before.
>>>
>>> Thanks,
>>> NeilBrown
>>>
>>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>>> index beef981aa54f..7663ea82e68d 100644
>>> --- a/fs/autofs4/autofs_i.h
>>> +++ b/fs/autofs4/autofs_i.h
>>> @@ -135,10 +135,13 @@ static inline struct autofs_info *autofs4_dentry_ino(struct dentry *dentry)
>>>  /* autofs4_oz_mode(): do we see the man behind the curtain?  (The
>>>   * processes which do manipulations for us in user space sees the raw
>>>   * filesystem without "magic".)
>>> + * A process performing certain ioctls can get temporary oz status.
>>>   */
>>> +extern struct task_struct *autofs_tmp_oz;
>>>  static inline int autofs4_oz_mode(struct autofs_sb_info *sbi)
>>>  {
>>> -	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
>>> +	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp ||
>>> +		autofs_tmp_oz == current;
>>>  }
>>>  
>>>  struct inode *autofs4_get_inode(struct super_block *, umode_t);
>>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
>>> index dd9f1bebb5a3..d76401669a20 100644
>>> --- a/fs/autofs4/dev-ioctl.c
>>> +++ b/fs/autofs4/dev-ioctl.c
>>> @@ -200,6 +200,20 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
>>>  	return 0;
>>>  }
>>>  
>>> +struct task_struct *autofs_tmp_oz;
>>> +int kern_path_oz(const char *pathname, int flags, struct path *path)
>>> +{
>>> +	static DEFINE_MUTEX(autofs_oz);
>>> +	int err;
>>> +
>>> +	mutex_lock(&autofs_oz);
>>> +	autofs_tmp_oz = current;
>>> +	err = kern_path(pathname, flags, path);
>>> +	autofs_tmp_oz = NULL;
>>> +	mutex_unlock(&autofs_oz);
>>> +	return err;
>>> +}
>>> +
>>
>> It's simple enough but does look like it will attract criticism as being
>> a hack!
>>
>> The kern_path_locked() function is very similar to what was originally
>> done, along with code to look down the mount stack (rather than up the
>> way it does now) to get the mount point. In this case, to be valid the
>> dentry can't be a symlink so that fits kern_path_locked() too.
> 
> Oh wait, that __lookup_hash() tries too hard to resolve the dentry,
> that won't quite work, and maybe d_lookup() can't be used safely in
> this context either ....

Umm .. d_lookup() does look ok so maybe path_parentat() + d_lookup()
would be ok.

> 
>>
>> So maybe it is worth going back to the way it was in the beginning and
>> be done with it .... OTOH Al must have had a reason for changing the
>> way it was done that I didn't get.
>>
>>>  /* Find the topmost mount satisfying test() */
>>>  static int find_autofs_mount(const char *pathname,
>>>  			     struct path *res,
>>> @@ -209,7 +223,8 @@ static int find_autofs_mount(const char *pathname,
>>>  	struct path path;
>>>  	int err;
>>>  
>>> -	err = kern_path_mountpoint(AT_FDCWD, pathname, &path, 0);
>>> +	err = kern_path_oz(pathname, 0, &path);
>>> +
>>>  	if (err)
>>>  		return err;
>>>  	err = -ENOENT;
>>> @@ -552,8 +567,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
>>>  
>>>  	if (!fp || param->ioctlfd == -1) {
>>>  		if (autofs_type_any(type))
>>> -			err = kern_path_mountpoint(AT_FDCWD,
>>> -						   name, &path, LOOKUP_FOLLOW);
>>> +			err = kern_path_oz(name, LOOKUP_FOLLOW, &path);
>>>  		else
>>>  			err = find_autofs_mount(name, &path,
>>>  						test_by_type, &type);
>>>
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Kent Aug. 23, 2017, 7:51 a.m. UTC | #4
On 23/08/17 10:54, Ian Kent wrote:
> On 23/08/17 10:40, Ian Kent wrote:
>> On 23/08/17 10:32, Ian Kent wrote:
>>> On 23/08/17 09:06, NeilBrown wrote:
>>>> On Mon, Aug 21 2017, Ian Kent wrote:
>>>>
>>>>>>
>>>>>> A mount isn't triggered by kern_path(pathname, 0, &path).
>>>>>> That '0' would need to include one of
>>>>>>   LOOKUP_PARENT | LOOKUP_DIRECTORY |
>>>>>>   LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT
>>>>>>
>>>>>> to trigger an automount (otherwise you just get -EISDIR).
>>>>>
>>>>> It's perfectly sensible to think that but there is a case where a
>>>>> a mount is triggered when using kern_path().
>>>>>
>>>>> The EISDIR return occurs for positive dentrys, negative dentrys
>>>>> will still trigger an automount (which is autofs specific,
>>>>> indirect mount map using nobrowse option, the install default).
>>>>
>>>> Ok, I understand this better now.  This difference between direct and
>>>> indirect mounts is slightly awkward. It is visible from user-space, but
>>>> not elegant to document.
>>>> When you use O_PATH to open a direct automount that has not already been
>>>> triggered, the open returns the underlying directory (and fstatfs
>>>> confirms that it is AUTOFS_SUPER_MAGIC).  When you use O_PATH on
>>>> an indirect automount, it *will* trigger the automount when "nobrowse" is
>>>> in effect, but it won't when "browse" is in effect.
>>>
>>> That inconsistency has bothered me for quite a while now.
>>>
>>> It was carried over from the autofs module behavior when automounting
>>> support was added to the VFS. What's worse is it prevents the use of
>>> the AT_NO_AUTOMOUNT flag from working properly with fstatat(2) and with
>>> statx().
>>>
>>> There is some risk in changing that so it does work but it really does
>>> need to work to enable userspace to not trigger an automount by using
>>> this flag.
>>>
>>> So that's (hopefully) going to change soonish, see:
>>> http://ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-at_no_automount-not-being-honored.patch
>>>
>>> The result should be that stat family calls don't trigger automounts except
>>> for fstatat(2) and statx() which will require the AT_NO_AUTOMOUNT flag.
>>>
>>>>
>>>> So we cannot just say "O_PATH doesn't trigger automounts", which is
>>>> essentially what I said in
>>>>
>>>> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=97a45d02e6671482e8b2cdcce3951930bf6bdb94
>>>>
>>>> It might be possible to modify automount so that it was more consistent
>>>> - i.e. if the point is triggered by a mkdir has been done, just to the
>>>> mkdir.  If it is triggered after a mkdir has been done, do the mount.  I
>>>> guess that might be racy, and in any case is hard to justify.
>>>>
>>>> Maybe I should change it to be about "direct automounts", and add a note
>>>> that indirect automounts aren't so predictable.
>>>
>>> Right and the semantics should be much more consistent in the near future.
>>> I hope (and expect) this semantic change won't cause problems.
>>>
>>>>
>>>> But back to my original issue of wanting to discard
>>>> kern_path_mountpoint, what would you think of the following approach -
>>>> slight revised from before.
>>>>
>>>> Thanks,
>>>> NeilBrown
>>>>
>>>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>>>> index beef981aa54f..7663ea82e68d 100644
>>>> --- a/fs/autofs4/autofs_i.h
>>>> +++ b/fs/autofs4/autofs_i.h
>>>> @@ -135,10 +135,13 @@ static inline struct autofs_info *autofs4_dentry_ino(struct dentry *dentry)
>>>>  /* autofs4_oz_mode(): do we see the man behind the curtain?  (The
>>>>   * processes which do manipulations for us in user space sees the raw
>>>>   * filesystem without "magic".)
>>>> + * A process performing certain ioctls can get temporary oz status.
>>>>   */
>>>> +extern struct task_struct *autofs_tmp_oz;
>>>>  static inline int autofs4_oz_mode(struct autofs_sb_info *sbi)
>>>>  {
>>>> -	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
>>>> +	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp ||
>>>> +		autofs_tmp_oz == current;
>>>>  }
>>>>  
>>>>  struct inode *autofs4_get_inode(struct super_block *, umode_t);
>>>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
>>>> index dd9f1bebb5a3..d76401669a20 100644
>>>> --- a/fs/autofs4/dev-ioctl.c
>>>> +++ b/fs/autofs4/dev-ioctl.c
>>>> @@ -200,6 +200,20 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +struct task_struct *autofs_tmp_oz;
>>>> +int kern_path_oz(const char *pathname, int flags, struct path *path)
>>>> +{
>>>> +	static DEFINE_MUTEX(autofs_oz);
>>>> +	int err;
>>>> +
>>>> +	mutex_lock(&autofs_oz);
>>>> +	autofs_tmp_oz = current;
>>>> +	err = kern_path(pathname, flags, path);
>>>> +	autofs_tmp_oz = NULL;
>>>> +	mutex_unlock(&autofs_oz);
>>>> +	return err;
>>>> +}
>>>> +
>>>
>>> It's simple enough but does look like it will attract criticism as being
>>> a hack!
>>>
>>> The kern_path_locked() function is very similar to what was originally
>>> done, along with code to look down the mount stack (rather than up the
>>> way it does now) to get the mount point. In this case, to be valid the
>>> dentry can't be a symlink so that fits kern_path_locked() too.
>>
>> Oh wait, that __lookup_hash() tries too hard to resolve the dentry,
>> that won't quite work, and maybe d_lookup() can't be used safely in
>> this context either ....
> 
> Umm .. d_lookup() does look ok so maybe path_parentat() + d_lookup()
> would be ok.

Double Umm ... with the patch above kern_path() with flags 0 or
LOOKUP_FOLLOW should get either EISDIR or ENOENT ... maybe I should
think occasionally !!  

> 
>>
>>>
>>> So maybe it is worth going back to the way it was in the beginning and
>>> be done with it .... OTOH Al must have had a reason for changing the
>>> way it was done that I didn't get.
>>>
>>>>  /* Find the topmost mount satisfying test() */
>>>>  static int find_autofs_mount(const char *pathname,
>>>>  			     struct path *res,
>>>> @@ -209,7 +223,8 @@ static int find_autofs_mount(const char *pathname,
>>>>  	struct path path;
>>>>  	int err;
>>>>  
>>>> -	err = kern_path_mountpoint(AT_FDCWD, pathname, &path, 0);
>>>> +	err = kern_path_oz(pathname, 0, &path);
>>>> +
>>>>  	if (err)
>>>>  		return err;
>>>>  	err = -ENOENT;
>>>> @@ -552,8 +567,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
>>>>  
>>>>  	if (!fp || param->ioctlfd == -1) {
>>>>  		if (autofs_type_any(type))
>>>> -			err = kern_path_mountpoint(AT_FDCWD,
>>>> -						   name, &path, LOOKUP_FOLLOW);
>>>> +			err = kern_path_oz(name, LOOKUP_FOLLOW, &path);
>>>>  		else
>>>>  			err = find_autofs_mount(name, &path,
>>>>  						test_by_type, &type);
>>>>
>>>
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
NeilBrown Aug. 24, 2017, 4:07 a.m. UTC | #5
On Wed, Aug 23 2017, Ian Kent wrote:

>
> That inconsistency has bothered me for quite a while now.
>
> It was carried over from the autofs module behavior when automounting
> support was added to the VFS. What's worse is it prevents the use of
> the AT_NO_AUTOMOUNT flag from working properly with fstatat(2) and with
> statx().
>
> There is some risk in changing that so it does work but it really does
> need to work to enable userspace to not trigger an automount by using
> this flag.
>
> So that's (hopefully) going to change soonish, see:
> http://ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-at_no_automount-not-being-honored.patch
>
> The result should be that stat family calls don't trigger automounts except
> for fstatat(2) and statx() which will require the AT_NO_AUTOMOUNT flag.
>

oooh, yes.  That's much better - thanks.

We should make sure that change gets into the man pages...

First however, we should probably correct the man page!
stat.2 says:


  NOTES
       On Linux, lstat() will generally not trigger  automounter
       action,  whereas  stat()  will  (but  see  the description of
       fstatat() AT_NO_AUTOMOUNT fag, above).

which is wrong: lstat and stat treat automounts the same.
@Michael: do you recall why you inserted that text?  The commit message
in commit 1ef5b2805471 ("stat.2: Cosmetic reworking of timestamp
discussion in NOTES") is not very helpful.

I propose correcting to

  NOTES:
      On Linux, lstat() nor stat() act as though AT_NO_AUTOMOUNT was set
      and will not trigger automounter action for direct automount
      points, though they may (prior to 4.14) for indirect automount
      points.


The more precise details, that automount action for indirect automount
points is not triggered when the 'browse' option is used, is probably
not necessary.

Ian: if you agree with that text, and Michael doesn't provide alternate
evidence, I'll submit a formal patch for the man page.... or should we
just wait until the patch actually lands?

Thanks,
NeilBrown
Ian Kent Aug. 24, 2017, 4:47 a.m. UTC | #6
On 24/08/17 12:07, NeilBrown wrote:
> On Wed, Aug 23 2017, Ian Kent wrote:
> 
>>
>> That inconsistency has bothered me for quite a while now.
>>
>> It was carried over from the autofs module behavior when automounting
>> support was added to the VFS. What's worse is it prevents the use of
>> the AT_NO_AUTOMOUNT flag from working properly with fstatat(2) and with
>> statx().
>>
>> There is some risk in changing that so it does work but it really does
>> need to work to enable userspace to not trigger an automount by using
>> this flag.
>>
>> So that's (hopefully) going to change soonish, see:
>> http://ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-at_no_automount-not-being-honored.patch
>>
>> The result should be that stat family calls don't trigger automounts except
>> for fstatat(2) and statx() which will require the AT_NO_AUTOMOUNT flag.
>>
> 
> oooh, yes.  That's much better - thanks.
> 
> We should make sure that change gets into the man pages...

Yes, I was wondering who to contact for that.

> 
> First however, we should probably correct the man page!
> stat.2 says:
> 
> 
>   NOTES
>        On Linux, lstat() will generally not trigger  automounter
>        action,  whereas  stat()  will  (but  see  the description of
>        fstatat() AT_NO_AUTOMOUNT fag, above).
> 
> which is wrong: lstat and stat treat automounts the same.
> @Michael: do you recall why you inserted that text?  The commit message
> in commit 1ef5b2805471 ("stat.2: Cosmetic reworking of timestamp
> discussion in NOTES") is not very helpful.
> 
> I propose correcting to
> 
>   NOTES:
>       On Linux, lstat() nor stat() act as though AT_NO_AUTOMOUNT was set
>       and will not trigger automounter action for direct automount
>       points, though they may (prior to 4.14) for indirect automount
>       points.

Shouldn't that be "lstat() and stat() act as though AT_NO_AUTOMOUNT is
set ..."

> 
> 
> The more precise details, that automount action for indirect automount
> points is not triggered when the 'browse' option is used, is probably
> not necessary.
> 
> Ian: if you agree with that text, and Michael doesn't provide alternate
> evidence, I'll submit a formal patch for the man page.... or should we
> just wait until the patch actually lands?

I thought the fstatat() description needed attention too, doubly so with
the AT_NO_AUTOMOUNT change.

The "The fstatat() system call operates in exactly the same way as stat()"
is wrong in the same way as the stat() description was wrong.

After the change fstatat() will trigger automounts if the AT_NO_AUTOMOUNT
flag is not given which is different from lstat() and stat().

The updated NOTE above probably needs to be referred to in order to clarify
what's meant by "in exactly the same way" since that probably refers to the
information returned rather than whether an automount will be done.

Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Kent Aug. 24, 2017, 4:58 a.m. UTC | #7
On 24/08/17 12:07, NeilBrown wrote:
> 
> 
> The more precise details, that automount action for indirect automount
> points is not triggered when the 'browse' option is used, is probably
> not necessary.
> 
> Ian: if you agree with that text, and Michael doesn't provide alternate
> evidence, I'll submit a formal patch for the man page.... or should we
> just wait until the patch actually lands?

So far only David commented about using ENOENT rather than EREMOTE.

I prefer ENOENT for this case myself and he didn't object when I
explained why, David, any concerns?

Al has been silent so far so either he hasn't seen it or he's ok with
it, Al, any concerns?

And I guess if there are no concerns there's a good chance Andrew is
ok with it for the next merge window, Andrew?

If everyone agrees then we could go ahead immediately so there's a
better chance of getting it into released man pages closer to the
change being merged.

Ian
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Kerrisk (man-pages) Aug. 24, 2017, 11:03 a.m. UTC | #8
Hi Neil,

On 24 August 2017 at 06:07, NeilBrown <neilb@suse.com> wrote:
> On Wed, Aug 23 2017, Ian Kent wrote:
>
>>
>> That inconsistency has bothered me for quite a while now.
>>
>> It was carried over from the autofs module behavior when automounting
>> support was added to the VFS. What's worse is it prevents the use of
>> the AT_NO_AUTOMOUNT flag from working properly with fstatat(2) and with
>> statx().
>>
>> There is some risk in changing that so it does work but it really does
>> need to work to enable userspace to not trigger an automount by using
>> this flag.
>>
>> So that's (hopefully) going to change soonish, see:
>> http://ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-at_no_automount-not-being-honored.patch
>>
>> The result should be that stat family calls don't trigger automounts except
>> for fstatat(2) and statx() which will require the AT_NO_AUTOMOUNT flag.
>>
>
> oooh, yes.  That's much better - thanks.
>
> We should make sure that change gets into the man pages...
>
> First however, we should probably correct the man page!
> stat.2 says:
>
>
>   NOTES
>        On Linux, lstat() will generally not trigger  automounter
>        action,  whereas  stat()  will  (but  see  the description of
>        fstatat() AT_NO_AUTOMOUNT fag, above).
>
> which is wrong: lstat and stat treat automounts the same.
> @Michael: do you recall why you inserted that text?  The commit message
> in commit 1ef5b2805471 ("stat.2: Cosmetic reworking of timestamp
> discussion in NOTES") is not very helpful.

That commit really was just cosmetic changes. The change that
introduced the text was 82d2be3d9d66b7, based on a note from Peter
Anvin:

[[
    > > Additionally, you may want to make a note in the stat/lstat man page tha
t on
    > > Linux, lstat(2) will generally not trigger automounter action, whereas
    > > stat(2) will.
    >
    > I don't understand this last piece.  Can you say some more.  (I'm not
    > familiar with automounter details.)

    An automounter (either an explicit one, like autofs, or an implicit
    one, such as are used by AFS or NFSv4) is something that triggers
    a mount when something is touched.

    However, it's undesirable to automount, say, everyone's home
    directory just because someone opened up /home in their GUI
    browser or typed "ls -l /home".  The early automounters simply
    didn't list the contents until you accessed it by name;
    this is still the case when you can't enumerate a mapping
    (say, all DNS names under /net).  However, this is extremely
    inconvenient, too.

    The solution we ended up settling on is to create something
    that looks like a directory (i.e. reports S_IFDIR in stat()),
    but behaves somewhat like a symlink.  In particular, when it is
    accessed in a way where a symlink would be dereferenced,
    the automount triggers and the directory is mounted.  However,
    system calls which do *not* cause a symlink to be dereferenced,
    like lstat(), also do not cause the automounter to trigger.
    This means that "ls -l", or a GUI file browser, can see a list
    of directories without causing each one of them to be automounted.

            -hpa
]]

Cheers,

Michael

> I propose correcting to
>
>   NOTES:
>       On Linux, lstat() nor stat() act as though AT_NO_AUTOMOUNT was set
>       and will not trigger automounter action for direct automount
>       points, though they may (prior to 4.14) for indirect automount
>       points.
>
>
> The more precise details, that automount action for indirect automount
> points is not triggered when the 'browse' option is used, is probably
> not necessary.
>
> Ian: if you agree with that text, and Michael doesn't provide alternate
> evidence, I'll submit a formal patch for the man page.... or should we
> just wait until the patch actually lands?
>
> Thanks,
> NeilBrown
>
Ian Kent Aug. 25, 2017, 12:05 a.m. UTC | #9
On 24/08/17 19:03, Michael Kerrisk (man-pages) wrote:
> Hi Neil,
> 
> On 24 August 2017 at 06:07, NeilBrown <neilb@suse.com> wrote:
>> On Wed, Aug 23 2017, Ian Kent wrote:
>>
>>>
>>> That inconsistency has bothered me for quite a while now.
>>>
>>> It was carried over from the autofs module behavior when automounting
>>> support was added to the VFS. What's worse is it prevents the use of
>>> the AT_NO_AUTOMOUNT flag from working properly with fstatat(2) and with
>>> statx().
>>>
>>> There is some risk in changing that so it does work but it really does
>>> need to work to enable userspace to not trigger an automount by using
>>> this flag.
>>>
>>> So that's (hopefully) going to change soonish, see:
>>> http://ozlabs.org/~akpm/mmotm/broken-out/autofs-fix-at_no_automount-not-being-honored.patch
>>>
>>> The result should be that stat family calls don't trigger automounts except
>>> for fstatat(2) and statx() which will require the AT_NO_AUTOMOUNT flag.
>>>
>>
>> oooh, yes.  That's much better - thanks.
>>
>> We should make sure that change gets into the man pages...
>>
>> First however, we should probably correct the man page!
>> stat.2 says:
>>
>>
>>   NOTES
>>        On Linux, lstat() will generally not trigger  automounter
>>        action,  whereas  stat()  will  (but  see  the description of
>>        fstatat() AT_NO_AUTOMOUNT fag, above).
>>
>> which is wrong: lstat and stat treat automounts the same.
>> @Michael: do you recall why you inserted that text?  The commit message
>> in commit 1ef5b2805471 ("stat.2: Cosmetic reworking of timestamp
>> discussion in NOTES") is not very helpful.
> 
> That commit really was just cosmetic changes. The change that
> introduced the text was 82d2be3d9d66b7, based on a note from Peter
> Anvin:

Indeed, that was correct for autofs v3 but we're at autofs v5 now and
a lot has changed over time (the commit is from 2008).

All I can do is apologize for not also checking the man pages and trying
to keep them up to date.

Let's just work on making them accurate now.

> 
> [[
>     > > Additionally, you may want to make a note in the stat/lstat man page tha
> t on
>     > > Linux, lstat(2) will generally not trigger automounter action, whereas
>     > > stat(2) will.
>     >
>     > I don't understand this last piece.  Can you say some more.  (I'm not
>     > familiar with automounter details.)
> 
>     An automounter (either an explicit one, like autofs, or an implicit
>     one, such as are used by AFS or NFSv4) is something that triggers
>     a mount when something is touched.
> 
>     However, it's undesirable to automount, say, everyone's home
>     directory just because someone opened up /home in their GUI
>     browser or typed "ls -l /home".  The early automounters simply
>     didn't list the contents until you accessed it by name;
>     this is still the case when you can't enumerate a mapping
>     (say, all DNS names under /net).  However, this is extremely
>     inconvenient, too.
> 
>     The solution we ended up settling on is to create something
>     that looks like a directory (i.e. reports S_IFDIR in stat()),
>     but behaves somewhat like a symlink.  In particular, when it is
>     accessed in a way where a symlink would be dereferenced,
>     the automount triggers and the directory is mounted.  However,
>     system calls which do *not* cause a symlink to be dereferenced,
>     like lstat(), also do not cause the automounter to trigger.
>     This means that "ls -l", or a GUI file browser, can see a list
>     of directories without causing each one of them to be automounted.
> 
>             -hpa
> ]]
> 
> Cheers,
> 
> Michael
> 
>> I propose correcting to
>>
>>   NOTES:
>>       On Linux, lstat() nor stat() act as though AT_NO_AUTOMOUNT was set
>>       and will not trigger automounter action for direct automount
>>       points, though they may (prior to 4.14) for indirect automount
>>       points.
>>
>>
>> The more precise details, that automount action for indirect automount
>> points is not triggered when the 'browse' option is used, is probably
>> not necessary.
>>
>> Ian: if you agree with that text, and Michael doesn't provide alternate
>> evidence, I'll submit a formal patch for the man page.... or should we
>> just wait until the patch actually lands?
>>
>> Thanks,
>> NeilBrown
>>
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells Sept. 8, 2017, 3:15 p.m. UTC | #10
Ian Kent <ikent@redhat.com> wrote:

> So far only David commented about using ENOENT rather than EREMOTE.
> 
> I prefer ENOENT for this case myself and he didn't object when I
> explained why, David, any concerns?

Not really - it just seems EREMOTE is a better fit since there is something
there, we're just not allowed to follow it.  This is different to a dangling
symlink IMO.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index beef981aa54f..7663ea82e68d 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -135,10 +135,13 @@  static inline struct autofs_info *autofs4_dentry_ino(struct dentry *dentry)
 /* autofs4_oz_mode(): do we see the man behind the curtain?  (The
  * processes which do manipulations for us in user space sees the raw
  * filesystem without "magic".)
+ * A process performing certain ioctls can get temporary oz status.
  */
+extern struct task_struct *autofs_tmp_oz;
 static inline int autofs4_oz_mode(struct autofs_sb_info *sbi)
 {
-	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp;
+	return sbi->catatonic || task_pgrp(current) == sbi->oz_pgrp ||
+		autofs_tmp_oz == current;
 }
 
 struct inode *autofs4_get_inode(struct super_block *, umode_t);
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index dd9f1bebb5a3..d76401669a20 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -200,6 +200,20 @@  static int autofs_dev_ioctl_protosubver(struct file *fp,
 	return 0;
 }
 
+struct task_struct *autofs_tmp_oz;
+int kern_path_oz(const char *pathname, int flags, struct path *path)
+{
+	static DEFINE_MUTEX(autofs_oz);
+	int err;
+
+	mutex_lock(&autofs_oz);
+	autofs_tmp_oz = current;
+	err = kern_path(pathname, flags, path);
+	autofs_tmp_oz = NULL;
+	mutex_unlock(&autofs_oz);
+	return err;
+}
+
 /* Find the topmost mount satisfying test() */
 static int find_autofs_mount(const char *pathname,
 			     struct path *res,
@@ -209,7 +223,8 @@  static int find_autofs_mount(const char *pathname,
 	struct path path;
 	int err;
 
-	err = kern_path_mountpoint(AT_FDCWD, pathname, &path, 0);
+	err = kern_path_oz(pathname, 0, &path);
+
 	if (err)
 		return err;
 	err = -ENOENT;
@@ -552,8 +567,7 @@  static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 
 	if (!fp || param->ioctlfd == -1) {
 		if (autofs_type_any(type))
-			err = kern_path_mountpoint(AT_FDCWD,
-						   name, &path, LOOKUP_FOLLOW);
+			err = kern_path_oz(name, LOOKUP_FOLLOW, &path);
 		else
 			err = find_autofs_mount(name, &path,
 						test_by_type, &type);