| Message ID | 20180502134249.20730.78919.stgit@scvm10.sc.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, May 02, 2018 at 06:42:51AM -0700, Dennis Dalessandro wrote: > From: Michael J. Ruhl <michael.j.ruhl@intel.com> > > A pio send egress error can occur when the PSM library attempts to > to send a bad packet. That issue is still being investigated. > > The pio error interrupt handler then attempts to progress the recovery > of the errored pio send context. > > Code inspection reveals that the handling lacks the necessary locking > if that recovery interleaves with a PSM close of the "context" object > contains the pio send context. > > The lack of the locking can cause the recovery to access the already > freed pio send context object and incorrectly deduce that the pio > send context is actually a kernel pio send context as shown by the > NULL deref stack below: > > [<ffffffff8143d78c>] _dev_info+0x6c/0x90 > [<ffffffffc0613230>] sc_restart+0x70/0x1f0 [hfi1] > [<ffffffff816ab124>] ? __schedule+0x424/0x9b0 > [<ffffffffc06133c5>] sc_halted+0x15/0x20 [hfi1] > [<ffffffff810aa3ba>] process_one_work+0x17a/0x440 > [<ffffffff810ab086>] worker_thread+0x126/0x3c0 > [<ffffffff810aaf60>] ? manage_workers.isra.24+0x2a0/0x2a0 > [<ffffffff810b252f>] kthread+0xcf/0xe0 > [<ffffffff810b2460>] ? insert_kthread_work+0x40/0x40 > [<ffffffff816b8798>] ret_from_fork+0x58/0x90 > [<ffffffff810b2460>] ? insert_kthread_work+0x40/0x40 > > This is the best case scenario and other scenarios can corrupt the > already freed memory. > > Fix by adding the necessary locking in the pio send context error > handler. > > Cc: <stable@vger.kernel.org> # 4.9.x > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > --- > drivers/infiniband/hw/hfi1/chip.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) Why are you sending this to for-next not for-rc? 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
On 5/4/2018 2:38 PM, Jason Gunthorpe wrote: > On Wed, May 02, 2018 at 06:42:51AM -0700, Dennis Dalessandro wrote: >> From: Michael J. Ruhl <michael.j.ruhl@intel.com> >> >> A pio send egress error can occur when the PSM library attempts to >> to send a bad packet. That issue is still being investigated. >> >> The pio error interrupt handler then attempts to progress the recovery >> of the errored pio send context. >> >> Code inspection reveals that the handling lacks the necessary locking >> if that recovery interleaves with a PSM close of the "context" object >> contains the pio send context. >> >> The lack of the locking can cause the recovery to access the already >> freed pio send context object and incorrectly deduce that the pio >> send context is actually a kernel pio send context as shown by the >> NULL deref stack below: >> >> [<ffffffff8143d78c>] _dev_info+0x6c/0x90 >> [<ffffffffc0613230>] sc_restart+0x70/0x1f0 [hfi1] >> [<ffffffff816ab124>] ? __schedule+0x424/0x9b0 >> [<ffffffffc06133c5>] sc_halted+0x15/0x20 [hfi1] >> [<ffffffff810aa3ba>] process_one_work+0x17a/0x440 >> [<ffffffff810ab086>] worker_thread+0x126/0x3c0 >> [<ffffffff810aaf60>] ? manage_workers.isra.24+0x2a0/0x2a0 >> [<ffffffff810b252f>] kthread+0xcf/0xe0 >> [<ffffffff810b2460>] ? insert_kthread_work+0x40/0x40 >> [<ffffffff816b8798>] ret_from_fork+0x58/0x90 >> [<ffffffff810b2460>] ? insert_kthread_work+0x40/0x40 >> >> This is the best case scenario and other scenarios can corrupt the >> already freed memory. >> >> Fix by adding the necessary locking in the pio send context error >> handler. >> >> Cc: <stable@vger.kernel.org> # 4.9.x >> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> >> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> >> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> >> --- >> drivers/infiniband/hw/hfi1/chip.c | 4 ++++ >> 1 files changed, 4 insertions(+), 0 deletions(-) > > Why are you sending this to for-next not for-rc? I went back and forth on this one. In the end decided against it because we've lived with it for so long, note stable tag is all the way back to 4.9, that and the fact that it's extremely unlikely to occur. I would be fine including it with the -rc though. I think a case could be made either way. -Denny -- 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 Fri, 2018-05-04 at 16:01 -0400, Dennis Dalessandro wrote: > On 5/4/2018 2:38 PM, Jason Gunthorpe wrote: > > On Wed, May 02, 2018 at 06:42:51AM -0700, Dennis Dalessandro wrote: > > > From: Michael J. Ruhl <michael.j.ruhl@intel.com> > > > > > > A pio send egress error can occur when the PSM library attempts to > > > to send a bad packet. That issue is still being investigated. > > > > > > The pio error interrupt handler then attempts to progress the recovery > > > of the errored pio send context. > > > > > > Code inspection reveals that the handling lacks the necessary locking > > > if that recovery interleaves with a PSM close of the "context" object > > > contains the pio send context. > > > > > > The lack of the locking can cause the recovery to access the already > > > freed pio send context object and incorrectly deduce that the pio > > > send context is actually a kernel pio send context as shown by the > > > NULL deref stack below: > > > > > > [<ffffffff8143d78c>] _dev_info+0x6c/0x90 > > > [<ffffffffc0613230>] sc_restart+0x70/0x1f0 [hfi1] > > > [<ffffffff816ab124>] ? __schedule+0x424/0x9b0 > > > [<ffffffffc06133c5>] sc_halted+0x15/0x20 [hfi1] > > > [<ffffffff810aa3ba>] process_one_work+0x17a/0x440 > > > [<ffffffff810ab086>] worker_thread+0x126/0x3c0 > > > [<ffffffff810aaf60>] ? manage_workers.isra.24+0x2a0/0x2a0 > > > [<ffffffff810b252f>] kthread+0xcf/0xe0 > > > [<ffffffff810b2460>] ? insert_kthread_work+0x40/0x40 > > > [<ffffffff816b8798>] ret_from_fork+0x58/0x90 > > > [<ffffffff810b2460>] ? insert_kthread_work+0x40/0x40 > > > > > > This is the best case scenario and other scenarios can corrupt the > > > already freed memory. > > > > > > Fix by adding the necessary locking in the pio send context error > > > handler. > > > > > > Cc: <stable@vger.kernel.org> # 4.9.x > > > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com> > > > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > > > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > > > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > > > --- > > > drivers/infiniband/hw/hfi1/chip.c | 4 ++++ > > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > Why are you sending this to for-next not for-rc? > > I went back and forth on this one. In the end decided against it because > we've lived with it for so long, note stable tag is all the way back to > 4.9, that and the fact that it's extremely unlikely to occur. I would be > fine including it with the -rc though. I think a case could be made > either way. > > -Denny > > > I went ahead and pulled this into for-rc instead of for-next. Thanks.
diff --git a/drivers/infiniband/hw/hfi1/chip.c b/drivers/infiniband/hw/hfi1/chip.c index cb9095d..e5254f8 100644 --- a/drivers/infiniband/hw/hfi1/chip.c +++ b/drivers/infiniband/hw/hfi1/chip.c @@ -5944,6 +5944,7 @@ static void is_sendctxt_err_int(struct hfi1_devdata *dd, u64 status; u32 sw_index; int i = 0; + unsigned long irq_flags; sw_index = dd->hw_to_sw[hw_context]; if (sw_index >= dd->num_send_contexts) { @@ -5953,10 +5954,12 @@ static void is_sendctxt_err_int(struct hfi1_devdata *dd, return; } sci = &dd->send_contexts[sw_index]; + spin_lock_irqsave(&dd->sc_lock, irq_flags); sc = sci->sc; if (!sc) { dd_dev_err(dd, "%s: context %u(%u): no sc?\n", __func__, sw_index, hw_context); + spin_unlock_irqrestore(&dd->sc_lock, irq_flags); return; } @@ -5978,6 +5981,7 @@ static void is_sendctxt_err_int(struct hfi1_devdata *dd, */ if (sc->type != SC_USER) queue_work(dd->pport->hfi1_wq, &sc->halt_work); + spin_unlock_irqrestore(&dd->sc_lock, irq_flags); /* * Update the counters for the corresponding status bits.