Re: [PATCH] btrfs: add io_uring interface for encoded reads

[Pavel Begunkov <asml.silence@gmail.com>]

On 8/13/24 01:49, David Sterba wrote:
> On Mon, Aug 12, 2024 at 08:17:43PM +0100, Pavel Begunkov wrote:
>> On 8/12/24 17:58, David Sterba wrote:
>>> On Mon, Aug 12, 2024 at 05:10:15PM +0100, Pavel Begunkov wrote:
>>>> And the last point, I'm surprised there are two versions of
>>>> btrfs_ioctl_encoded_io_args. Maybe, it's a good moment to fix it if
>>>> we're creating a new interface.
>>>>
>>>> E.g. by adding a new structure defined right with u64 and such, use it
>>>> in io_uring, and cast to it in the ioctl code when it's x64 (with
>>>> a good set of BUILD_BUG_ON sprinkled) and convert structures otherwise?
>>>
>>> If you mean the 32bit version of the ioctl struct
>>> (btrfs_ioctl_encoded_io_args_32), I don't think we can fix it. It's been
>>
>> Right, I meant btrfs_ioctl_encoded_io_args_32. And to clarify, nothing
>> can be done for the ioctl(2) part, I only suggested to have a single
>> structure when it comes to io_uring.
>>
>>> there from the beginning and it's not a mistake. I don't remember the
>>> details why and only vaguely remember that I'd asked why we need it.
>>> Similar 64/32 struct is in the send ioctl but that was a mistake due to
>>> a pointer being passed in the structure and that needs to be handled due
>>> to different type width.
>>
>> Would be interesting to learn why, maybe Omar remembers? Only two
>> fields are not explicitly sized, both could've been just u64.
>> The structure iov points to (struct iovec) would've had a compat
>> flavour, but that doesn't require a separate
>> btrfs_ioctl_encoded_io_args.
> 
> Found it:
> 
> "why don't we avoid the send 32bit workaround"
> https://lore.kernel.org/linux-btrfs/20190828120650.GZ2752@twin.jikos.cz/
> 
> "because big-endian"
> https://lore.kernel.org/linux-btrfs/20190903171458.GA7452@vader/

union {
	void __user *buf;
	__u64 __buf_alignment;
};

Endianness is indeed a problem here, but I don't see the
purpose of aliasing it with a pointer instead of keeping
just u64, which is a common pattern.

struct btrfs_ioctl_encoded_io_args {
	__u64 buf;
	...
};

// user
void *buf = ...;
arg.buf = (__u64)(uintptr_t)buf;

// kernel
void __user *p = u64_to_user_ptr(arg.buf);

-- 
Pavel Begunkov