| Message ID | 20190416121310.20783-1-leon@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [rdma-next] RDMA/rdmavt: Catch use-after-free access of AH structures | expand |
On 4/16/2019 8:13 AM, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > Prior to commit d345691471b4 ("RDMA: Handle AH allocations by IB/core"), > AH destroy path is rdmavt returned -EBUSY warning to application and > caused to potential leakage of kernel memory of AH structure. > > After that commit, the AH structure is always freed but such early > return in driver code can potentially cause to use-after-free error. > > Add warning to catch such situation to help driver developers to fix > AH release path. > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/sw/rdmavt/ah.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/infiniband/sw/rdmavt/ah.c b/drivers/infiniband/sw/rdmavt/ah.c > index e6f7e4689d4d..0e147b32cbe9 100644 > --- a/drivers/infiniband/sw/rdmavt/ah.c > +++ b/drivers/infiniband/sw/rdmavt/ah.c > @@ -141,8 +141,7 @@ void rvt_destroy_ah(struct ib_ah *ibah, u32 destroy_flags) > struct rvt_ah *ah = ibah_to_rvtah(ibah); > unsigned long flags; > > - if (atomic_read(&ah->refcount) != 0) > - return; > + WARN_ON_ONCE(atomic_read(&ah->refcount)); > > spin_lock_irqsave(&dev->n_ahs_lock, flags); > dev->n_ahs_allocated--; We already know of this and are preparing a patch. I don't have a time estimate but would surely expect it in time for the merge window. -Denny
On Tue, Apr 16, 2019 at 09:40:21AM -0400, Dennis Dalessandro wrote: > On 4/16/2019 8:13 AM, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@mellanox.com> > > > > Prior to commit d345691471b4 ("RDMA: Handle AH allocations by IB/core"), > > AH destroy path is rdmavt returned -EBUSY warning to application and > > caused to potential leakage of kernel memory of AH structure. > > > > After that commit, the AH structure is always freed but such early > > return in driver code can potentially cause to use-after-free error. > > > > Add warning to catch such situation to help driver developers to fix > > AH release path. > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > drivers/infiniband/sw/rdmavt/ah.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/infiniband/sw/rdmavt/ah.c b/drivers/infiniband/sw/rdmavt/ah.c > > index e6f7e4689d4d..0e147b32cbe9 100644 > > +++ b/drivers/infiniband/sw/rdmavt/ah.c > > @@ -141,8 +141,7 @@ void rvt_destroy_ah(struct ib_ah *ibah, u32 destroy_flags) > > struct rvt_ah *ah = ibah_to_rvtah(ibah); > > unsigned long flags; > > > > - if (atomic_read(&ah->refcount) != 0) > > - return; > > + WARN_ON_ONCE(atomic_read(&ah->refcount)); > > > > spin_lock_irqsave(&dev->n_ahs_lock, flags); > > dev->n_ahs_allocated--; > > We already know of this and are preparing a patch. I don't have a time > estimate but would surely expect it in time for the merge window. Lets leave this in patchworks then, if we get to rc7 without a proper fix I'll merge this From what I can tell the driver was broken before too, returning -EBUSY is just a memory leaking bug though Jason
On Tue, Apr 16, 2019 at 03:13:10PM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > Prior to commit d345691471b4 ("RDMA: Handle AH allocations by IB/core"), > AH destroy path is rdmavt returned -EBUSY warning to application and > caused to potential leakage of kernel memory of AH structure. > > After that commit, the AH structure is always freed but such early > return in driver code can potentially cause to use-after-free error. > > Add warning to catch such situation to help driver developers to fix > AH release path. > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/sw/rdmavt/ah.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Applied to for-next Denny, since you missed the merge window with the fix, please send a fixup next cycle. The WARN_ON will scare people who might be able to hit this buggy case. Jason
On 5/6/2019 11:51 AM, Jason Gunthorpe wrote: > On Tue, Apr 16, 2019 at 03:13:10PM +0300, Leon Romanovsky wrote: >> From: Leon Romanovsky <leonro@mellanox.com> >> >> Prior to commit d345691471b4 ("RDMA: Handle AH allocations by IB/core"), >> AH destroy path is rdmavt returned -EBUSY warning to application and >> caused to potential leakage of kernel memory of AH structure. >> >> After that commit, the AH structure is always freed but such early >> return in driver code can potentially cause to use-after-free error. >> >> Add warning to catch such situation to help driver developers to fix >> AH release path. >> >> Signed-off-by: Leon Romanovsky <leonro@mellanox.com> >> --- >> drivers/infiniband/sw/rdmavt/ah.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) > > Applied to for-next > > Denny, since you missed the merge window with the fix, please send a > fixup next cycle. The WARN_ON will scare people who might be able to > hit this buggy case. Sounds reasonable to me. -Denny
diff --git a/drivers/infiniband/sw/rdmavt/ah.c b/drivers/infiniband/sw/rdmavt/ah.c index e6f7e4689d4d..0e147b32cbe9 100644 --- a/drivers/infiniband/sw/rdmavt/ah.c +++ b/drivers/infiniband/sw/rdmavt/ah.c @@ -141,8 +141,7 @@ void rvt_destroy_ah(struct ib_ah *ibah, u32 destroy_flags) struct rvt_ah *ah = ibah_to_rvtah(ibah); unsigned long flags; - if (atomic_read(&ah->refcount) != 0) - return; + WARN_ON_ONCE(atomic_read(&ah->refcount)); spin_lock_irqsave(&dev->n_ahs_lock, flags); dev->n_ahs_allocated--;