Re: [PATCH] vfs: document locking for mnt_notify_add()

[Jan Kara]

On Fri 29-05-26 21:38:08, Jann Horn wrote:
> The locking in mnt_notify_add(), which was introduced in commit
> bf630c401641 ("vfs: add notifications for mount attach and detach"), is a
> bit gnarly.
> notify_list is protected by namespace_lock, but there are cases where
> mnt_notify_add() is called without holding namespace_lock, for example:
> 
>     __do_sys_fsmount -> mnt_add_to_ns -> mnt_notify_add
> 
> Luckily, in cases where the namespace_lock isn't held, the namespace is
> always freshly created and can't have any fsnotify marks yet, which means
> the notify_list isn't actually accessed.
> 
> The existing comment claims that not accessing the notify_list in these
> cases is merely an optimization, which is wrong. Fix the comment, and add a
> locking assertion.
> 
> To allow mnt_notify_add() to reference the namespace_sem, move it into
> fs/namespace.c.
> 
> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>

Thanks for looking into this. Adding Miklos to CC who actually wrote that
code. Also in cases like path_pivot_root() I don't think we hold the
namespace_sem so the locking is even more complicated if it is correct at
all. Miklos?

								Honza

> ---
> I'm sending this patch because I spent some time staring at this
> trying to figure out if this was buggy or not.
> 
> I don't know if this is working as intended or working by accident,
> and it might be nice if this was cleaned up to have simpler locking;
> but for now, document what's going on for the next person who stares
> at this code.
> ---
>  fs/mount.h     | 10 +---------
>  fs/namespace.c | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/mount.h b/fs/mount.h
> index e0816c11a198..99016db2f408 100644
> --- a/fs/mount.h
> +++ b/fs/mount.h
> @@ -219,15 +219,7 @@ static inline struct mnt_namespace *to_mnt_ns(struct ns_common *ns)
>  }
>  
>  #ifdef CONFIG_FSNOTIFY
> -static inline void mnt_notify_add(struct mount *m)
> -{
> -	/* Optimize the case where there are no watches */
> -	if ((m->mnt_ns && m->mnt_ns->n_fsnotify_marks) ||
> -	    (m->prev_ns && m->prev_ns->n_fsnotify_marks))
> -		list_add_tail(&m->to_notify, &notify_list);
> -	else
> -		m->prev_ns = m->mnt_ns;
> -}
> +void mnt_notify_add(struct mount *m);
>  #else
>  static inline void mnt_notify_add(struct mount *m)
>  {
> diff --git a/fs/namespace.c b/fs/namespace.c
> index fe919abd2f01..56f130b49e58 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -6431,6 +6431,25 @@ bool mnt_may_suid(struct vfsmount *mnt)
>  	       current_in_userns(mnt->mnt_sb->s_user_ns);
>  }
>  
> +#ifdef CONFIG_FSNOTIFY
> +void mnt_notify_add(struct mount *m)
> +{
> +	/*
> +	 * notify_list is protected by namespace_sem.
> +	 * It is possible to call this function without holding namespace_sem,
> +	 * but in those cases, the mount is associated with a new mount
> +	 * namespace that can't have any fanotify marks yet.
> +	 */
> +	if ((m->mnt_ns && m->mnt_ns->n_fsnotify_marks) ||
> +	    (m->prev_ns && m->prev_ns->n_fsnotify_marks)) {
> +		rwsem_assert_held_write(&namespace_sem);
> +		list_add_tail(&m->to_notify, &notify_list);
> +	} else {
> +		m->prev_ns = m->mnt_ns;
> +	}
> +}
> +#endif
> +
>  static struct ns_common *mntns_get(struct task_struct *task)
>  {
>  	struct ns_common *ns = NULL;
> 
> ---
> base-commit: 8fde5d1d47f69db6082dfa34500c27f8485389a5
> change-id: 20260529-vfs-mnt-notify-comment-4369e0558f0b
> 
> Best regards,
> --  
> Jann Horn <jannh@xxxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR