Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 15 Aug 2022 16:56:23 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     Jeremy Bongio <bongiojp@gmail.com>
Cc:     linux-ext4@vger.kernel.org
Subject: Re: [PATCH v3] tune2fs: Add support for get/set UUID ioctls.
Message-ID: <Yvqy93lxTBWGeuxw@mit.edu>
References: <20220719235204.237526-1-bongiojp@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220719235204.237526-1-bongiojp@gmail.com>
Precedence: bulk

On Tue, Jul 19, 2022 at 04:52:04PM -0700, Jeremy Bongio wrote:
> +/*
> + * Use EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID to get/set file system UUID.
> + * Return:	0 on success
> + *             -1 when the old method should be used
> + */
> +int handle_fsuuid(__u8 *uuid, bool get)
> +{
> +	errcode_t ret;
> +	int mnt_flags, fd;
> +	char label[FSLABEL_MAX];
> +	int maxlen = FSLABEL_MAX - 1;
> +	char mntpt[PATH_MAX + 1];
> +	struct fsuuid *fsuuid = NULL;
> +
> +	fsuuid = malloc(sizeof(*fsuuid) + UUID_SIZE);
> +	if (!fsuuid)
> +		return -1;

fsuuid is not getting freed in this function, so this will leak
memory.

	...
> +	if (get)
> +		ret = ioctl(fd, EXT4_IOC_GETFSUUID, fsuuid);
> +	else
> +		ret = ioctl(fd, EXT4_IOC_SETFSUUID, fsuuid);

In the EXT4_IOC_GETFSUUID case, you need to copy fsuuid->fsu_uuid to
uuid, so it is returned to the caller.


> +	ret = ext2fs_check_mount_point(device_name, &mnt_flags,
> +					  mntpt, sizeof(mntpt));
> +	if (ret || !(mnt_flags & EXT2_MF_MOUNTED) ||
> +		(!get && (mnt_flags & EXT2_MF_READONLY)) ||
> +		!mntpt[0])
> +		return -1;

handle_fsuuid() is getting called twice by tune2fs when handling the
-U option, which means we're calling ext2fs_check_mount_point() twice.

And around line 3364, tune2fs calls ext2fs_check_if_mounted() which
fetches the mnt_flags but which doesn't get the mountpoint.

So I wonder if wouldn't be better off changing tune2fs's main() to
call ext2fs_check_mount_point() instead of ext2fs_check_if_mounted(),
and we can just determine the mountpoint once.

Then, instead of calling handle_fsuuid/[gs]et_mounted_fsuuid, in the
handling of -U, we can do something like this:

	if (U_flag) {
		int fd = -1;
		struct fsuuid *fsuuid = NULL;
		...

		if ((mnt_flags & EXT2_MF_MOUNTED) &&
		    !(mnt_flags & EXT2_MF_READONLY) && mntpt) {
			fd = open(mntpt, O_RDONLY);
			if (fd >= 0) {
				fsuuid = malloc(sizeof(*fsuuid) + UUID_SIZE);
				if (!fsuuid) {
					close(fd);
					fd = -1;
				}
			}
		}		
				

In other words, we can just inline all of handle_fsuuid, and the call
to get_mounted_fsuuid() just becomes:

	if (fd >= 0) {
		fsuuid->fsu_len - UUID_SIZE;
		fsuuid->fsu_flags = 0;
		ret = ioctl(fd, EXT4_IOC_GETFSUUID, fsuuid);
	}

... and similarly for set_mounted_fsuuid().


Then at the end of tune2fs -U processing, we can do something like this:

	if (fd >= 0)
		close(fd);
	free(fsuuid);

						- Ted