> On 7 Jan 2020, at 19:42, Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> On Mon, Jan 06, 2020 at 01:27:11PM +0100, Håkon Bugge wrote:
>> In ib_uverbs_event_read(), events are waited for, then pulled off the
>> kernel's event queue, and finally returned to user space.
>>
>> There is an explicit check to see if the device is gone, and if so and
>> the there are no events pending, an -EIO is returned.
>>
>> However, said test does not check for queue empty whilst holding the
>> lock, so there is a race where the existing code perceives the queue
>> to be empty, when it in fact isn't. Fixed by acquiring the lock ahead
>> of the list_empty() test.
>>
>> Fixes: 036b10635739 ("IB/uverbs: Enable device removal when there are active user space applications")
>> Signed-off-by: Håkon Bugge <haakon.bugge@xxxxxxxxxx>
>> ---
>> drivers/infiniband/core/uverbs_main.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
>> index 970d8e31dd65..7165e51790ed 100644
>> --- a/drivers/infiniband/core/uverbs_main.c
>> +++ b/drivers/infiniband/core/uverbs_main.c
>> @@ -245,12 +245,14 @@ static ssize_t ib_uverbs_event_read(struct ib_uverbs_event_queue *ev_queue,
>> !uverbs_file->device->ib_dev)))
>> return -ERESTARTSYS;
>>
>> + spin_lock_irq(&ev_queue->lock);
>> +
>> /* If device was disassociated and no event exists set an error */
>> if (list_empty(&ev_queue->event_list) &&
>> - !uverbs_file->device->ib_dev)
>> + !uverbs_file->device->ib_dev) {
>> + spin_unlock_irq(&ev_queue->lock);
>> return -EIO;
>
> I noticed this too last month. While this patch is an improvement, I
> had written this one which also fixes the wrong use of devce->ib_dev
> without a READ_ONCE or locking. It is just winding its way through
> testing right now.
>
> What do you think?
>
> From 37ddee0ea682eaf47e6167a090ae0a4e943f7f68 Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> Date: Tue, 26 Nov 2019 20:58:04 -0400
> Subject: [PATCH] RDMA/core: Fix locking in ib_uverbs_event_read
>
> This should not be using ib_dev to test for disassociation, during
> disassociation is_closed is set under lock and the waitq is triggered.
>
> Instead check is_closed and be sure to re-obtain the lock to test the
> value after the wait_event returns.
>
> Fixes: 036b10635739 ("IB/uverbs: Enable device removal when there are active user space applications")
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
LGTM,
Reviewed-by: Håkon Bugge <haakon.bugge@xxxxxxxxxx>
(or feel free to use my s-o-b, as we coined 80% of this independently of each other).
Thxs, Håkon
> ---
> drivers/infiniband/core/uverbs_main.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 953a8c3fae64bd..2b7dc94b6a7a69 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -215,7 +215,6 @@ void ib_uverbs_release_file(struct kref *ref)
> }
>
> static ssize_t ib_uverbs_event_read(struct ib_uverbs_event_queue *ev_queue,
> - struct ib_uverbs_file *uverbs_file,
> struct file *filp, char __user *buf,
> size_t count, loff_t *pos,
> size_t eventsz)
> @@ -233,19 +232,16 @@ static ssize_t ib_uverbs_event_read(struct ib_uverbs_event_queue *ev_queue,
>
> if (wait_event_interruptible(ev_queue->poll_wait,
> (!list_empty(&ev_queue->event_list) ||
> - /* The barriers built into wait_event_interruptible()
> - * and wake_up() guarentee this will see the null set
> - * without using RCU
> - */
> - !uverbs_file->device->ib_dev)))
> + ev_queue->is_closed)))
> return -ERESTARTSYS;
>
> + spin_lock_irq(&ev_queue->lock);
> +
> /* If device was disassociated and no event exists set an error */
> - if (list_empty(&ev_queue->event_list) &&
> - !uverbs_file->device->ib_dev)
> + if (list_empty(&ev_queue->event_list) && ev_queue->is_closed) {
> + spin_unlock_irq(&ev_queue->lock);
> return -EIO;
> -
> - spin_lock_irq(&ev_queue->lock);
> + }
> }
>
> event = list_entry(ev_queue->event_list.next, struct ib_uverbs_event, list);
> @@ -280,8 +276,7 @@ static ssize_t ib_uverbs_async_event_read(struct file *filp, char __user *buf,
> {
> struct ib_uverbs_async_event_file *file = filp->private_data;
>
> - return ib_uverbs_event_read(&file->ev_queue, file->uverbs_file, filp,
> - buf, count, pos,
> + return ib_uverbs_event_read(&file->ev_queue, filp, buf, count, pos,
> sizeof(struct ib_uverbs_async_event_desc));
> }
>
> @@ -291,9 +286,8 @@ static ssize_t ib_uverbs_comp_event_read(struct file *filp, char __user *buf,
> struct ib_uverbs_completion_event_file *comp_ev_file =
> filp->private_data;
>
> - return ib_uverbs_event_read(&comp_ev_file->ev_queue,
> - comp_ev_file->uobj.ufile, filp,
> - buf, count, pos,
> + return ib_uverbs_event_read(&comp_ev_file->ev_queue, filp, buf, count,
> + pos,
> sizeof(struct ib_uverbs_comp_event_desc));
> }
>
> --
> 2.24.1
>
