Re: [PATCH] vfs: fix EBUSY on FSCONFIG_CMD_CREATE retry

[Jan Kara]

On Sat 13-12-25 02:03:56, Chen Linxuan via B4 Relay wrote:
> From: Chen Linxuan <me@xxxxxxxxxxxxx>
> 
> When using fsconfig(..., FSCONFIG_CMD_CREATE, ...), the filesystem
> context is retrieved from the file descriptor. Since the file structure
> persists across syscall restarts, the context state is preserved:
> 
> 	// fs/fsopen.c
> 	SYSCALL_DEFINE5(fsconfig, ...)
> 	{
> 		...
> 		fc = fd_file(f)->private_data;
> 		...
> 		ret = vfs_fsconfig_locked(fc, cmd, &param);
> 		...
> 	}
> 
> In vfs_cmd_create(), the context phase is transitioned to
> FS_CONTEXT_CREATING before calling vfs_get_tree():
> 
> 	// fs/fsopen.c
> 	static int vfs_cmd_create(struct fs_context *fc, bool exclusive)
> 	{
> 		...
> 		fc->phase = FS_CONTEXT_CREATING;
> 		...
> 		ret = vfs_get_tree(fc);
> 		...
> 	}
> 
> However, vfs_get_tree() may return -ERESTARTNOINTR if the filesystem
> implementation needs to restart the syscall. For example, cgroup v1 does
> this when it encounters a race condition where the root is dying:
> 
> 	// kernel/cgroup/cgroup-v1.c
> 	int cgroup1_get_tree(struct fs_context *fc)
> 	{
> 		...
> 		if (unlikely(ret > 0)) {
> 			msleep(10);
> 			return restart_syscall();
> 		}
> 		return ret;
> 	}
> 
> If the syscall is restarted, fsconfig() is called again and retrieves
> the *same* fs_context. However, vfs_cmd_create() rejects the call
> because the phase was left as FS_CONTEXT_CREATING during the first
> attempt:

Well, not quite. The phase is actually set to FS_CONTEXT_FAILED if
vfs_get_tree() returns any error. Still the effect is the same.

> 	if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
> 		return -EBUSY;
> 
> Fix this by resetting fc->phase back to FS_CONTEXT_CREATE_PARAMS if
> vfs_get_tree() returns -ERESTARTNOINTR.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Chen Linxuan <me@xxxxxxxxxxxxx>
> ---
>  fs/fsopen.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index f645c99204eb..8a7cb031af50 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -229,6 +229,10 @@ static int vfs_cmd_create(struct fs_context *fc, bool exclusive)
>  	fc->exclusive = exclusive;
>  
>  	ret = vfs_get_tree(fc);
> +	if (ret == -ERESTARTNOINTR) {
> +		fc->phase = FS_CONTEXT_CREATE_PARAMS;
> +		return ret;
> +	}
>  	if (ret) {
>  		fc->phase = FS_CONTEXT_FAILED;
>  		return ret;

Thanks for the patch. It looks good to me. I'd slightly prefer style like:

	if (ret) {
		if (ret == -ERESTARTNOINTR)
			fc->phase = FS_CONTEXT_CREATE_PARAMS;
		else
			fc->phase = FS_CONTEXT_FAILED;
		return ret;
	}

but I can live with what you propose as well. So feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR