| Message ID | 20180905135347.138195-2-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | qedf: fixup NULL pointer reference in fc_lookup_rport() | expand |
On Wed, 2018-09-05 at 15:53 +0200, Hannes Reinecke wrote: > test_bit() is atomic, test_bit() || test_bit() is not. > So protect consecutive bit tests with a lock to avoid races. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/scsi/qedf/qedf_els.c | 9 +++++++-- > drivers/scsi/qedf/qedf_main.c | 5 ++++- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/qedf/qedf_els.c > b/drivers/scsi/qedf/qedf_els.c > index 04f0c4d2e256..acb8157a96a8 100644 > --- a/drivers/scsi/qedf/qedf_els.c > +++ b/drivers/scsi/qedf/qedf_els.c > @@ -335,20 +335,25 @@ void qedf_restart_rport(struct qedf_rport > *fcport) > struct fc_lport *lport; > struct fc_rport_priv *rdata; > u32 port_id; > + unsigned long flags; > > if (!fcport) > return; > > + spin_lock_irqsave(&fcport->rport_lock, flags); > if (test_bit(QEDF_RPORT_IN_RESET, &fcport->flags) || > !test_bit(QEDF_RPORT_SESSION_READY, &fcport->flags) || > test_bit(QEDF_RPORT_UPLOADING_CONNECTION, &fcport->flags)) Could you avoid the need for locking like this? unsigned long flgs = atomic_read(&fcport->flags); if (flgs & (QEDF_RPORT_IN_RESET|QEDF_RPORT_UPLOADING_CONNECTION) || !flgs & QEDF_RPORT_SESSION_READY) { Regards, Martin
On 09/05/2018 04:09 PM, Martin Wilck wrote: > On Wed, 2018-09-05 at 15:53 +0200, Hannes Reinecke wrote: >> test_bit() is atomic, test_bit() || test_bit() is not. >> So protect consecutive bit tests with a lock to avoid races. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/scsi/qedf/qedf_els.c | 9 +++++++-- >> drivers/scsi/qedf/qedf_main.c | 5 ++++- >> 2 files changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/scsi/qedf/qedf_els.c >> b/drivers/scsi/qedf/qedf_els.c >> index 04f0c4d2e256..acb8157a96a8 100644 >> --- a/drivers/scsi/qedf/qedf_els.c >> +++ b/drivers/scsi/qedf/qedf_els.c >> @@ -335,20 +335,25 @@ void qedf_restart_rport(struct qedf_rport >> *fcport) >> struct fc_lport *lport; >> struct fc_rport_priv *rdata; >> u32 port_id; >> + unsigned long flags; >> >> if (!fcport) >> return; >> >> + spin_lock_irqsave(&fcport->rport_lock, flags); >> if (test_bit(QEDF_RPORT_IN_RESET, &fcport->flags) || >> !test_bit(QEDF_RPORT_SESSION_READY, &fcport->flags) || >> test_bit(QEDF_RPORT_UPLOADING_CONNECTION, &fcport->flags)) > > Could you avoid the need for locking like this? > > unsigned long flgs = atomic_read(&fcport->flags); > if (flgs & (QEDF_RPORT_IN_RESET|QEDF_RPORT_UPLOADING_CONNECTION) || > !flgs & QEDF_RPORT_SESSION_READY) { > No. There still is a race condition between those two tests. QEDF_RPORT_UPLOADING_CONNECTION might be set just after the first test, rendering the entire statement invalid. Cheers, Hannes
On Thu, 2018-09-06 at 09:03 +0200, Hannes Reinecke wrote: > On 09/05/2018 04:09 PM, Martin Wilck wrote: > > On Wed, 2018-09-05 at 15:53 +0200, Hannes Reinecke wrote: > > > test_bit() is atomic, test_bit() || test_bit() is not. > > > So protect consecutive bit tests with a lock to avoid races. > > > > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > > --- > > > drivers/scsi/qedf/qedf_els.c | 9 +++++++-- > > > drivers/scsi/qedf/qedf_main.c | 5 ++++- > > > 2 files changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/scsi/qedf/qedf_els.c > > > b/drivers/scsi/qedf/qedf_els.c > > > index 04f0c4d2e256..acb8157a96a8 100644 > > > --- a/drivers/scsi/qedf/qedf_els.c > > > +++ b/drivers/scsi/qedf/qedf_els.c > > > @@ -335,20 +335,25 @@ void qedf_restart_rport(struct qedf_rport > > > *fcport) > > > struct fc_lport *lport; > > > struct fc_rport_priv *rdata; > > > u32 port_id; > > > + unsigned long flags; > > > > > > if (!fcport) > > > return; > > > > > > + spin_lock_irqsave(&fcport->rport_lock, flags); > > > if (test_bit(QEDF_RPORT_IN_RESET, &fcport->flags) || > > > !test_bit(QEDF_RPORT_SESSION_READY, &fcport->flags) || > > > test_bit(QEDF_RPORT_UPLOADING_CONNECTION, &fcport->flags)) > > > > Could you avoid the need for locking like this? > > > > unsigned long flgs = atomic_read(&fcport->flags); > > if (flgs > > & (QEDF_RPORT_IN_RESET|QEDF_RPORT_UPLOADING_CONNECTION) || > > !flgs & QEDF_RPORT_SESSION_READY) { > > > > No. > There still is a race condition between those two tests. > QEDF_RPORT_UPLOADING_CONNECTION might be set just after the first > test, > rendering the entire statement invalid. ... and you need to sync with the set_bit(QEDF_RPORT_IN_RESET) later on. I overlooked that, sorry. But then, there's at least one more place where QEDF_RPORT_SESSION_READY is cleared without taking the lock (qedf_cleanup_fcport()). Perhaps you need to protect that, too? Thanks, Martin
diff --git a/drivers/scsi/qedf/qedf_els.c b/drivers/scsi/qedf/qedf_els.c index 04f0c4d2e256..acb8157a96a8 100644 --- a/drivers/scsi/qedf/qedf_els.c +++ b/drivers/scsi/qedf/qedf_els.c @@ -335,20 +335,25 @@ void qedf_restart_rport(struct qedf_rport *fcport) struct fc_lport *lport; struct fc_rport_priv *rdata; u32 port_id; + unsigned long flags; if (!fcport) return; + spin_lock_irqsave(&fcport->rport_lock, flags); if (test_bit(QEDF_RPORT_IN_RESET, &fcport->flags) || !test_bit(QEDF_RPORT_SESSION_READY, &fcport->flags) || test_bit(QEDF_RPORT_UPLOADING_CONNECTION, &fcport->flags)) { - QEDF_ERR(&(fcport->qedf->dbg_ctx), "fcport %p already in reset or not offloaded.\n", - fcport); + QEDF_ERR(&(fcport->qedf->dbg_ctx), + "fcport %p already in reset or not offloaded.\n", + fcport); + spin_unlock_irqrestore(&fcport->rport_lock, flags); return; } /* Set that we are now in reset */ set_bit(QEDF_RPORT_IN_RESET, &fcport->flags); + spin_unlock_irqrestore(&fcport->rport_lock, flags); rdata = fcport->rdata; if (rdata) { diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c index 0a5dd5595dd3..dd64236ef4c4 100644 --- a/drivers/scsi/qedf/qedf_main.c +++ b/drivers/scsi/qedf/qedf_main.c @@ -1370,8 +1370,10 @@ static void qedf_rport_event_handler(struct fc_lport *lport, fcport = (struct qedf_rport *)&rp[1]; /* Only free this fcport if it is offloaded already */ + spin_lock_irqsave(&fcport->rport_lock, flags); if (test_bit(QEDF_RPORT_SESSION_READY, &fcport->flags)) { set_bit(QEDF_RPORT_UPLOADING_CONNECTION, &fcport->flags); + spin_unlock_irqrestore(&fcport->rport_lock, flags); qedf_cleanup_fcport(qedf, fcport); /* @@ -1385,7 +1387,8 @@ static void qedf_rport_event_handler(struct fc_lport *lport, clear_bit(QEDF_RPORT_UPLOADING_CONNECTION, &fcport->flags); atomic_dec(&qedf->num_offloads); - } + } else + spin_unlock_irqrestore(&fcport->rport_lock, flags); break;
test_bit() is atomic, test_bit() || test_bit() is not. So protect consecutive bit tests with a lock to avoid races. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/scsi/qedf/qedf_els.c | 9 +++++++-- drivers/scsi/qedf/qedf_main.c | 5 ++++- 2 files changed, 11 insertions(+), 3 deletions(-)