Re: [PATCH RDMA/uverbs] RDMA/uverbs: Protect list_empty() by lock

["Håkon Bugge" <haakon.bugge@oracle.com>]

> On 7 Jan 2020, at 19:42, Jason Gunthorpe <jgg@ziepe.ca> 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@oracle.com>
>> ---
>> 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@mellanox.com>
> 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@mellanox.com>

LGTM,

Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
(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
>