Re: [PATCH 07/14] Change unshare_fs_struct() to never fail.

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-07-15 at 17:14 +1000, NeilBrown wrote:
> nfsd threads need to not share the init fs_struct as they need to
> manipulate umask independently.  So they call unshare_fs_struct() and
> are the only user of that function.
> 
> In the unlikely event that unshare_fs_struct() fails, the thread will
> exit calling svc_exit_thread() BEFORE svc_thread_should_stop() reports
> 'true'.
> 
> This is a problem because svc_exit_thread() assumes that
> svc_stop_threads() is running and consequently (in the nfsd case)
> nfsd_mutex is held.  This ensures that the list_del_rcu() call in
> svc_exit_thread() cannot race with any other manipulation of
> ->sp_all_threads.
> 
> While it would be possible to add some other exclusion, doing so would
> introduce unnecessary complexity.  unshare_fs_struct() does not fail in
> practice.  So the simplest solution is to make this explicit.  i.e.  use
> __GFP_NOFAIL which is safe on such a small allocation - about 64 bytes.
> 

I know some folks are trying hard to get rid of (or minimize the use
of) __GFP_NOFAIL. This might not be a long term solution.

> Change unshare_fs_struct() to not return any error, and remove the error
> handling from nfsd().
> 
> An alternate approach would be to create a variant of
> kthread_create_on_node() which didn't set CLONE_FS.
> 

This sounds like it might be the better approach. I guess you could
just add a set of CLONE_* flags to struct kthread_create_info and fix
up the callers to set that appropriately?

> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/fs_struct.c            | 42 ++++++++++++++++++++-------------------
>  fs/nfsd/nfssvc.c          |  9 +++------
>  include/linux/fs_struct.h |  2 +-
>  3 files changed, 26 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/fs_struct.c b/fs/fs_struct.c
> index 64c2d0814ed6..49fba862e408 100644
> --- a/fs/fs_struct.c
> +++ b/fs/fs_struct.c
> @@ -109,35 +109,39 @@ void exit_fs(struct task_struct *tsk)
>  	}
>  }
>  
> +static void init_fs_struct(struct fs_struct *fs, struct fs_struct *old)
> +{
> +	fs->users = 1;
> +	fs->in_exec = 0;
> +	spin_lock_init(&fs->lock);
> +	seqcount_spinlock_init(&fs->seq, &fs->lock);
> +	fs->umask = old->umask;
> +
> +	spin_lock(&old->lock);
> +	fs->root = old->root;
> +	path_get(&fs->root);
> +	fs->pwd = old->pwd;
> +	path_get(&fs->pwd);
> +	spin_unlock(&old->lock);
> +}
> +
>  struct fs_struct *copy_fs_struct(struct fs_struct *old)
>  {
>  	struct fs_struct *fs = kmem_cache_alloc(fs_cachep, GFP_KERNEL);
>  	/* We don't need to lock fs - think why ;-) */
> -	if (fs) {
> -		fs->users = 1;
> -		fs->in_exec = 0;
> -		spin_lock_init(&fs->lock);
> -		seqcount_spinlock_init(&fs->seq, &fs->lock);
> -		fs->umask = old->umask;
> -
> -		spin_lock(&old->lock);
> -		fs->root = old->root;
> -		path_get(&fs->root);
> -		fs->pwd = old->pwd;
> -		path_get(&fs->pwd);
> -		spin_unlock(&old->lock);
> -	}
> +	if (fs)
> +		init_fs_struct(fs, old);
>  	return fs;
>  }
>  
> -int unshare_fs_struct(void)
> +void unshare_fs_struct(void)
>  {
>  	struct fs_struct *fs = current->fs;
> -	struct fs_struct *new_fs = copy_fs_struct(fs);
> +	struct fs_struct *new_fs = kmem_cache_alloc(fs_cachep,
> +						    GFP_KERNEL| __GFP_NOFAIL);
>  	int kill;
>  
> -	if (!new_fs)
> -		return -ENOMEM;
> +	init_fs_struct(new_fs, fs);
>  
>  	task_lock(current);
>  	spin_lock(&fs->lock);
> @@ -148,8 +152,6 @@ int unshare_fs_struct(void)
>  
>  	if (kill)
>  		free_fs_struct(fs);
> -
> -	return 0;
>  }
>  EXPORT_SYMBOL_GPL(unshare_fs_struct);
>  
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 7377422a34df..f5de04a63c6f 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -873,11 +873,9 @@ nfsd(void *vrqstp)
>  
>  	/* At this point, the thread shares current->fs
>  	 * with the init process. We need to create files with the
> -	 * umask as defined by the client instead of init's umask. */
> -	if (unshare_fs_struct() < 0) {
> -		printk("Unable to start nfsd thread: out of memory\n");
> -		goto out;
> -	}
> +	 * umask as defined by the client instead of init's umask.
> +	 */
> +	unshare_fs_struct();
>  
>  	current->fs->umask = 0;
>  
> @@ -899,7 +897,6 @@ nfsd(void *vrqstp)
>  
>  	atomic_dec(&nfsd_th_cnt);
>  
> -out:
>  	/* Release the thread */
>  	svc_exit_thread(rqstp);
>  	return 0;
> diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
> index 783b48dedb72..8282e6c7ff29 100644
> --- a/include/linux/fs_struct.h
> +++ b/include/linux/fs_struct.h
> @@ -22,7 +22,7 @@ extern void set_fs_root(struct fs_struct *, const struct path *);
>  extern void set_fs_pwd(struct fs_struct *, const struct path *);
>  extern struct fs_struct *copy_fs_struct(struct fs_struct *);
>  extern void free_fs_struct(struct fs_struct *);
> -extern int unshare_fs_struct(void);
> +extern void unshare_fs_struct(void);
>  
>  static inline void get_fs_root(struct fs_struct *fs, struct path *root)
>  {

-- 
Jeff Layton <jlayton@kernel.org>