Re: [RFC PATCH v4 4/5] btrfs: ioctl: introduce btrfs_uring_import_iovec()

[Jens Axboe <axboe@kernel.dk>]

On 3/18/25 1:55 AM, Sidong Yang wrote:
> On Tue, Mar 18, 2025 at 07:25:51AM +0000, Pavel Begunkov wrote:
>> On 3/18/25 01:58, Sidong Yang wrote:
>>> On Mon, Mar 17, 2025 at 09:40:05AM -0600, Jens Axboe wrote:
>>>> On 3/17/25 7:57 AM, Sidong Yang wrote:
>>>>> This patch introduces btrfs_uring_import_iovec(). In encoded read/write
>>>>> with uring cmd, it uses import_iovec without supporting fixed buffer.
>>>>> btrfs_using_import_iovec() could use fixed buffer if cmd flags has
>>>>> IORING_URING_CMD_FIXED.
>>>>>
>>>>> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
>>>>> ---
>>>>>   fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++--------
>>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>>> index 6c18bad53cd3..a7b52fd99059 100644
>>>>> --- a/fs/btrfs/ioctl.c
>>>>> +++ b/fs/btrfs/ioctl.c
>>>>> @@ -4802,6 +4802,28 @@ struct btrfs_uring_encoded_data {
>>>>>   	struct iov_iter iter;
>>>>>   };
>>>>> +static int btrfs_uring_import_iovec(struct io_uring_cmd *cmd,
>>>>> +				    unsigned int issue_flags, int rw)
>>>>> +{
>>>>> +	struct btrfs_uring_encoded_data *data =
>>>>> +		io_uring_cmd_get_async_data(cmd)->op_data;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (cmd && (cmd->flags & IORING_URING_CMD_FIXED)) {
>>>>> +		data->iov = NULL;
>>>>> +		ret = io_uring_cmd_import_fixed_vec(cmd, data->args.iov,
>>>>> +						    data->args.iovcnt,
>>>>> +						    ITER_DEST, issue_flags,
>>>>> +						    &data->iter);
>>>>> +	} else {
>>>>> +		data->iov = data->iovstack;
>>>>> +		ret = import_iovec(rw, data->args.iov, data->args.iovcnt,
>>>>> +				   ARRAY_SIZE(data->iovstack), &data->iov,
>>>>> +				   &data->iter);
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>
>>>> How can 'cmd' be NULL here?
>>>
>>> It seems that there is no checkes for 'cmd' before and it works same as before.
>>> But I think it's better to add a check in function start for safety.
>>
>> The check goes two lines after you already dereferenced it, and it
>> seems to be called from io_uring cmd specific code. The null check
>> only adds to confusion.
> 
> You mean 'cmd' already dereferenced for async_data. Is it okay to just
> delete code checking 'cmd' like below?
> 
> if (cmd->flags & IORING_URING_CMD_FIXED) {

Either 'cmd' may be NULL and it's valid (and the current code is fine),
or it'd be invalid, in which case that should return an error. But we
don't add random pointer == NULL checks as defensive programming. And
this one should never ever see cmd == NULL, hence the code needs to go
away. It's just confusing otherwise. Consider you reading some code
trying to understand what it does, and it has a check for cmd == NULL in
there. That would make you wonder "hmm what cases pass in a NULL for cmd
here?". When the answer is "this should never happen", don't have the
check. It just obfuscates the code.

-- 
Jens Axboe