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
Description: PGP signature
