| 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 |
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)); }
> 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 --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);
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(-)