Re: [PATCH] udf: convert to new mount API

[Eric Sandeen <sandeen@redhat.com>]

On 2/9/24 1:43 PM, Eric Sandeen wrote:
> Convert the UDF filesystem to the new mount API.
> 
> UDF is slightly unique in that it always preserves prior mount
> options across a remount, so that's handled by udf_init_options().
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Tested by running through xfstests, as well as some targeted testing of
> remount behavior.
> 
> NB: I did not convert i.e any udf_err() to errorf(fc, ) because the former
> does some nice formatting, not sure how or if you'd like to handle that, Jan?
> 
>  fs/udf/super.c | 495 +++++++++++++++++++++++++------------------------
>  1 file changed, 255 insertions(+), 240 deletions(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 928a04d9d9e0..03fa98fe4e1c 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c

...

> +	case Opt_gid:
> +		if (0 == kstrtoint(param->string, 10, &uv)) {
>  			uopt->gid = make_kgid(current_user_ns(), uv);
>  			if (!gid_valid(uopt->gid))
> -				return 0;
> +				return -EINVAL;
>  			uopt->flags |= (1 <<  );
> -			break;
> -		case Opt_uid:
> -			if (match_uint(args, &uv))
> -				return 0;
> +		} else if (!strcmp(param->string, "forget")) {
> +			uopt->flags |= (1 << UDF_FLAG_GID_FORGET);
> +		} else if (!strcmp(param->string, "ignore")) {
> +			/* this option is superseded by gid=<number> */
> +			;
> +		} else {
> +			return -EINVAL;
> +		}
> +		break;

I wonder if I need to redo this and not directly set the make_kgid option
into uopt->gid. We do test that uopt->gid is valid, and return an error, and
skip setting UDF_FLAG_GID_SET, but ...

...

> -static int udf_fill_super(struct super_block *sb, void *options, int silent)
> +static int udf_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	int ret = -EINVAL;
>  	struct inode *inode = NULL;
> -	struct udf_options uopt;
> +	struct udf_options *uopt = fc->fs_private;
>  	struct kernel_lb_addr rootdir, fileset;
>  	struct udf_sb_info *sbi;
>  	bool lvid_open = false;
> -
> -	uopt.flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
> -	/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */
> -	uopt.uid = make_kuid(current_user_ns(), overflowuid);
> -	uopt.gid = make_kgid(current_user_ns(), overflowgid);

this initialization (now moved to udf_init_options) gets overwritten
even if the [gu]id was invalid during parsing ...

> +	sbi->s_flags = uopt->flags;
> +	sbi->s_uid = uopt->uid;
> +	sbi->s_gid = uopt->gid;

... and gets set into sbi here.

In the past (I think) the whole mount would fail with an invalid UID/GID but w/
fsconfig, we could just fail that one config and continue with the rest.

It looks like sbi->s_[gu]id is not accessed unless UDF_FLAG_[GU]ID_SET is
set, but maybe it's best to never set something invalid into the uopt.

-Eric