diff mbox series

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

Message ID 20200106122711.217198-1-haakon.bugge@oracle.com (mailing list archive)
State Superseded
Headers show
Series [RDMA/uverbs] RDMA/uverbs: Protect list_empty() by lock | expand

Commit Message

Haakon Bugge Jan. 6, 2020, 12:27 p.m. UTC
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(-)

Comments

Jason Gunthorpe Jan. 7, 2020, 6:42 p.m. UTC | #1
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>
---
 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));
 }
Haakon Bugge Jan. 8, 2020, 2:23 p.m. UTC | #2
> 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
>
diff mbox series

Patch

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;
-
-		spin_lock_irq(&ev_queue->lock);
+		}
 	}
 
 	event = list_entry(ev_queue->event_list.next, struct ib_uverbs_event, list);