Discussion:
possible bug when deleting a route during if_detach
(too old to reply)
gmcnutt
2011-02-24 17:25:37 UTC
Permalink
if_detach() calls rt_walktree with if_rt_walktree as the visiting callback,
which calls rtrequest(RTM_DELETE, ...) from within the tree iteration in
rn_walktree. Although rn_walktree protects itself from deletion of the current
node while iterating, there is no protection if the *next* node is deleted
during the callback. This can happen if the called node is a route and there is
a clone of it following it in the iteration sequence. That is because
rtrequest1, in the RTM_DELETE case, will call rtflushclone, and this will
delete the cloned route. Upon return to rn_walktree, the next node may be
deleted, but rn_walktree will dereference it.

Here's the call sequence:
if_detach
`-rt_walktree
+-rn_walktree /* rt2 is next */
| `-rn_walktree_visitor(rt1)
| `-if_rt_detach(rt1)
| `-rtrequest1(rt1)
| +-rtflushclone(rt1)
| | `-rt_walktree(rt1)
| | `-rn_walktree_visitor(rt2)
| | `-rtflushclone1(rt2)
| | `-rtdeletemsg(rt2)
| | `-rtfree(rt2)
| `-rtfree(rt1)
`- /* crash on deref of rt2 (next) */

The cloned route in this case (labeled rt2 above) was created as a consequence
of if_arp.c setting the RTF_CLONING flag, presumably on rt1, but I haven't dug
into how route.c and radix.c determine their ordering yet to satisfy myself
that this is the case.

It seems the fundamental problem is that rn_walktree is defenseless against
this kind of radix tree mutation while it is iterating. Am I right, or is my
system somehow abusing some assumption about how those radix trees are supposed
to be ordered?



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
David Young
2011-02-24 17:43:33 UTC
Permalink
Post by gmcnutt
if_detach() calls rt_walktree with if_rt_walktree as the visiting callback,
which calls rtrequest(RTM_DELETE, ...) from within the tree iteration in
rn_walktree. Although rn_walktree protects itself from deletion of the current
node while iterating, there is no protection if the *next* node is deleted
during the callback. This can happen if the called node is a route and there is
a clone of it following it in the iteration sequence. That is because
rtrequest1, in the RTM_DELETE case, will call rtflushclone, and this will
delete the cloned route. Upon return to rn_walktree, the next node may be
deleted, but rn_walktree will dereference it.
I am pretty sure that this is fixed in -current.

Dave
--
David Young OJC Technologies
***@ojctech.com Urbana, IL * (217) 344-0444 x24

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Antti Kantee
2011-02-24 17:47:11 UTC
Permalink
Post by David Young
Post by gmcnutt
if_detach() calls rt_walktree with if_rt_walktree as the visiting callback,
which calls rtrequest(RTM_DELETE, ...) from within the tree iteration in
rn_walktree. Although rn_walktree protects itself from deletion of the current
node while iterating, there is no protection if the *next* node is deleted
during the callback. This can happen if the called node is a route and there is
a clone of it following it in the iteration sequence. That is because
rtrequest1, in the RTM_DELETE case, will call rtflushclone, and this will
delete the cloned route. Upon return to rn_walktree, the next node may be
deleted, but rn_walktree will dereference it.
I am pretty sure that this is fixed in -current.
Instead of speculation, why not write a test to make sure it is and
stays fixed?
--
älä karot toivorikkauttas, kyl rätei ja lumpui piisaa

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
David Young
2011-02-24 17:49:23 UTC
Permalink
Post by Antti Kantee
Post by David Young
I am pretty sure that this is fixed in -current.
Instead of speculation, why not write a test to make sure it is and
stays fixed?
I'd love to, but I don't know where to begin. Is there an existing test
that is a good starting point?

Dave
--
David Young OJC Technologies
***@ojctech.com Urbana, IL * (217) 344-0444 x24

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Antti Kantee
2011-02-24 17:53:41 UTC
Permalink
Post by David Young
Post by Antti Kantee
Post by David Young
I am pretty sure that this is fixed in -current.
Instead of speculation, why not write a test to make sure it is and
stays fixed?
I'd love to, but I don't know where to begin. Is there an existing test
that is a good starting point?
See the lone test in tests/net/route. Also, check tests/net/t_icmp2
for an example of interface creation/configuration.

If you run into an obstacle with the infrastructure, feel free to contact
me on or off list.
--
älä karot toivorikkauttas, kyl rätei ja lumpui piisaa

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Antti Kantee
2011-02-24 17:55:24 UTC
Permalink
Post by Antti Kantee
check tests/net/t_icmp2
make that tests/net/icmp/t_ping2
--
älä karot toivorikkauttas, kyl rätei ja lumpui piisaa

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
gmcnutt
2011-02-24 18:03:16 UTC
Permalink
Post by David Young
I am pretty sure that this is fixed in -current.
Dave
Indeed it is, and I overlooked it, thank you.




--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Loading...