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
