On Thu, Mar 07, 2019 at 01:53:44AM -0800, Ira Weiny wrote:
> > @@ -440,11 +441,17 @@ static void aliasguid_query_handler(int status,
> > alias_guid_work,
> > msecs_to_jiffies(resched_delay_sec * 1000));
> > }
> > - if (cb_ctx->sa_query) {
> > - list_del(&cb_ctx->list);
> > + if (!list_empty(&cb_ctx->list)) {
> > + /* we own the callback context */
> > + list_del_init(&cb_ctx->list);
> > kfree(cb_ctx);
>
> Doesn't aliasguid_query_handler() get called from send_handler()?
> Which means that this kfree() will delete the query before
> send_handler is done with it?
Yep, send_handler can't touch query after calling callback in this
mode.
Probably the best solution is to have a ib_sa_destory_query() that
does the free_mad/etc, and do something like this:
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 6c221ec919640d..b563e1ffd06772 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -2150,6 +2150,7 @@ static void send_handler(struct ib_mad_agent *agent,
{
struct ib_sa_query *query = mad_send_wc->send_buf->context[0];
unsigned long flags;
+ bool callback_owns_memory = !query->release;
if (query->callback)
switch (mad_send_wc->status) {
@@ -2167,6 +2168,10 @@ static void send_handler(struct ib_mad_agent *agent,
break;
}
+ /* The callback must call ib_sa_destroy() */
+ if (callback_owns_memory)
+ return;
+
spin_lock_irqsave(&idr_lock, flags);
idr_remove(&query_idr, query->id);
spin_unlock_irqrestore(&idr_lock, flags);
@@ -2174,8 +2179,7 @@ static void send_handler(struct ib_mad_agent *agent,
free_mad(query);
if (query->client)
ib_sa_client_put(query->client);
- if (query->release)
- query->release(query);
+ query->release(query);
}
static void recv_handler(struct ib_mad_agent *mad_agent,
> This whole patch seems like an anti-patern from the rest of the SA query
> interface. What if ib_sa_guidinfo_rec_callback() took a reference to the
> sa_query before it returns it and allow the free to happen when that reference
> is put from the driver?
The entire sa query interface is a giant anti-pattern with bugs. This
approach the correct way to do things like this.
Jason
