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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
