| Message ID | 20181009063000.46117-1-hare@suse.de (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | lpfc: fixup crash in lpfc_els_unsol_buffer() | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On Tue, 2018-10-09 at 08:30 +0200, Hannes Reinecke wrote: > lpfc_nlp_get() in lpfc_els_unsol_buffer() is not running under a > lock, so there is a chance that it might actually fail. But as we > never check the return value we'll get a crash in lpfc_nlp_put() > later on trying to free an invalid buffer. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/scsi/lpfc/lpfc_els.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c > index f1c1faa74b46..ef6aaa2609ae 100644 > --- a/drivers/scsi/lpfc/lpfc_els.c > +++ b/drivers/scsi/lpfc/lpfc_els.c > @@ -7930,6 +7930,9 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > elsiocb->context1 = lpfc_nlp_get(ndlp); > elsiocb->vport = vport; > > + if (!elsiocb->context1) > + goto dropit; > + > if ((cmd & ELS_CMD_MASK) == ELS_CMD_RSCN) { > cmd &= ELS_CMD_MASK; > } Well, that's fine. But if lpfc_nlp_get() can return NULL, because the ndlp structure could be going away, perhaps we should be looking more closely at the code in lpfc_nlp_get() that uses the structure a bunch of times, including dereferencing ndlp->phba and taking the phba->nlp_lock before taking a kref on it? (As well as the debugging log calls...) -Ewan
Ewan, >> + if (!elsiocb->context1) >> + goto dropit; >> + >> if ((cmd & ELS_CMD_MASK) == ELS_CMD_RSCN) { >> cmd &= ELS_CMD_MASK; >> } > > Well, that's fine. But if lpfc_nlp_get() can return NULL, because the > ndlp structure could be going away, perhaps we should be looking more > closely at the code in lpfc_nlp_get() that uses the structure a bunch of > times, including dereferencing ndlp->phba and taking the phba->nlp_lock > before taking a kref on it? (As well as the debugging log calls...) Hannes? James?
diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c index f1c1faa74b46..ef6aaa2609ae 100644 --- a/drivers/scsi/lpfc/lpfc_els.c +++ b/drivers/scsi/lpfc/lpfc_els.c @@ -7930,6 +7930,9 @@ lpfc_els_unsol_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, elsiocb->context1 = lpfc_nlp_get(ndlp); elsiocb->vport = vport; + if (!elsiocb->context1) + goto dropit; + if ((cmd & ELS_CMD_MASK) == ELS_CMD_RSCN) { cmd &= ELS_CMD_MASK; }
lpfc_nlp_get() in lpfc_els_unsol_buffer() is not running under a lock, so there is a chance that it might actually fail. But as we never check the return value we'll get a crash in lpfc_nlp_put() later on trying to free an invalid buffer. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/scsi/lpfc/lpfc_els.c | 3 +++ 1 file changed, 3 insertions(+)