linux-kernel - Re: [PATCH v2 2/7] overlayfs: Convert to super_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZcKpSU9frvTUb2eq@dread.disaster.area>
Date: Wed, 7 Feb 2024 08:48:57 +1100
From: Dave Chinner <david@...morbit.com>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: brauner@...nel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, Miklos Szeredi <miklos@...redi.hu>,
	Amir Goldstein <amir73il@...il.com>, linux-unionfs@...r.kernel.org
Subject: Re: [PATCH v2 2/7] overlayfs: Convert to super_set_uuid()

On Tue, Feb 06, 2024 at 03:18:50PM -0500, Kent Overstreet wrote:
> We don't want to be settingc sb->s_uuid directly anymore, as there's a
> length field that also has to be set, and this conversion was not
> completely trivial.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@...ux.dev>
> Cc: Miklos Szeredi <miklos@...redi.hu>
> Cc: Amir Goldstein <amir73il@...il.com>
> Cc: linux-unionfs@...r.kernel.org
> ---
>  fs/overlayfs/util.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 0217094c23ea..f1f0ee9a9dff 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -760,13 +760,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
>  			 const struct path *upperpath)
>  {
>  	bool set = false;
> +	uuid_t uuid;
>  	int res;
>  
>  	/* Try to load existing persistent uuid */
> -	res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, sb->s_uuid.b,
> +	res = ovl_path_getxattr(ofs, upperpath, OVL_XATTR_UUID, uuid.b,
>  				UUID_SIZE);
>  	if (res == UUID_SIZE)
> -		return true;
> +		goto success;
>  
>  	if (res != -ENODATA)
>  		goto fail;
> @@ -794,14 +795,14 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
>  	}
>  
>  	/* Generate overlay instance uuid */
> -	uuid_gen(&sb->s_uuid);
> +	uuid_gen(&uuid);
>  
>  	/* Try to store persistent uuid */
>  	set = true;
> -	res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, sb->s_uuid.b,
> +	res = ovl_setxattr(ofs, upperpath->dentry, OVL_XATTR_UUID, uuid.b,
>  			   UUID_SIZE);
>  	if (res == 0)
> -		return true;
> +		goto success;

This is a bit weird. Normally the success case is in line, and we
jump out of line for the fail case. I think this is more better:

	if (res)
		goto fail;
success:
	super_set_uuid(sb, uuid.b, sizeof(uuid));
	return true;
>  
>  fail:
>  	memset(sb->s_uuid.b, 0, UUID_SIZE);

And then the fail case follows naturally.

-Dave.

-- 
Dave Chinner
david@...morbit.com