On Fri, Aug 31, 2018 at 07:16:03AM -0700, Steve Wise wrote:
> Currently a uverbs completion event queue is flushed of events in
> ib_uverbs_comp_event_close() with the queue spinlock held and then
> released. Yet setting ev_queue->is_closed is not set until later in
> uverbs_hot_unplug_completion_event_file().
>
> In between the time ib_uverbs_comp_event_close() releases the lock
> and uverbs_hot_unplug_completion_event_file() acquires the lock, a
> completion event can arrive and be inserted into the event queue by
> ib_uverbs_comp_handler().
>
> This can cause a "double add" list_add warning or crash depending on
> the kernel configuration, or a memory leak because the event is never
> dequeued since the queue is already closed down.
>
> So move the setting of ev_queue->is_closed to
> ib_uverbs_comp_event_close().
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 1e7710f3f656 ("IB/core: Change completion channel to use the reworked objects schema")
> Signed-off-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
>
> I added the RFC tag because I'm not sure removing setting
> is_closed in uverbs_hot_unplug_completion_event_file() is correct.
> Can uverbs_hot_unplug_completion_event_file() be called w/o
> ib_uverbs_comp_event_close() being called first? Should is_closed be
> set in both places?
>
> drivers/infiniband/core/uverbs_main.c | 1 +
> drivers/infiniband/core/uverbs_std_types.c | 4 ----
> 2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 823beca448e1..b3ef7db68bde 100644
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -440,6 +440,7 @@ static int ib_uverbs_comp_event_close(struct inode *inode, struct file *filp)
> list_del(&entry->obj_list);
> kfree(entry);
> }
> + file->ev_queue.is_closed = 1;
> spin_unlock_irq(&file->ev_queue.lock);
Since event_close is essentially the release function of a kref, it is
definately wrong to set is_closed here, as there can be nothing to
read it.
> uverbs_close_fd(filp);
> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
> index 203cc96ac6f5..76fc9b7fec59 100644
> +++ b/drivers/infiniband/core/uverbs_std_types.c
> @@ -199,10 +199,6 @@ static int uverbs_hot_unplug_completion_event_file(struct ib_uobject *uobj,
> uobj);
> struct ib_uverbs_event_queue *event_queue = &comp_event_file->ev_queue;
>
> - spin_lock_irq(&event_queue->lock);
> - event_queue->is_closed = 1;
> - spin_unlock_irq(&event_queue->lock);
> -
Hot unplug and close are totally different things, the is_closed must
remain here to block access to a disassociated device.
Jason
