* [PATCH RDMA/uverbs] RDMA/uverbs: Protect list_empty() by lock
@ 2020-01-06 12:27 Håkon Bugge
2020-01-07 18:42 ` Jason Gunthorpe
0 siblings, 1 reply; 3+ messages in thread
From: Håkon Bugge @ 2020-01-06 12:27 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: linux-rdma
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;
-
- spin_lock_irq(&ev_queue->lock);
+ }
}
event = list_entry(ev_queue->event_list.next, struct ib_uverbs_event, list);
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH RDMA/uverbs] RDMA/uverbs: Protect list_empty() by lock
2020-01-06 12:27 [PATCH RDMA/uverbs] RDMA/uverbs: Protect list_empty() by lock Håkon Bugge
@ 2020-01-07 18:42 ` Jason Gunthorpe
2020-01-08 14:23 ` Håkon Bugge
0 siblings, 1 reply; 3+ messages in thread
From: Jason Gunthorpe @ 2020-01-07 18:42 UTC (permalink / raw)
To: Håkon Bugge; +Cc: Doug Ledford, linux-rdma
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));
}
--
2.24.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH RDMA/uverbs] RDMA/uverbs: Protect list_empty() by lock
2020-01-07 18:42 ` Jason Gunthorpe
@ 2020-01-08 14:23 ` Håkon Bugge
0 siblings, 0 replies; 3+ messages in thread
From: Håkon Bugge @ 2020-01-08 14:23 UTC (permalink / raw)
To: Jason Gunthorpe, Jason Gunthorpe; +Cc: Doug Ledford, OFED mailing list
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-08 14:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-06 12:27 [PATCH RDMA/uverbs] RDMA/uverbs: Protect list_empty() by lock Håkon Bugge
2020-01-07 18:42 ` Jason Gunthorpe
2020-01-08 14:23 ` Håkon Bugge
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).