On Wed, 2016-04-20 at 15:24 +0200, Hannes Reinecke wrote:
> When pushing items on a workqueue we cannot take reference
> when the workqueue item is executed, as the structure might
> already been freed at that time.
> So instead we need to take a reference before adding it
> to the workqueue, thereby ensuring that the workqueue item
> will always be valid.
Have you actually seen this happen? The rdata structure is fully ref
counted, so if it's done a final put, then something should see
unreferenced memory. It looks like the model is that the final put is
done from the queue, so I don't quite see how you can lose the final
reference in either of the places you alter.
Plus, kref_get_unless_zero() should not be used. At that point, the
structure would be freed, so there's no point looking for it.
kref_get_unless_zero is for refcounts that don't necessarily free the
structure (embedded ones).
James
> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> ---
> drivers/scsi/libfc/fc_rport.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/libfc/fc_rport.c
> b/drivers/scsi/libfc/fc_rport.c
> index 589ff9a..8b08263f 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -263,7 +263,6 @@ static void fc_rport_work(struct work_struct
> *work)
> ids = rdata->ids;
> rdata->event = RPORT_EV_NONE;
> rdata->major_retries = 0;
> - kref_get(&rdata->kref);
> mutex_unlock(&rdata->rp_mutex);
>
> if (!rport)
> @@ -297,7 +296,6 @@ static void fc_rport_work(struct work_struct
> *work)
> FC_RPORT_DBG(rdata, "lld callback ev %d\n",
> event);
> rdata->lld_event_callback(lport, rdata,
> event);
> }
> - kref_put(&rdata->kref, lport->tt.rport_destroy);
> break;
>
> case RPORT_EV_FAILED:
> @@ -377,6 +375,7 @@ static void fc_rport_work(struct work_struct
> *work)
> mutex_unlock(&rdata->rp_mutex);
> break;
> }
> + kref_put(&rdata->kref, lport->tt.rport_destroy);
> }
>
> /**
> @@ -438,8 +437,15 @@ static void fc_rport_enter_delete(struct
> fc_rport_priv *rdata,
>
> fc_rport_state_enter(rdata, RPORT_ST_DELETE);
>
> - if (rdata->event == RPORT_EV_NONE)
> - queue_work(rport_event_queue, &rdata->event_work);
> + if (rdata->event == RPORT_EV_NONE) {
> + if (!kref_get_unless_zero(&rdata->kref)) {
> + FC_RPORT_DBG(rdata, "port already
> deleted\n");
> + return;
> + }
> + if (!queue_work(rport_event_queue, &rdata
> ->event_work))
> + kref_put(&rdata->kref,
> + rdata->local_port
> ->tt.rport_destroy);
> + }
> rdata->event = event;
> }
>
> @@ -487,8 +493,15 @@ static void fc_rport_enter_ready(struct
> fc_rport_priv *rdata)
>
> FC_RPORT_DBG(rdata, "Port is Ready\n");
>
> - if (rdata->event == RPORT_EV_NONE)
> - queue_work(rport_event_queue, &rdata->event_work);
> + if (rdata->event == RPORT_EV_NONE) {
> + if (!kref_get_unless_zero(&rdata->kref)) {
> + FC_RPORT_DBG(rdata, "port already
> deleted\n");
> + return;
> + }
> + if (!queue_work(rport_event_queue, &rdata
> ->event_work))
> + kref_put(&rdata->kref,
> + rdata->local_port
> ->tt.rport_destroy);
> + }
> rdata->event = RPORT_EV_READY;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
