Re: [PATCH] firewire-sbp2: fix panic after rmmod with slow targets

[Chris Boot <bootc@bootc.net>]

On 22/08/2011 15:35, Stefan Richter wrote:
> On Aug 22 Chris Boot wrote:
>> If firewire-sbp2 starts a login to a target that doesn't complete ORBs
>> in a timely manner (and has to retry the login), and the module is
>> removed before the operation times out, you end up with a null-pointer
>> dereference and a kernel panic.
>>
>> This happens because the code in sbp2_remove() just does a
>> sbp2_target_put(), assuming it will be the last remaining reference. If
>> there are jobs in the workqueue, this is not the case, and the module is
>> successfully unloaded while references still exist.
> The problem is not that sbp2_target_put()'s caller assumes that it is
> putting the last reference.  sbp2_target_put()'s very purpose is to clean
> up when, and only when, the last reference is gone.

Yes that's what I meant by my message, but I guess I wasn't clear 
enough. I edited it a bit with the second revision, I hope it's a bit 
clearer now.

[snip]

>> Signed-off-by: Chris Boot<bootc@bootc.net>
>> ---
>>   drivers/firewire/sbp2.c |    5 +++++
>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
>> index 41841a3..3867aaa 100644
>> --- a/drivers/firewire/sbp2.c
>> +++ b/drivers/firewire/sbp2.c
>> @@ -1198,6 +1198,11 @@ static int sbp2_remove(struct device *dev)
>>   {
>>   	struct fw_unit *unit = fw_unit(dev);
>>   	struct sbp2_target *tgt = dev_get_drvdata(&unit->device);
>> +	struct sbp2_logical_unit *lu, *next;
>> +
>> +	list_for_each_entry_safe(lu, next,&tgt->lu_list, link) {
>> +		cancel_delayed_work_sync(&lu->work);
>> +	}
>>
>>   	sbp2_target_put(tgt);
>>   	return 0;
> list_for_each_entry() is sufficient here.  You are not changing the list
> while you iterate over it.
>
> Would you please resend the patch with list_for_each_entry()?

Done.

> More importantly, I think the whole sbp2_target instance reference
> counting can be removed with his work canceling in place.  But I have not
> analyzed this fully yet, and I don't expect you to do this for me.  Though
> if you like to do so, that would of course be welcome.

If you don't mind I'll let you do that as I'm not yet familiar enough 
with the kernel - only enough to be dangerous! :-)

Chris