Re: [PATCH] btrfs: add 32-bit compat ioctl for BTRFS_IOC_GET_SUBVOL_INFO

["Miquel Sabaté Solà" <mssola@mssola.com>]

[-- Attachment #1: Type: text/plain, Size: 6685 bytes --]

Hi,

Just a small comment from my side, but feel free to ignore it :)

Daan De Meyer @ 2026-05-21 07:51 GMT:

> On 64-bit kernels with 32-bit userspace, struct btrfs_ioctl_timespec is
> laid out as 16 bytes (8B sec + 4B nsec + 4B trailing padding) instead of
> the 12 bytes a 32-bit userspace expects, because the surrounding struct
> is not packed. As a result, struct btrfs_ioctl_get_subvol_info_args has
> a different size and layout in 32-bit userspace than in the 64-bit
> kernel, and BTRFS_IOC_GET_SUBVOL_INFO returns garbage to 32-bit callers.
>
> Mirror what was done for BTRFS_IOC_SET_RECEIVED_SUBVOL: add a packed
> btrfs_ioctl_get_subvol_info_args_32 with btrfs_ioctl_timespec_32 fields,
> define BTRFS_IOC_GET_SUBVOL_INFO_32 with that struct as the size
> argument, factor the existing handler into a shared _btrfs_ioctl_get_
> subvol_info() helper, and add btrfs_ioctl_get_subvol_info_32() which
> fills the kernel struct and translates field-by-field into the 32-bit
> struct before copy_to_user().
>
> Signed-off-by: Daan De Meyer <daan@amutable.com>
> ---
>  fs/btrfs/ioctl.c | 112 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 99 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a39460bf68a7..31be2590f1b2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -82,6 +82,30 @@ struct btrfs_ioctl_received_subvol_args_32 {
>
>  #define BTRFS_IOC_SET_RECEIVED_SUBVOL_32 _IOWR(BTRFS_IOCTL_MAGIC, 37, \
>  				struct btrfs_ioctl_received_subvol_args_32)
> +
> +struct btrfs_ioctl_get_subvol_info_args_32 {
> +	__u64 treeid;
> +	char name[BTRFS_VOL_NAME_MAX + 1];
> +	__u64 parent_id;
> +	__u64 dirid;
> +	__u64 generation;
> +	__u64 flags;
> +	__u8 uuid[BTRFS_UUID_SIZE];
> +	__u8 parent_uuid[BTRFS_UUID_SIZE];
> +	__u8 received_uuid[BTRFS_UUID_SIZE];
> +	__u64 ctransid;
> +	__u64 otransid;
> +	__u64 stransid;
> +	__u64 rtransid;
> +	struct btrfs_ioctl_timespec_32 ctime;
> +	struct btrfs_ioctl_timespec_32 otime;
> +	struct btrfs_ioctl_timespec_32 stime;
> +	struct btrfs_ioctl_timespec_32 rtime;
> +	__u64 reserved[8];
> +} __attribute__ ((__packed__));
> +
> +#define BTRFS_IOC_GET_SUBVOL_INFO_32 _IOR(BTRFS_IOCTL_MAGIC, 60, \
> +				struct btrfs_ioctl_get_subvol_info_args_32)
>  #endif
>
>  #if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT)
> @@ -1945,9 +1969,9 @@ static int btrfs_ioctl_ino_lookup_user(struct file *file, void __user *argp)
>  }
>
>  /* Get the subvolume information in BTRFS_ROOT_ITEM and BTRFS_ROOT_BACKREF */
> -static int btrfs_ioctl_get_subvol_info(struct inode *inode, void __user *argp)
> +static int _btrfs_ioctl_get_subvol_info(struct inode *inode,
> +					struct btrfs_ioctl_get_subvol_info_args *subvol_info)
>  {
> -	struct btrfs_ioctl_get_subvol_info_args *subvol_info;
>  	struct btrfs_fs_info *fs_info;
>  	struct btrfs_root *root;
>  	struct btrfs_path *path;
> @@ -1964,12 +1988,6 @@ static int btrfs_ioctl_get_subvol_info(struct inode *inode, void __user *argp)
>  	if (!path)
>  		return -ENOMEM;
>
> -	subvol_info = kzalloc_obj(*subvol_info);
> -	if (!subvol_info) {
> -		btrfs_free_path(path);
> -		return -ENOMEM;
> -	}
> -
>  	fs_info = BTRFS_I(inode)->root->fs_info;
>
>  	/* Get root_item of inode's subvolume */
> @@ -2048,15 +2066,79 @@ static int btrfs_ioctl_get_subvol_info(struct inode *inode, void __user *argp)
>  		}
>  	}
>
> -	btrfs_free_path(path);
> -	path = NULL;
> -	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
> -		ret = -EFAULT;
> -
>  out:
>  	btrfs_put_root(root);
>  out_free:
>  	btrfs_free_path(path);
> +	return ret;
> +}
> +
> +#ifdef CONFIG_64BIT
> +static int btrfs_ioctl_get_subvol_info_32(struct inode *inode, void __user *argp)
> +{
> +	struct btrfs_ioctl_get_subvol_info_args *subvol_info = NULL;
> +	struct btrfs_ioctl_get_subvol_info_args_32 *subvol_info_32 = NULL;

Maybe these two variables could be declared with the AUTO_KFREE helper,
so you could remove goto's and the cleanup section in the end.

> +	int ret;
> +
> +	subvol_info = kzalloc_obj(*subvol_info);
> +	if (!subvol_info)
> +		return -ENOMEM;
> +
> +	subvol_info_32 = kzalloc_obj(*subvol_info_32);
> +	if (!subvol_info_32) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = _btrfs_ioctl_get_subvol_info(inode, subvol_info);
> +	if (ret)
> +		goto out;
> +
> +	subvol_info_32->treeid = subvol_info->treeid;
> +	memcpy(subvol_info_32->name, subvol_info->name, sizeof(subvol_info_32->name));
> +	subvol_info_32->parent_id = subvol_info->parent_id;
> +	subvol_info_32->dirid = subvol_info->dirid;
> +	subvol_info_32->generation = subvol_info->generation;
> +	subvol_info_32->flags = subvol_info->flags;
> +	memcpy(subvol_info_32->uuid, subvol_info->uuid, BTRFS_UUID_SIZE);
> +	memcpy(subvol_info_32->parent_uuid, subvol_info->parent_uuid, BTRFS_UUID_SIZE);
> +	memcpy(subvol_info_32->received_uuid, subvol_info->received_uuid, BTRFS_UUID_SIZE);
> +	subvol_info_32->ctransid = subvol_info->ctransid;
> +	subvol_info_32->otransid = subvol_info->otransid;
> +	subvol_info_32->stransid = subvol_info->stransid;
> +	subvol_info_32->rtransid = subvol_info->rtransid;
> +	subvol_info_32->ctime.sec = subvol_info->ctime.sec;
> +	subvol_info_32->ctime.nsec = subvol_info->ctime.nsec;
> +	subvol_info_32->otime.sec = subvol_info->otime.sec;
> +	subvol_info_32->otime.nsec = subvol_info->otime.nsec;
> +	subvol_info_32->stime.sec = subvol_info->stime.sec;
> +	subvol_info_32->stime.nsec = subvol_info->stime.nsec;
> +	subvol_info_32->rtime.sec = subvol_info->rtime.sec;
> +	subvol_info_32->rtime.nsec = subvol_info->rtime.nsec;
> +
> +	if (copy_to_user(argp, subvol_info_32, sizeof(*subvol_info_32)))
> +		ret = -EFAULT;
> +
> +out:
> +	kfree(subvol_info_32);
> +	kfree(subvol_info);
> +	return ret;
> +}
> +#endif
> +
> +static int btrfs_ioctl_get_subvol_info(struct inode *inode, void __user *argp)
> +{
> +	struct btrfs_ioctl_get_subvol_info_args *subvol_info;
> +	int ret;
> +
> +	subvol_info = kzalloc_obj(*subvol_info);
> +	if (!subvol_info)
> +		return -ENOMEM;
> +
> +	ret = _btrfs_ioctl_get_subvol_info(inode, subvol_info);
> +	if (!ret && copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
> +		ret = -EFAULT;
> +
>  	kfree(subvol_info);
>  	return ret;
>  }
> @@ -5273,6 +5355,10 @@ long btrfs_ioctl(struct file *file, unsigned int
>  		return btrfs_ioctl_set_features(file, argp);
>  	case BTRFS_IOC_GET_SUBVOL_INFO:
>  		return btrfs_ioctl_get_subvol_info(inode, argp);
> +#ifdef CONFIG_64BIT
> +	case BTRFS_IOC_GET_SUBVOL_INFO_32:
> +		return btrfs_ioctl_get_subvol_info_32(inode, argp);
> +#endif
>  	case BTRFS_IOC_GET_SUBVOL_ROOTREF:
>  		return btrfs_ioctl_get_subvol_rootref(root, argp);
>  	case BTRFS_IOC_INO_LOOKUP_USER:

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]