Re: [PATCH RFC] ib_uverbs: atomically flush and mark closed the comp event queue

[Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>]

On 9/1/2018 3:45 PM, Jason Gunthorpe wrote:
> On Fri, Aug 31, 2018 at 07:16:03AM -0700, Steve Wise wrote:
>> Currently a uverbs completion event queue is flushed of events in
>> ib_uverbs_comp_event_close() with the queue spinlock held and then
>> released.  Yet setting ev_queue->is_closed is not set until later in
>> uverbs_hot_unplug_completion_event_file().
>>
>> In between the time ib_uverbs_comp_event_close() releases the lock
>> and uverbs_hot_unplug_completion_event_file() acquires the lock, a
>> completion event can arrive and be inserted into the event queue by
>> ib_uverbs_comp_handler().
>>
>> This can cause a "double add" list_add warning or crash depending on
>> the kernel configuration, or a memory leak because the event is never
>> dequeued since the queue is already closed down.
>>
>> So move the setting of ev_queue->is_closed to
>> ib_uverbs_comp_event_close().
>>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Fixes: 1e7710f3f656 ("IB/core: Change completion channel to use the reworked objects schema")
>> Signed-off-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
>>
>> I added the RFC tag because I'm not sure removing setting
>> is_closed in uverbs_hot_unplug_completion_event_file() is correct.
>> Can uverbs_hot_unplug_completion_event_file() be called w/o
>> ib_uverbs_comp_event_close() being called first?  Should is_closed be
>> set in both places?
>>
>>  drivers/infiniband/core/uverbs_main.c      | 1 +
>>  drivers/infiniband/core/uverbs_std_types.c | 4 ----
>>  2 files changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
>> index 823beca448e1..b3ef7db68bde 100644
>> +++ b/drivers/infiniband/core/uverbs_main.c
>> @@ -440,6 +440,7 @@ static int ib_uverbs_comp_event_close(struct inode *inode, struct file *filp)
>>  			list_del(&entry->obj_list);
>>  		kfree(entry);
>>  	}
>> +	file->ev_queue.is_closed = 1;
>>  	spin_unlock_irq(&file->ev_queue.lock);
> 
> Since event_close is essentially the release function of a kref, it is
> definately wrong to set is_closed here, as there can be nothing to
> read it.

Hey Jason,

Question:  Why is there similar logic in ib_uverbs_async_event_close()
to set ev_queue.is_closed for the async ev_queue?:

...
        mutex_lock(&uverbs_file->device->lists_mutex);
        spin_lock_irq(&file->ev_queue.lock);
        closed_already = file->ev_queue.is_closed;
        file->ev_queue.is_closed = 1;
        list_for_each_entry_safe(entry, tmp, &file->ev_queue.event_list,
list) {
                if (entry->counter)
                        list_del(&entry->obj_list);
                kfree(entry);
        }
        spin_unlock_irq(&file->ev_queue.lock);
        if (!closed_already) {
                list_del(&file->list);
                ib_unregister_event_handler(&uverbs_file->event_handler);
        }
        mutex_unlock(&uverbs_file->device->lists_mutex);
...


Thanks,

Steve.