firewire-sbp2: fix panic after rmmod with slow targets
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · 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 asbp2_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.
The problem is that sbp2_target_get() does not take a reference to the module. scsi_device_get() does. But as long as SCSI Inquiry was not completed and e.g. somebody mounts a filesystem on an SBP-2 attached device, there is no module reference kept by the SCSI core, hence firewire-sbp2 can be unloaded.
Quoted 3 lines
This patch cancels pending work for each unit in
sbp2_remove(), which hopefully means there are no extra references around that prevent us from unloading. This fixes my crash.
The fix looks good to me. On thing to watch out for is that sbp2_remove() may be executed by the very same workqueue which also executes lu->work. (This is fw_workqueue = alloc_workqueue("firewire", WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0).) But AFAIU, the workqueue infrastructure is able to handle this without deadlock since kernel 2.6.36. Tejun, is this understanding correct?
Quoted 11 lines
Signed-off-by Chris Boot <[email protected]>--- drivers/firewire/sbp2.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)drivers/firewire/sbp2.c +0
-0@@ -1198,6 +1198,11 @@ static int sbp2_remove(struct device *dev) {
drivers/firewire/sbp2.c static int sbp2_remove(struct device *dev) · line 1208
Quoted 10 lines
drivers/firewire/sbp2.c +5
-0struct 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()?
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.
Signature · 3 lines
Stefan Richter -=====-==-== =--- =-==- http://arcgraph.de/sr/
PATCH 1/1 firewire-sbp2: fix panic after rmmod with slow targets
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 there are no remaining references to the target. If there are jobs in the workqueue, this is not the case, and the module is successfully unloaded while references still exist.
This patch cancels pending work for each unit in sbp2_remove(), which hopefully means there are no extra references around that prevent us from unloading. This fixes my crash.
--- drivers/firewire/sbp2.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/firewire/sbp2.c +5 -0
@@ -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 (lu, next, &tgt->lu_list, link) { +cancel_delayed_work_sync (&lu->work); + }sbp2_target_put (tgt);return 0 ;
Signature · 1 line
1.7.5.4
drivers/firewire/sbp2.c static int sbp2_remove(struct device *dev) · line 1203
Quoted 1 line · Chris Boot
drivers/firewire/sbp2.c +1
-0+list_for_each_entry (lu, next, &tgt->lu_list, link) {
That'll teach me for assuming the define had the same number of arguments, and not compile-testing at least... :-(
I'll send rev 3 over shortly.
Chris
PATCH v3 1/1 firewire-sbp2: fix panic after rmmod with slow targets
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 there are no remaining references to the target. If there are jobs in the workqueue, this is not the case, and the module is successfully unloaded while references still exist.
This patch cancels pending work for each unit in sbp2_remove(), which hopefully means there are no extra references around that prevent us from unloading. This fixes my crash.
--- drivers/firewire/sbp2.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/firewire/sbp2.c +5 -0
@@ -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; + +list_for_each_entry (lu, &tgt->lu_list, link) { +cancel_delayed_work_sync (&lu->work); + }sbp2_target_put (tgt);return 0 ;
Signature · 1 line
1.7.5.4
Quoted 4 lines · On Aug 22 Chris Boot wrote:
drivers/firewire/sbp2.c +0
-0@@ -1198,6 +1198,11 @@ static int sbp2_remove(struct device *dev) {
drivers/firewire/sbp2.c static int sbp2_remove(struct device *dev) · line 1208
Quoted 10 lines
drivers/firewire/sbp2.c +5
-0struct fw_unit *unit =fw_unit (dev);struct sbp2_target *tgt =dev_get_drvdata (&unit->device); +struct sbp2_logical_unit *lu; + +list_for_each_entry (lu, &tgt->lu_list, link) { +cancel_delayed_work_sync (&lu->work); + }sbp2_target_put (tgt);return 0 ;
Thanks. I reproduced the bug, applied your patch, and confirmed that the patch fixes the bug. Committed to linux1394-2.6.git but without the curly braces.
Signature · 3 lines
Stefan Richter -=====-==-== =--- =-=== http://arcgraph.de/sr/
Quoted 6 lines · Stefan Richter
Quoted 5 lines · 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.
Quoted 7 lines
Quoted 4 lines
This happens because the code in
sbp2_remove()just does asbp2_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]
Quoted 19 lines
Quoted 19 lines
Signed-off-by Chris Boot<[email protected]>--- drivers/firewire/sbp2.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)drivers/firewire/sbp2.c +5
-0@@ -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); + }
Quoted 6 lines
drivers/firewire/sbp2.c static int sbp2_remove(struct device *dev) · line 1208
Quoted 2 lines
drivers/firewire/sbp2.c +0
-0sbp2_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.
Quoted 4 lines
More importantly, I think the whole
sbp2_targetinstance 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