From: Jiri Bohac <jbohac@suse•cz>
To: David Miller <davem@davemloft•net>
Cc: jbohac@suse•cz, herbert@gondor•apana.org.au,
yoshfuji@linux-ipv6•org, netdev@vger•kernel.org,
shemminger@vyatta•com
Subject: Re: IPv6: race condition in __ipv6_ifa_notify() and dst_free() ?
Date: Tue, 27 Apr 2010 17:50:34 +0200 [thread overview]
Message-ID: <20100427155034.GA11157@midget.suse.cz> (raw)
In-Reply-To: <20100422.185400.71096585.davem@davemloft.net>
On Thu, Apr 22, 2010 at 06:54:00PM -0700, David Miller wrote:
> From: Jiri Bohac <jbohac@suse•cz>
> Date: Thu, 22 Apr 2010 17:49:08 +0200
>
> > I still don't see why __ipv6_ifa_notify() needs to call
> > dst_free(). Shouldn't that be dst_release() instead, to drop the
> > reference obtained by dst_hold(&ifp->rt->u.dst)?
>
> It likely wants to do both.
>
> Just doing dst_release() doesn't mark the 'dst' object as obsolete,
> and therefore it won't get force garbage collected.
Sure. So If I understand it correctly, there are two problems:
- the reference taken by dst_hold() just above the ip6_del_rt()
is never dropped if ip6_del_rt() fails; so shouldn't the code
be like this?:
dst_hold(&ifp->rt->u.dst);
if (ip6_del_rt(ifp->rt)) {
dst_release(&ifp->rt->u.dst);
dst_free(&ifp->rt->u.dst);
}
- if ip6_del_rt() fails because it races with something else
deleting the address, dst_free() will be called twice. This is
what Herbert is fixing with additional locking. However -- even
when he fixes that -- how can ip6_del_rt() fail with the
ifp->rt still needing a dst_free()?
AFAICS, it can fail by:
- __ip6_del_rt() finding that (rt ==
net->ipv6.ip6_null_entry); we don't want to call
dst_free() on net->ipv6.ip6_null_entry, do we?
- fib6_del() returning -ENOENT for multiple reasons;
but doesn't that mean that something else has removed
the route already and called dst_free on it?
In either case, the dst_free() looks like not being needed. And it only
does no harm in most cases, because these events are rare and
it usually finds (obsolete > 2) and does nothing.
I think that what Herbert is doing is only going to enforce that
the ip6_del_rt() is never going to fail, so the dst_free()
won't ever be called anyway, right?
Thanks,
--
Jiri Bohac <jbohac@suse•cz>
SUSE Labs, SUSE CZ
next prev parent reply other threads:[~2010-04-27 15:50 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-04-20 17:44 IPv6: race condition in __ipv6_ifa_notify() and dst_free() ? Jiri Bohac
2010-04-20 17:57 ` Eric Dumazet
2010-04-20 20:49 ` Jiri Bohac
2010-04-20 20:57 ` Eric Dumazet
2010-04-20 21:16 ` Stephen Hemminger
2010-04-20 21:35 ` Jiri Bohac
2010-04-21 21:34 ` Jiri Bohac
2010-04-22 2:32 ` Herbert Xu
2010-04-22 7:43 ` David Miller
2010-04-22 14:25 ` Herbert Xu
2010-04-22 15:49 ` Jiri Bohac
2010-04-22 16:17 ` Stephen Hemminger
2010-04-23 1:54 ` David Miller
2010-04-23 2:10 ` Herbert Xu
2010-04-23 15:05 ` Herbert Xu
2010-05-18 11:02 ` [0/4] Fix addrconf race conditions Herbert Xu
2010-05-18 11:04 ` [PATCH 1/4] ipv6: Replace inet6_ifaddr->dead with state Herbert Xu
2010-05-18 17:23 ` Stephen Hemminger
2010-05-18 22:27 ` David Miller
2010-05-18 11:04 ` [PATCH 2/4] ipv6: Use state_lock to protect ifa state Herbert Xu
2010-05-18 11:04 ` [PATCH 3/4] ipv6: Use POSTDAD state Herbert Xu
2010-05-18 11:04 ` [PATCH 4/4] ipv6: Never schedule DAD timer on dead address Herbert Xu
2010-05-18 17:25 ` [0/4] Fix addrconf race conditions Stephen Hemminger
2010-05-18 22:27 ` David Miller
2010-05-18 22:35 ` Herbert Xu
2010-04-27 15:50 ` Jiri Bohac [this message]
2010-04-27 15:55 ` IPv6: race condition in __ipv6_ifa_notify() and dst_free() ? Herbert Xu
2010-05-09 6:48 ` Herbert Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100427155034.GA11157@midget.suse.cz \
--to=jbohac@suse$(echo .)cz \
--cc=davem@davemloft$(echo .)net \
--cc=herbert@gondor$(echo .)apana.org.au \
--cc=netdev@vger$(echo .)kernel.org \
--cc=shemminger@vyatta$(echo .)com \
--cc=yoshfuji@linux-ipv6$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox