| Message ID | 1444568298-17289-1-git-send-email-matanb@mellanox.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
> Fixes: a977049dacde ('[PATCH] IB: Add the kernel CM implementation')
Please remove the the "[PATCH]" thing from the change-log and respin
--
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 Mon, Oct 12, 2015 at 10:13 AM, Matan Barak <matanb@mellanox.com> wrote: > But that's the name of the commit.... > > -----Original Message----- > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-owner@vger.kernel.org] On Behalf Of Or Gerlitz > Sent: Sunday, October 11, 2015 6:28 PM > To: Matan Barak <matanb@mellanox.com> > Cc: Doug Ledford <dledford@redhat.com>; linux-rdma@vger.kernel.org; Or Gerlitz <ogerlitz@mellanox.com>; Eran Ben Elisha <eranbe@mellanox.com>; Sean Hefty <sean.hefty@intel.com>; Jason Gunthorpe <jgunthorpe@obsidianresearch.com>; Doron Tsur <doront@mellanox.com> > Subject: Re: [PATCH rdma-RC] IB/cm: Fix rb-tree duplicate free and use-after-free > >> Fixes: a977049dacde ('[PATCH] IB: Add the kernel CM implementation') > > Please remove the the "[PATCH]" thing from the change-log and respin oops, back in 2005 this appeared in the commit title... so ok to let it go here... Or. -- 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
> ib_send_cm_sidr_rep could sometimes erase the node from the sidr > (depending on errors in the process). Since ib_send_cm_sidr_rep is > called both from cm_sidr_req_handler and cm_destroy_id, cm_id_priv This should clarify that it is the app calling from the callback, and not a direct call from the cm_sidr_req_handler. > could be either erased from the rb_tree twice or not erased at all. In an error case, I can see why it would be left in the rbtree, but I don't see how it can be removed twice. > Fixing that by making sure it's erased only once before freeing > cm_id_priv. > > Fixes: a977049dacde ('[PATCH] IB: Add the kernel CM implementation') > Signed-off-by: Doron Tsur <doront@mellanox.com> > Signed-off-by: Matan Barak <matanb@mellanox.com> > --- > > Hi Doug, > This patch fixes a bug in the CM. In some flow, rb-tree could be > freed twice or used after it was freed. This bug was picked by > our regression tests and this fix was verified. > > Thanks, > Doron and Matan > > drivers/infiniband/core/cm.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index f5cf1c4..56ff0f3 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -844,6 +844,11 @@ retest: > case IB_CM_SIDR_REQ_RCVD: > spin_unlock_irq(&cm_id_priv->lock); > cm_reject_sidr_req(cm_id_priv, IB_SIDR_REJECT); > + spin_lock_irq(&cm.lock); > + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) > + rb_erase(&cm_id_priv->sidr_id_node, > + &cm.remote_sidr_table); > + spin_unlock_irq(&cm.lock); We should be able to use a return value from cm_reject_sidr_req() -- passed through from ib_send_cm_sidr_rep() to determine if the id was removed from the tree. > break; > case IB_CM_REQ_SENT: > case IB_CM_MRA_REQ_RCVD: > @@ -3210,7 +3215,10 @@ int ib_send_cm_sidr_rep(struct ib_cm_id *cm_id, > spin_unlock_irqrestore(&cm_id_priv->lock, flags); > > spin_lock_irqsave(&cm.lock, flags); > - rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); > + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) { > + rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); > + RB_CLEAR_NODE(&cm_id_priv->sidr_id_node); > + } > spin_unlock_irqrestore(&cm.lock, flags); Something is very wrong in this function if the id is not in the tree at this point. -- 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 10/12/2015 7:37 PM, Hefty, Sean wrote: >> ib_send_cm_sidr_rep could sometimes erase the node from the sidr >> (depending on errors in the process). Since ib_send_cm_sidr_rep is >> called both from cm_sidr_req_handler and cm_destroy_id, cm_id_priv > > This should clarify that it is the app calling from the callback, and not a direct call from the cm_sidr_req_handler. > Consider the following error flows: Double free: cm_sidr_req_handler:3156->cm_reject_sidr_req:663->ib_send_cm_sidr_rep:3233->erase_rb cm_sidr_req_handler:3173->ib_destroy_cm_id->cm_destroy_id:846->ib_send_cm_sidr_rep:3233->erase_rb RB contains free node: cm_sidr_req_handler:3156->cm_reject_sidr_req:663->ib_send_cm_sidr_rep->returns error(for example cm_alloc_msg,3219) cm_sidr_req_handler:3173->ib_destroy_cm_id->cm_destroy_id:846->cm_reject_sidr_req->cm_reject_sidr_req:663->returns error(for example cm_alloc_msg,3219)->RB wasn't erased but memory is freed :910 kfree(cm_id_priv) >> could be either erased from the rb_tree twice or not erased at all. > > In an error case, I can see why it would be left in the rbtree, but I don't see how it can be removed twice. > > >> Fixing that by making sure it's erased only once before freeing >> cm_id_priv. >> >> Fixes: a977049dacde ('[PATCH] IB: Add the kernel CM implementation') >> Signed-off-by: Doron Tsur <doront@mellanox.com> >> Signed-off-by: Matan Barak <matanb@mellanox.com> >> --- >> >> Hi Doug, >> This patch fixes a bug in the CM. In some flow, rb-tree could be >> freed twice or used after it was freed. This bug was picked by >> our regression tests and this fix was verified. >> >> Thanks, >> Doron and Matan >> >> drivers/infiniband/core/cm.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c >> index f5cf1c4..56ff0f3 100644 >> --- a/drivers/infiniband/core/cm.c >> +++ b/drivers/infiniband/core/cm.c >> @@ -844,6 +844,11 @@ retest: >> case IB_CM_SIDR_REQ_RCVD: >> spin_unlock_irq(&cm_id_priv->lock); >> cm_reject_sidr_req(cm_id_priv, IB_SIDR_REJECT); >> + spin_lock_irq(&cm.lock); >> + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) >> + rb_erase(&cm_id_priv->sidr_id_node, >> + &cm.remote_sidr_table); >> + spin_unlock_irq(&cm.lock); This change seeks to remove the about to be freed node from the rb tree, while verifying it has not been freed already > > We should be able to use a return value from cm_reject_sidr_req() -- passed through from ib_send_cm_sidr_rep() to determine if the id was removed from the tree. > But this won't protect from double free in ib_send_cm_sidr_rep, unless we pass this parameter to the cm destroy function, but this alternative is cumbersome. >> break; >> case IB_CM_REQ_SENT: >> case IB_CM_MRA_REQ_RCVD: >> @@ -3210,7 +3215,10 @@ int ib_send_cm_sidr_rep(struct ib_cm_id *cm_id, >> spin_unlock_irqrestore(&cm_id_priv->lock, flags); >> >> spin_lock_irqsave(&cm.lock, flags); >> - rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); >> + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) { >> + rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); >> + RB_CLEAR_NODE(&cm_id_priv->sidr_id_node); >> + } This change protects against double free >> spin_unlock_irqrestore(&cm.lock, flags); > > Something is very wrong in this function if the id is not in the tree at this point. > We agree, but there's an error flow that triggers this behavior. Regards, Matan and Doron. -- 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 10/15/2015 11:15 AM, Matan Barak wrote: > > > On 10/12/2015 7:37 PM, Hefty, Sean wrote: >>> ib_send_cm_sidr_rep could sometimes erase the node from the sidr >>> (depending on errors in the process). Since ib_send_cm_sidr_rep is >>> called both from cm_sidr_req_handler and cm_destroy_id, cm_id_priv >> >> This should clarify that it is the app calling from the callback, and >> not a direct call from the cm_sidr_req_handler. >> > > Consider the following error flows: > > Double free: > cm_sidr_req_handler:3156->cm_reject_sidr_req:663->ib_send_cm_sidr_rep:3233->erase_rb > > cm_sidr_req_handler:3173->ib_destroy_cm_id->cm_destroy_id:846->ib_send_cm_sidr_rep:3233->erase_rb > > > RB contains free node: > cm_sidr_req_handler:3156->cm_reject_sidr_req:663->ib_send_cm_sidr_rep->returns > error(for example cm_alloc_msg,3219) > > cm_sidr_req_handler:3173->ib_destroy_cm_id->cm_destroy_id:846->cm_reject_sidr_req->cm_reject_sidr_req:663->returns > > error(for example cm_alloc_msg,3219)->RB wasn't erased but memory is > freed :910 kfree(cm_id_priv) > > >>> could be either erased from the rb_tree twice or not erased at all. >> >> In an error case, I can see why it would be left in the rbtree, but I >> don't see how it can be removed twice. >> >> >>> Fixing that by making sure it's erased only once before freeing >>> cm_id_priv. >>> >>> Fixes: a977049dacde ('[PATCH] IB: Add the kernel CM implementation') >>> Signed-off-by: Doron Tsur <doront@mellanox.com> >>> Signed-off-by: Matan Barak <matanb@mellanox.com> >>> --- >>> >>> Hi Doug, >>> This patch fixes a bug in the CM. In some flow, rb-tree could be >>> freed twice or used after it was freed. This bug was picked by >>> our regression tests and this fix was verified. >>> >>> Thanks, >>> Doron and Matan >>> >>> drivers/infiniband/core/cm.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c >>> index f5cf1c4..56ff0f3 100644 >>> --- a/drivers/infiniband/core/cm.c >>> +++ b/drivers/infiniband/core/cm.c >>> @@ -844,6 +844,11 @@ retest: >>> case IB_CM_SIDR_REQ_RCVD: >>> spin_unlock_irq(&cm_id_priv->lock); >>> cm_reject_sidr_req(cm_id_priv, IB_SIDR_REJECT); >>> + spin_lock_irq(&cm.lock); >>> + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) >>> + rb_erase(&cm_id_priv->sidr_id_node, >>> + &cm.remote_sidr_table); >>> + spin_unlock_irq(&cm.lock); > > This change seeks to remove the about to be freed node from the rb tree, > while verifying it has not been freed already > >> >> We should be able to use a return value from cm_reject_sidr_req() -- >> passed through from ib_send_cm_sidr_rep() to determine if the id was >> removed from the tree. >> > > But this won't protect from double free in ib_send_cm_sidr_rep, unless > we pass this parameter to the cm destroy function, but this alternative > is cumbersome. > >>> break; >>> case IB_CM_REQ_SENT: >>> case IB_CM_MRA_REQ_RCVD: >>> @@ -3210,7 +3215,10 @@ int ib_send_cm_sidr_rep(struct ib_cm_id *cm_id, >>> spin_unlock_irqrestore(&cm_id_priv->lock, flags); >>> >>> spin_lock_irqsave(&cm.lock, flags); >>> - rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); >>> + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) { >>> + rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); >>> + RB_CLEAR_NODE(&cm_id_priv->sidr_id_node); >>> + } > > This change protects against double free > >>> spin_unlock_irqrestore(&cm.lock, flags); >> >> Something is very wrong in this function if the id is not in the tree >> at this point. >> > > We agree, but there's an error flow that triggers this behavior. Sean, I need to close on this patch. What is your position after Matan's explanation?
On 10/20/2015 04:27 PM, Doug Ledford wrote: > On 10/15/2015 11:15 AM, Matan Barak wrote: >> >> >> On 10/12/2015 7:37 PM, Hefty, Sean wrote: >>>> ib_send_cm_sidr_rep could sometimes erase the node from the sidr >>>> (depending on errors in the process). Since ib_send_cm_sidr_rep is >>>> called both from cm_sidr_req_handler and cm_destroy_id, cm_id_priv >>> >>> This should clarify that it is the app calling from the callback, and >>> not a direct call from the cm_sidr_req_handler. >>> >> >> Consider the following error flows: >> >> Double free: >> cm_sidr_req_handler:3156->cm_reject_sidr_req:663->ib_send_cm_sidr_rep:3233->erase_rb >> >> cm_sidr_req_handler:3173->ib_destroy_cm_id->cm_destroy_id:846->ib_send_cm_sidr_rep:3233->erase_rb >> >> >> RB contains free node: >> cm_sidr_req_handler:3156->cm_reject_sidr_req:663->ib_send_cm_sidr_rep->returns >> error(for example cm_alloc_msg,3219) >> >> cm_sidr_req_handler:3173->ib_destroy_cm_id->cm_destroy_id:846->cm_reject_sidr_req->cm_reject_sidr_req:663->returns >> >> error(for example cm_alloc_msg,3219)->RB wasn't erased but memory is >> freed :910 kfree(cm_id_priv) >> >> >>>> could be either erased from the rb_tree twice or not erased at all. >>> >>> In an error case, I can see why it would be left in the rbtree, but I >>> don't see how it can be removed twice. >>> >>> >>>> Fixing that by making sure it's erased only once before freeing >>>> cm_id_priv. >>>> >>>> Fixes: a977049dacde ('[PATCH] IB: Add the kernel CM implementation') >>>> Signed-off-by: Doron Tsur <doront@mellanox.com> >>>> Signed-off-by: Matan Barak <matanb@mellanox.com> >>>> --- >>>> >>>> Hi Doug, >>>> This patch fixes a bug in the CM. In some flow, rb-tree could be >>>> freed twice or used after it was freed. This bug was picked by >>>> our regression tests and this fix was verified. >>>> >>>> Thanks, >>>> Doron and Matan >>>> >>>> drivers/infiniband/core/cm.c | 10 +++++++++- >>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c >>>> index f5cf1c4..56ff0f3 100644 >>>> --- a/drivers/infiniband/core/cm.c >>>> +++ b/drivers/infiniband/core/cm.c >>>> @@ -844,6 +844,11 @@ retest: >>>> case IB_CM_SIDR_REQ_RCVD: >>>> spin_unlock_irq(&cm_id_priv->lock); >>>> cm_reject_sidr_req(cm_id_priv, IB_SIDR_REJECT); >>>> + spin_lock_irq(&cm.lock); >>>> + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) >>>> + rb_erase(&cm_id_priv->sidr_id_node, >>>> + &cm.remote_sidr_table); >>>> + spin_unlock_irq(&cm.lock); >> >> This change seeks to remove the about to be freed node from the rb tree, >> while verifying it has not been freed already >> >>> >>> We should be able to use a return value from cm_reject_sidr_req() -- >>> passed through from ib_send_cm_sidr_rep() to determine if the id was >>> removed from the tree. >>> >> >> But this won't protect from double free in ib_send_cm_sidr_rep, unless >> we pass this parameter to the cm destroy function, but this alternative >> is cumbersome. >> >>>> break; >>>> case IB_CM_REQ_SENT: >>>> case IB_CM_MRA_REQ_RCVD: >>>> @@ -3210,7 +3215,10 @@ int ib_send_cm_sidr_rep(struct ib_cm_id *cm_id, >>>> spin_unlock_irqrestore(&cm_id_priv->lock, flags); >>>> >>>> spin_lock_irqsave(&cm.lock, flags); >>>> - rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); >>>> + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) { >>>> + rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); >>>> + RB_CLEAR_NODE(&cm_id_priv->sidr_id_node); >>>> + } >> >> This change protects against double free >> >>>> spin_unlock_irqrestore(&cm.lock, flags); >>> >>> Something is very wrong in this function if the id is not in the tree >>> at this point. >>> >> >> We agree, but there's an error flow that triggers this behavior. > > Sean, I need to close on this patch. What is your position after > Matan's explanation? > Absent an objection from Sean, I've pulled this in. A use after free bug is a pretty serious issue, and you've listed an error flow that triggers it. The only thing bugging me is that this code is 10+ years old and this didn't show up until now, which makes me think that some recent change is the cause of this. I've made note of that fact in my tag commit and I think this warrants further examination in the next kernel cycle. But since we are so close to out of time on 4.3, I deemed it better to fix the use after free issue, even if it isn't necessarily the perfect fix, than leave that hanging about.
> > Sean, I need to close on this patch. What is your position after > > Matan's explanation? > > > > Absent an objection from Sean, I've pulled this in. A use after free > bug is a pretty serious issue, and you've listed an error flow that > triggers it. The only thing bugging me is that this code is 10+ years > old and this didn't show up until now, which makes me think that some > recent change is the cause of this. I've made note of that fact in my > tag commit and I think this warrants further examination in the next > kernel cycle. But since we are so close to out of time on 4.3, I deemed > it better to fix the use after free issue, even if it isn't necessarily > the perfect fix, than leave that hanging about. I was out last week. I think one of the reasons that this bug hasn't shown up is that very few apps use UD QPs, and those that do likely exchange QP information using some other out of band mechanism. - Sean -- 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/cm.c b/drivers/infiniband/core/cm.c index f5cf1c4..56ff0f3 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -844,6 +844,11 @@ retest: case IB_CM_SIDR_REQ_RCVD: spin_unlock_irq(&cm_id_priv->lock); cm_reject_sidr_req(cm_id_priv, IB_SIDR_REJECT); + spin_lock_irq(&cm.lock); + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) + rb_erase(&cm_id_priv->sidr_id_node, + &cm.remote_sidr_table); + spin_unlock_irq(&cm.lock); break; case IB_CM_REQ_SENT: case IB_CM_MRA_REQ_RCVD: @@ -3210,7 +3215,10 @@ int ib_send_cm_sidr_rep(struct ib_cm_id *cm_id, spin_unlock_irqrestore(&cm_id_priv->lock, flags); spin_lock_irqsave(&cm.lock, flags); - rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); + if (!RB_EMPTY_NODE(&cm_id_priv->sidr_id_node)) { + rb_erase(&cm_id_priv->sidr_id_node, &cm.remote_sidr_table); + RB_CLEAR_NODE(&cm_id_priv->sidr_id_node); + } spin_unlock_irqrestore(&cm.lock, flags); return 0;