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: 7256 bytes --]

Hi,

Just realized I'm commenting on an old thread and that the AUTO_KFREE
suggestion was already applied by David.

Sorry for the noise.

Miquel Sabaté Solà @ 2026-06-17 13:18 +02:

> 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 --]