| Message ID | 1446039867-27883-1-git-send-email-kaike.wan@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Oct 28, 2015 at 09:44:27AM -0400, kaike.wan@intel.com wrote: > From: Kaike Wan <kaike.wan@intel.com> > > It was found by Saurabh Sengar that the netlink code tried to allocate > memory with GFP_KERNEL while holding a spinlock. While it is possible > to fix the issue by replacing GFP_KERNEL with GFP_ATOMIC, it is better > to get rid of the spinlock while sending the packet. However, in order > to protect against a race condition that a quick response may be received > before the request is put on the request list, we need to put the request > on the list first. > > Signed-off-by: Kaike Wan <kaike.wan@intel.com> > --- > drivers/infiniband/core/sa_query.c | 22 ++++++++++++---------- > 1 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c > index edcf568..240b57c 100644 > --- a/drivers/infiniband/core/sa_query.c > +++ b/drivers/infiniband/core/sa_query.c > @@ -562,23 +562,25 @@ static int ib_nl_make_request(struct ib_sa_query *query) > INIT_LIST_HEAD(&query->list); > query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq); > > + /* Put the request on the list first.*/ > spin_lock_irqsave(&ib_nl_request_lock, flags); > + delay = msecs_to_jiffies(sa_local_svc_timeout_ms); > + query->timeout = delay + jiffies; > + list_add_tail(&query->list, &ib_nl_request_list); > + spin_unlock_irqrestore(&ib_nl_request_lock, flags); > + > ret = ib_nl_send_msg(query); What about using the gfp_mask through this stack? I think you need to split ib_nl_send_msg into "create message" and "send message". Then don't add the message to the list unless it is ready to go. Then you can get rid of the code below which is removing it on error. > + spin_lock_irqsave(&ib_nl_request_lock, flags); Do we still need the spin lock? > if (ret <= 0) { > ret = -EIO; > - goto request_out; > + /* Remove the request */ > + list_del(&query->list); Don't need to do this if we split ib_nl_send_msg. Ira > } else { > ret = 0; > + /* Start the timeout if this is the only request */ > + if (ib_nl_request_list.next == &query->list) > + queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay); > } > - > - delay = msecs_to_jiffies(sa_local_svc_timeout_ms); > - query->timeout = delay + jiffies; > - list_add_tail(&query->list, &ib_nl_request_list); > - /* Start the timeout if this is the only request */ > - if (ib_nl_request_list.next == &query->list) > - queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay); > - > -request_out: > spin_unlock_irqrestore(&ib_nl_request_lock, flags); > > return ret; > -- > 1.7.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 28, 2015 at 09:44:27AM -0400, kaike.wan@intel.com wrote: > ret = ib_nl_send_msg(query); > + spin_lock_irqsave(&ib_nl_request_lock, flags); Looks like query could be kfree'd before ib_nl_send_msg returns, eg by send_handler? > if (ret <= 0) { > ret = -EIO; > - goto request_out; > + /* Remove the request */ > + list_del(&query->list); This one is probably OK iff nl_send_msg cannot call send_handler if it returns error, which looks true. > } else { > ret = 0; > + /* Start the timeout if this is the only request */ > + if (ib_nl_request_list.next == &query->list) This one looks sketchy. Maybe move this to the first locking block? A extra timer on send error is not important enough to worry about.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Weiny, Ira > Sent: Wednesday, October 28, 2015 11:40 AM > > What about using the gfp_mask through this stack? Can be done. > > I think you need to split ib_nl_send_msg into "create message" and "send > message". Then don't add the message to the list unless it is ready to go. > Then you can get rid of the code below which is removing it on error. Even if you split it, you can't get rid of the code unless you hold the spinlock because the sending could fail. > > > + spin_lock_irqsave(&ib_nl_request_lock, flags); > > Do we still need the spin lock? Yes: to remove it from the list in case of failure or schedule the delayed work. > > > if (ret <= 0) { > > ret = -EIO; > > - goto request_out; > > + /* Remove the request */ > > + list_del(&query->list); > > Don't need to do this if we split ib_nl_send_msg. See above. Kaike > > Ira > > > } else { > > ret = 0; > > + /* Start the timeout if this is the only request */ > > + if (ib_nl_request_list.next == &query->list) > > + queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, > delay); > > } > > - > > - delay = msecs_to_jiffies(sa_local_svc_timeout_ms); > > - query->timeout = delay + jiffies; > > - list_add_tail(&query->list, &ib_nl_request_list); > > - /* Start the timeout if this is the only request */ > > - if (ib_nl_request_list.next == &query->list) > > - queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay); > > - > > -request_out: > > spin_unlock_irqrestore(&ib_nl_request_lock, flags); > > > > return ret; > > -- > > 1.7.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" > > in the body of a message to majordomo@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] > Sent: Wednesday, October 28, 2015 12:47 PM > > > ret = ib_nl_send_msg(query); > > + spin_lock_irqsave(&ib_nl_request_lock, flags); > > Looks like query could be kfree'd before ib_nl_send_msg returns, eg by > send_handler? It's possible only when the request is successfully sent and a response is received before ib_nl_send_msg returns. Therefore, we should not touch the request and query if the sending is successfully. However, if the sending fails, we could remove the request from the list. > > > > if (ret <= 0) { > > ret = -EIO; > > - goto request_out; > > + /* Remove the request */ > > + list_del(&query->list); > > This one is probably OK iff nl_send_msg cannot call send_handler if it returns > error, which looks true. Correct. > > > } else { > > ret = 0; > > + /* Start the timeout if this is the only request */ > > + if (ib_nl_request_list.next == &query->list) > > This one looks sketchy. Maybe move this to the first locking block? A extra > timer on send error is not important enough to worry about.. You are correct. We should move it into the first block. Kaike > > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c index edcf568..240b57c 100644 --- a/drivers/infiniband/core/sa_query.c +++ b/drivers/infiniband/core/sa_query.c @@ -562,23 +562,25 @@ static int ib_nl_make_request(struct ib_sa_query *query) INIT_LIST_HEAD(&query->list); query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq); + /* Put the request on the list first.*/ spin_lock_irqsave(&ib_nl_request_lock, flags); + delay = msecs_to_jiffies(sa_local_svc_timeout_ms); + query->timeout = delay + jiffies; + list_add_tail(&query->list, &ib_nl_request_list); + spin_unlock_irqrestore(&ib_nl_request_lock, flags); + ret = ib_nl_send_msg(query); + spin_lock_irqsave(&ib_nl_request_lock, flags); if (ret <= 0) { ret = -EIO; - goto request_out; + /* Remove the request */ + list_del(&query->list); } else { ret = 0; + /* Start the timeout if this is the only request */ + if (ib_nl_request_list.next == &query->list) + queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay); } - - delay = msecs_to_jiffies(sa_local_svc_timeout_ms); - query->timeout = delay + jiffies; - list_add_tail(&query->list, &ib_nl_request_list); - /* Start the timeout if this is the only request */ - if (ib_nl_request_list.next == &query->list) - queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay); - -request_out: spin_unlock_irqrestore(&ib_nl_request_lock, flags); return ret;