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

[Chris Boot]

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@xxxxxxxxx>
> > ---
> >   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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/