Actually, even the original code looks good, and if that panic'd, the
following won't change
that (verified it also).
When unregister_netdev() is called by the driver, it first calls
unregister_netdevice() which
drops it's last ref to the dev, making it zero. unregister_netdev() then
calls rtnl_unlock() which
calls netdev_run_todo(), which calls netdev_wait_allrefs() and only after
that succeeds,
does the driver do a free_netdev(). So the dev should not be freed while
the wait_ref() is
executing, and the original code looks correct.
I don't know if it is some corruption on my system, some hardware problem ?
I will look
some more, also try to get a different machine.
- KK
|---------+---------------------------->
| | Stephen Hemminger|
| | <shemminger@osdl.|
| | org> |
| | Sent by: |
| | netdev-bounce@oss|
| | .sgi.com |
| | |
| | |
| | 11/05/2003 05:09 |
| | PM |
| | |
|---------+---------------------------->
>-----------------------------------------------------------------------------------------------------------------|
| |
| To: Krishna Kumar/Beaverton/IBM@IBMUS |
| cc: davem@redhat.com, krkumar@us.ltcfwd.linux.ibm.com, netdev@oss.sgi.com |
| Subject: Re: [PATCH] panic during unregister_netdevice() |
| |
>-----------------------------------------------------------------------------------------------------------------|
Try this. Instead of dropping the last reference in unregister, it does it
after all other references are gone (sort of like the old 2.4 code).
diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c Wed Nov 5 17:04:57 2003+++ b/net/core/dev.c Wed Nov 5 17:04:57 2003
From: David S. Miller <hidden> Date: 2003-11-06 19:59:35
On Thu, 6 Nov 2003 11:58:24 -0800
Krishna Kumar [off-list ref] wrote:
When unregister_netdev() is called by the driver, it first calls
unregister_netdevice() which
drops it's last ref to the dev, making it zero. unregister_netdev() then
calls rtnl_unlock() which
calls netdev_run_todo(), which calls netdev_wait_allrefs() and only after
that succeeds,
does the driver do a free_netdev(). So the dev should not be freed while
the wait_ref() is
executing, and the original code looks correct.
That's correct.
I don't know if it is some corruption on my system, some hardware problem ?
I will look
some more, also try to get a different machine.
It could be some 'user after free' or similar issue.
Just an idea of something else to look for.
My earlier comments about "putting to zero multiple times" were
misguided, I forgot that these days dev_put() just decrements the
count and does not do anything special when the count reaches zero.
I think I found the problem in the link event code. linkwatch_event()
calls rtnl_unlock() when it gets an event (UNREGISTER) for the device
going down. But this gets called before the device unregister gets called,
via the rmmod/pci_device_remove), and frees up the dev. Later the device
unregister code panics the system.
I also noticed that this panic happens for e100 but not for the 3com
driver. 3com doesn't generate events for up/down using the linkwatch.
I tested with the following patch, and the panic disappeared (the
device shutdown properly). Dave, any need for rtnl_unlock() in this
code ?
Thanks,
- KK
diff -ruN linux-2.6.0-test9-bk9/net/core/link_watch.c linux-2.6.0-test9-bk9.new/net/core/link_watch.c
On Thu, 6 Nov 2003 11:58:24 -0800
Krishna Kumar [off-list ref] wrote:
quoted
When unregister_netdev() is called by the driver, it first calls
unregister_netdevice() which
drops it's last ref to the dev, making it zero. unregister_netdev() then
calls rtnl_unlock() which
calls netdev_run_todo(), which calls netdev_wait_allrefs() and only after
that succeeds,
does the driver do a free_netdev(). So the dev should not be freed while
the wait_ref() is
executing, and the original code looks correct.
That's correct.
quoted
I don't know if it is some corruption on my system, some hardware problem ?
I will look
some more, also try to get a different machine.
It could be some 'user after free' or similar issue.
Just an idea of something else to look for.
My earlier comments about "putting to zero multiple times" were
misguided, I forgot that these days dev_put() just decrements the
count and does not do anything special when the count reaches zero.
While using PRIVACY extensions, I sometimes get a hang when I remove the
interface. But I can reproduce this every time using the test script at
the end of the mail (hang depends on the order of address deletion).
The bug is in ipv6_del_addr() where if a temp address is being deleted, it
does an __in6_ifa_put() of the main address from which it was derived
(basically the autoconf prefix address). So if the main address was
deleted first, it's ifp ref count would be 1 and it would 'wait' to be
freed till it's temp address was freed first. When the temp address is
deleted, the __put() routine drops the main address's ifp ref count to 0,
but not free it. unregister_netdevice() hangs giving message that ref
count is 1. Fix tested overnight.
Also, the code at the top of the routine is unnecessary, the same is being
done when the address is found a little later in that routine.
Thanks,
- KK
-------------------------- PATCH -----------------------------------------
diff -ruN linux-2.6.0-test9-bk9/net/ipv6/addrconf.c linux-2.6.0-test9-bk9.new/net/ipv6/addrconf.c
From: David S. Miller <hidden> Date: 2003-11-09 06:30:49
On Fri, 7 Nov 2003 11:01:41 -0800 (PST)
Krishna Kumar [off-list ref] wrote:
While using PRIVACY extensions, I sometimes get a hang when I remove the
interface. But I can reproduce this every time using the test script at
the end of the mail (hang depends on the order of address deletion).
...
The bug is in ipv6_del_addr() where if a temp address is being deleted
...
Also, the code at the top of the routine is unnecessary, the same is being
done when the address is found a little later in that routine.
Hi Dave,
The following look to be bugs in xfrm related code.
1. In xfrm_lookup, a couple of bugs :
- the found or allocated xfrm_states are not passed correctly to
xfrm_bundle_create (and to the subsequent frees in case of create
failing) if the first xfrm_tmpl_resolve failed and the second one
succeeded.
- error handling is wrong.
2. In pfkey_get(), the xfrm_state is dereferenced after it is dropped,
which could lead to dereferencing freed memory.
3. ipcomp_tunnel_create and xfrm_state_construct don't set x->km.state
to XFRM_STATE_DEAD. This can lead to the BUG_TRAP in __xfrm_state_destroy
when xfrm_state_put() finds this is the last reference. This was reported
as one of the problems of [Bug 1754] some time back.
4. I am not sure of this one. I think dst_free() cannot be used when
a bundle of dst's are allocated and have to be freed. We need a new
dst_bundle_free() routine to free all linked dst's ?? Change is in
__xfrm[46]_bundle_create & xfrm_lookup(), the lookup one I am not very
sure. Why are we doing a dst_put(), shouldn't this be a free of all
dst's off the dst->child list since the routine marking 'dead' cleared all
entries off the dst->next list ?
These changes compile cleanly, but I couldn't test since these are
corner cases. Please let me know if this can be applied. I am sending
as one patch file for now instead of multiple files as they all small.
Thanks,
- KK
diff -ruN linux-2.6.0-rc2-bk6.org/include/net/dst.h linux-2.6.0-rc2-bk6/include/net/dst.h
@@ -694,6 +694,16 @@staticintstale_bundle(structdst_entry*dst);+voiddst_bundle_free(structdst_entry*dst)+{+structdst_entry*next;++while(dst){+next=dst->child;+dst_free(dst);+}+}+/* Main function: finds/creates a bundle for given flow.**AtthemomentweeatarawIProute.Mostlytospeeduplookups
@@ -799,9 +809,16 @@gotorestart;}}-if(err)+if(err<0)gotoerror;-}elseif(nx==0){+/*+*Savenumberofxfrm_state'sfound/createdforboth+*thenx==0checkbelowaswellastopassthe+*rightvaluetoxfrm_bundle_create().+*/+nx=err;+}+if(nx==0){/* Flow passes not transformed. */xfrm_pol_put(policy);return0;
@@ -827,8 +844,8 @@write_unlock_bh(&policy->lock);xfrm_pol_put(policy);-if(dst)-dst_free(dst);+if(dst)/* 'dead' freed dst->next list only */+dst_bundle_free(dst);gotorestart;}dst->next=policy->bundles;
From: David S. Miller <hidden> Date: 2004-01-10 04:48:08
On Fri, 9 Jan 2004 17:40:03 -0800 (PST)
Krishna Kumar [off-list ref] wrote:
These changes compile cleanly, but I couldn't test since these are
corner cases. Please let me know if this can be applied. I am sending
as one patch file for now instead of multiple files as they all small.
Maybe you should actually try to test these changes before I think
about applying them, for example:
quoted hunk
+void dst_bundle_free(struct dst_entry *dst)+{+ struct dst_entry *next;++ while (dst) {+ next = dst->child;+ dst_free(dst);+ }+}
Explain to me how that won't loop forever if given a non-NULL dst?
Next, this dst_bundle_free() thing is totally not needed as far as I can
tell. When dst_free() is made, the top-level of the bundle's dst gets added
to the garbage collection list, the garbage collection properly walks the
children to process the whole bundle.
Please redo this patch and please test it this time :)