| Message ID | 0-v1-ace813388969+48859-uverbs_poll_fix%jgg@mellanox.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | eb356e6dc15a30af604f052cd0e170450193c254 |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/uverbs: Make the event_queue fds return POLLERR when disassociated | expand |
On Mon, Apr 06, 2020 at 09:44:26PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > If is_closed is set, and the event list is empty, then read() will return > -EIO without blocking. After setting is_closed in > ib_uverbs_free_event_queue(), we do trigger a wake_up on the poll_wait, > but the fops->poll() function does not check it, so poll will continue to > sleep on an empty list. > > Fixes: 14e23bd6d221 ("RDMA/core: Fix locking in ib_uverbs_event_read") > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > --- > drivers/infiniband/core/uverbs_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c > index 2d4083bf4a0487..8710a3427146e7 100644 > --- a/drivers/infiniband/core/uverbs_main.c > +++ b/drivers/infiniband/core/uverbs_main.c > @@ -296,6 +296,8 @@ static __poll_t ib_uverbs_event_poll(struct ib_uverbs_event_queue *ev_queue, > spin_lock_irq(&ev_queue->lock); > if (!list_empty(&ev_queue->event_list)) > pollflags = EPOLLIN | EPOLLRDNORM; > + else if (ev_queue->is_closed) > + pollflags = EPOLLERR; > spin_unlock_irq(&ev_queue->lock); Don't you need to set EPOLLHUP too? Probably, it won't change anything, just for the sake of the correctness. Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Tue, Apr 07, 2020 at 08:16:32AM +0300, Leon Romanovsky wrote: > On Mon, Apr 06, 2020 at 09:44:26PM -0300, Jason Gunthorpe wrote: > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > If is_closed is set, and the event list is empty, then read() will return > > -EIO without blocking. After setting is_closed in > > ib_uverbs_free_event_queue(), we do trigger a wake_up on the poll_wait, > > but the fops->poll() function does not check it, so poll will continue to > > sleep on an empty list. > > > > Fixes: 14e23bd6d221 ("RDMA/core: Fix locking in ib_uverbs_event_read") > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > drivers/infiniband/core/uverbs_main.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c > > index 2d4083bf4a0487..8710a3427146e7 100644 > > +++ b/drivers/infiniband/core/uverbs_main.c > > @@ -296,6 +296,8 @@ static __poll_t ib_uverbs_event_poll(struct ib_uverbs_event_queue *ev_queue, > > spin_lock_irq(&ev_queue->lock); > > if (!list_empty(&ev_queue->event_list)) > > pollflags = EPOLLIN | EPOLLRDNORM; > > + else if (ev_queue->is_closed) > > + pollflags = EPOLLERR; > > spin_unlock_irq(&ev_queue->lock); > > Don't you need to set EPOLLHUP too? Probably, it won't change anything, > just for the sake of the correctness. HUP means read will return 0, in this case read returns -EIO Jason
On Tue, Apr 07, 2020 at 08:51:15AM -0300, Jason Gunthorpe wrote: > On Tue, Apr 07, 2020 at 08:16:32AM +0300, Leon Romanovsky wrote: > > On Mon, Apr 06, 2020 at 09:44:26PM -0300, Jason Gunthorpe wrote: > > > From: Jason Gunthorpe <jgg@mellanox.com> > > > > > > If is_closed is set, and the event list is empty, then read() will return > > > -EIO without blocking. After setting is_closed in > > > ib_uverbs_free_event_queue(), we do trigger a wake_up on the poll_wait, > > > but the fops->poll() function does not check it, so poll will continue to > > > sleep on an empty list. > > > > > > Fixes: 14e23bd6d221 ("RDMA/core: Fix locking in ib_uverbs_event_read") > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > drivers/infiniband/core/uverbs_main.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c > > > index 2d4083bf4a0487..8710a3427146e7 100644 > > > +++ b/drivers/infiniband/core/uverbs_main.c > > > @@ -296,6 +296,8 @@ static __poll_t ib_uverbs_event_poll(struct ib_uverbs_event_queue *ev_queue, > > > spin_lock_irq(&ev_queue->lock); > > > if (!list_empty(&ev_queue->event_list)) > > > pollflags = EPOLLIN | EPOLLRDNORM; > > > + else if (ev_queue->is_closed) > > > + pollflags = EPOLLERR; > > > spin_unlock_irq(&ev_queue->lock); > > > > Don't you need to set EPOLLHUP too? Probably, it won't change anything, > > just for the sake of the correctness. > > HUP means read will return 0, in this case read returns -EIO I see, it is because we don't have events to read. Thanks > > Jason
On Mon, Apr 06, 2020 at 09:44:26PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > If is_closed is set, and the event list is empty, then read() will return > -EIO without blocking. After setting is_closed in > ib_uverbs_free_event_queue(), we do trigger a wake_up on the poll_wait, > but the fops->poll() function does not check it, so poll will continue to > sleep on an empty list. > > Fixes: 14e23bd6d221 ("RDMA/core: Fix locking in ib_uverbs_event_read") > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/core/uverbs_main.c | 2 ++ > 1 file changed, 2 insertions(+) Applied to for-next Jason
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 2d4083bf4a0487..8710a3427146e7 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -296,6 +296,8 @@ static __poll_t ib_uverbs_event_poll(struct ib_uverbs_event_queue *ev_queue, spin_lock_irq(&ev_queue->lock); if (!list_empty(&ev_queue->event_list)) pollflags = EPOLLIN | EPOLLRDNORM; + else if (ev_queue->is_closed) + pollflags = EPOLLERR; spin_unlock_irq(&ev_queue->lock); return pollflags;