On Mon, Sep 10, 2018 at 02:24:03PM -0500, Steve Wise wrote:
>
>
> On 9/1/2018 3:45 PM, Jason Gunthorpe wrote:
> > 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.
>
> Hey Jason,
>
> Question: Why is there similar logic in ib_uverbs_async_event_close()
> to set ev_queue.is_closed for the async ev_queue?:
Presumably because those functions use a different refcounting scheme
for their event queue..
Oh, but they don't! ib_uverbs_lookup_comp_file() drops the kref on the
file * and just retains on the uobject! Why!?
That creates the possibility of a half destructed uobject!!
Everytime I look at this stuff I end up in tears :P
So, yes, it does look helpfull to move the is_closed like this..
But better would be to just not have something so insane in the first
place :(
Jason
