Re: [PATCH 5/5] io_uring: add support for user mapped provided buffer ring

[Jens Axboe <axboe@kernel.dk>]

On 3/16/23 12:07 PM, Ammar Faizi wrote:
> I tried to verify the for-next build report. And I think this doesn't
> look right.
> 
> On Tue, Mar 14, 2023 at 11:16:42AM -0600, Jens Axboe wrote:
>> @@ -214,15 +215,27 @@ static int __io_remove_buffers(struct io_ring_ctx *ctx,
>>  	if (!nbufs)
>>  		return 0;
>>  
>> -	if (bl->is_mapped && bl->buf_nr_pages) {
>> -		int j;
>> -
>> +	if (bl->is_mapped) {
>>  		i = bl->buf_ring->tail - bl->head;
>                     ^^^^^^^^^^^^^^^^^^
> 
> Dereference bl->buf_ring. It implies bl->buf_ring is not NULL.
> 
>> -		for (j = 0; j < bl->buf_nr_pages; j++)
>> -			unpin_user_page(bl->buf_pages[j]);
>> -		kvfree(bl->buf_pages);
>> -		bl->buf_pages = NULL;
>> -		bl->buf_nr_pages = 0;
>> +		if (bl->is_mmap) {
>> +			if (bl->buf_ring) {
>                         ^^^^^^^^^^^^^^^^^
> 
> A NULL check against bl->buf_ring here. If it was possible to be NULL,
> wouldn't the above dereference BUG()?

I don't think it's possible and we should probably just remove that
latter check. If the buffer group is visible, either method will have
a valid ->buf_ring IFF is_mmap/is_mapped is set.

-- 
Jens Axboe