Re: [PATCH v2] erofs: accept source file descriptor via fsconfig

Aleksa Sarai <[email protected]> Mon, 13 Jul 2026 14:52:25 +1000
Newsgroups	org.ozlabs.lists.linux-erofs,org.kernel.vger.linux-fsdevel
Message-ID	<[email protected]>
On 2026-07-11, Giuseppe Scrivano <[email protected]> wrote:
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 86fa5c6a0c70..3040d4cf9b85 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -386,6 +386,7 @@ static void erofs_default_options(struct erofs_sb_info *sbi)
>  enum {
>  	Opt_user_xattr, Opt_acl, Opt_cache_strategy, Opt_dax, Opt_dax_enum,
>  	Opt_device, Opt_domain_id, Opt_directio, Opt_fsoffset, Opt_inode_share,
> +	Opt_source_fd,
>  };
>  
>  static const struct constant_table erofs_param_cache_strategy[] = {
> @@ -413,6 +414,7 @@ static const struct fs_parameter_spec erofs_fs_parameters[] = {
>  	fsparam_flag_no("directio",	Opt_directio),
>  	fsparam_u64("fsoffset",		Opt_fsoffset),
>  	fsparam_flag("inode_share",	Opt_inode_share),
> +	fsparam_fd("source",		Opt_source_fd),
>  	{}
>  };
>  
> @@ -524,6 +526,11 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>  		else
>  			set_opt(&sbi->opt, INODE_SHARE);
>  		break;
> +	case Opt_source_fd:
> +		if (sbi->dif0.file)
> +			return -EINVAL;
> +		sbi->dif0.file = get_file(param->file);
> +		break;

I don't think this handling is right for a few reasons:

 1. AFAICS this shadows the default "source" handling logic (because
    -ENOPARAM is not returned for the non-fd case), which means that
    this regresses existing erofs users -- everyone already uses
    "source" today. I must really be missing something if this worked
    when you tested it.

    Additionally, fsparam_fd unfortunately permits strings (where the
    string is the numerical value of the fd number), meaning that this
    will call get_file(<garbage>) if someone uses FSCONFIG_SET_STRING.
    You will need to check param->type at least to avoid that.

    I meant to send a patch for this earlier this year, but a nicer
    solution would be to have a custom helper similar to fs_lookup_param
    except that it permits FSCONFIG_SET_FD, FSCONFIG_SET_PATH,
    FSCONFIG_SET_PATH_EMPTY, and FSCONFIG_SET_STRING. This is sorely
    missing and people keep accidentally creating unusable interfaces as
    a result. I mentioned this in an LPC talk last year[1].

    proc_parse_pidns_param was my minimal version that only accepts
    FSCONFIG_SET_FD and FSCONFIG_SET_STRING, and if you don't want to
    add dirfd support yet then you should use something more like that.

 2. On a slightly less critical note, fc->source has special handling in
    the VFS in a few places and AFAICS this is the first example of
    someone adding an implementation of "source" that does not set
    fc->source to a proper value, which deserves some additional review.

    (At at quick glance it seems this just means that some stuff in
    procfs will show as "none" rather than fc->source debugging, but
    again it probably needs a closer look.)

[1]: https://youtu.be/NX5IzF6JXp0?t=72

-- 
Aleksa Sarai
Founding Engineer at Amutable
https://www.cyphar.com/
signature.asc (application/pgp-signature, 265 B)