Re: [PATCH] libfc: unsafe refcounting in fc_rport_work()

[Hannes Reinecke <hare@suse.de>]

On 04/20/2016 09:03 PM, James Bottomley wrote:
> 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.
> 
Yes, I _did_ see this happen; a customer was complaining about a
soft lockup happening in fc_rport_timeout every 30 seconds.

> 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).
> 
Yes, you are right; turns out to be a problem with mutexes and krefs
in general (cf https://lkml.org/lkml/2015/2/11/245).

I'll be sending a new patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html