Bug 8320
| Summary: | replacing route in kernel doesn't send netlink message | ||
|---|---|---|---|
| Product: | Networking | Reporter: | Milan Kocian (milan.kocian) |
| Component: | IPV4 | Assignee: | Stephen Hemminger (stephen) |
| Status: | CLOSED CODE_FIX | ||
| Severity: | low | CC: | bunk |
| Priority: | P2 | ||
| Hardware: | i386 | ||
| OS: | Linux | ||
| Kernel Version: | 2.6.20.6 | Subsystem: | |
| Regression: | --- | Bisected commit-id: | |
Reply-To: akpm@linux-foundation.org On Wed, 11 Apr 2007 02:37:01 -0700 bugme-daemon@bugzilla.kernel.org wrote: > > http://bugzilla.kernel.org/show_bug.cgi?id=8320 > > Summary: replacing route in kernel doesn't send netlink message > Kernel Version: 2.6.20.6 > Status: NEW > Severity: low > Owner: shemminger@osdl.org > Submitter: milan.kocian@wq.cz > > > Most recent kernel where this bug did *NOT* occur: I think all 2.6 kernels > Distribution: Debian (with vanilla kernel) > Hardware Environment: PC > Software Environment: Debian > Problem Description: > When you replace route (via ip r r ), no netlink message is sent. Or is it feature? > > Steps to reproduce: > 1. run 'ip monitor all' on one console > 2. do 'ip r r EXISTING_ROUTE via DST' on second console > 3. no message on console one > > Small patch for fib_hash (tested) but use carefully I am newbie :-) : > > --- fib_hash.c.old 2007-04-11 10:39:34.895667672 +0200 > +++ fib_hash.c 2007-04-11 10:41:34.623466280 +0200 > @@ -457,6 +457,8 @@ > fib_release_info(fi_drop); > if (state & FA_S_ACCESSED) > rt_cache_flush(-1); > + rtmsg_fib(RTM_NEWROUTE, key, fa, cfg->fc_dst_len, tb->tb_id, > + &cfg->fc_nlinfo); > return 0; > } > > And for fib_trie (not tested): > > --- fib_trie.c.old 2007-04-11 10:39:22.728517360 +0200 > +++ fib_trie.c 2007-04-11 10:40:40.778651936 +0200 > @@ -1205,6 +1205,8 @@ > fib_release_info(fi_drop); > if (state & FA_S_ACCESSED) > rt_cache_flush(-1); > + rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, tb->tb_id, > + &cfg->fc_nlinfo); > > goto succeeded; > } Thanks. We prefer to receive patches via email rather than via bugzilla. But that's a relatively minor matter - let's see what the net guys think about the change first ;) Andrew Morton wrote:
> On Wed, 11 Apr 2007 02:37:01 -0700 bugme-daemon@bugzilla.kernel.org wrote:
>
>
>>http://bugzilla.kernel.org/show_bug.cgi?id=8320
>>
>> Summary: replacing route in kernel doesn't send netlink message
>> Kernel Version: 2.6.20.6
>> Status: NEW
>> Severity: low
>> Owner: shemminger@osdl.org
>> Submitter: milan.kocian@wq.cz
>>
>>When you replace route (via ip r r ), no netlink message is sent. Or is it feature?
>>
>>Steps to reproduce:
>>1. run 'ip monitor all' on one console
>>2. do 'ip r r EXISTING_ROUTE via DST' on second console
>>3. no message on console one
>>
>>Small patch for fib_hash (tested) but use carefully I am newbie :-) :
>>
>>--- fib_hash.c.old 2007-04-11 10:39:34.895667672 +0200
>>+++ fib_hash.c 2007-04-11 10:41:34.623466280 +0200
>>@@ -457,6 +457,8 @@
>> fib_release_info(fi_drop);
>> if (state & FA_S_ACCESSED)
>> rt_cache_flush(-1);
>>+ rtmsg_fib(RTM_NEWROUTE, key, fa, cfg->fc_dst_len, tb->tb_id,
>>+ &cfg->fc_nlinfo);
>> return 0;
>> }
I think having notifications for this case makes sense (IIRC I used
to use a similar patch some time ago, but can't find it right now).
But we need to indicate somehow that it is a replacement and not a
completely new route, either by sending a RTM_DELROUTE for the old
route first (which would match what devinet does for addresses)
or by echoing the NLM_F_REPLACE flag. The former would probably be
easier for userspace to understand since it wouldn't need to
replicate the replacement logic just to find out which rule got
replaced.
Reply-To: milon@wq.cz On Wed, 2007-04-11 at 20:19 +0200, Patrick McHardy wrote: > > I think having notifications for this case makes sense (IIRC I used > to use a similar patch some time ago, but can't find it right now). > But we need to indicate somehow that it is a replacement and not a > completely new route, either by sending a RTM_DELROUTE for the old > route first (which would match what devinet does for addresses) > or by echoing the NLM_F_REPLACE flag. The former would probably be > easier for userspace to understand since it wouldn't need to > replicate the replacement logic just to find out which rule got > replaced. > > Hard to tell what is better. I slightly tried to test my patch with quagga routing daemon. And then I tested second case: send RTM_DELROUTE before RTM_NEWROUTE. Quagga updates internal rib in both cases (as I saw in debug logs). I was in fear that quagga will try to install sefl route but it doesn't catch. So from my point of view is all the same :). Milan Koci Reply-To: davem@davemloft.net From: Patrick McHardy <kaber@trash.net> Date: Mon, 16 Apr 2007 06:59:06 +0200 > RTM_DELROUTE + RTM_NEWROUTE seem to be safer, although you're correct > that it might cause userspace to perform some action upon receiving > the DELROUTE message since the update is non-atomic. So I really don't > know, I'm in favour of having notifications for replacements, but I > fear we might break something. We can cry foul about a broken application if an application following the API correctly would interpret the new messages correctly. I think it doesn't make sense to do a delete then a newroute for the atomicity issues, and therefore the replace makes the most sense as long as existing correct uses of the API would not explode on this. David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Mon, 16 Apr 2007 06:59:06 +0200
>
>
>>RTM_DELROUTE + RTM_NEWROUTE seem to be safer, although you're correct
>>that it might cause userspace to perform some action upon receiving
>>the DELROUTE message since the update is non-atomic. So I really don't
>>know, I'm in favour of having notifications for replacements, but I
>>fear we might break something.
>
>
> We can cry foul about a broken application if an application following
> the API correctly would interpret the new messages correctly.
>
> I think it doesn't make sense to do a delete then a newroute for
> the atomicity issues, and therefore the replace makes the most
> sense as long as existing correct uses of the API would not
> explode on this.
They shouldn't, worst case is that they ignore NLM_F_REPLACE and treat
it as a completely new route, which is at least half way correct and
not really worse than today.
Milan, could you cook up another patch which uses NLM_F_REPLACE?
Reply-To: milon@wq.cz On Tue, 2007-04-17 at 14:58 +0200, Patrick McHardy wrote: > David Miller wrote: > > From: Patrick McHardy <kaber@trash.net> > > Date: Mon, 16 Apr 2007 06:59:06 +0200 > > > > > >>RTM_DELROUTE + RTM_NEWROUTE seem to be safer, although you're correct > >>that it might cause userspace to perform some action upon receiving > >>the DELROUTE message since the update is non-atomic. So I really don't > >>know, I'm in favour of having notifications for replacements, but I > >>fear we might break something. > > > > > > We can cry foul about a broken application if an application following > > the API correctly would interpret the new messages correctly. > > > > I think it doesn't make sense to do a delete then a newroute for > > the atomicity issues, and therefore the replace makes the most > > sense as long as existing correct uses of the API would not > > explode on this. > > They shouldn't, worst case is that they ignore NLM_F_REPLACE and treat > it as a completely new route, which is at least half way correct and > not really worse than today. > > Milan, could you cook up another patch which uses NLM_F_REPLACE? > I can try it. Output is in patch below. Review carefully. I don't know if it's best approach. It's tested and working without problem (probably :-)) --- net/ipv4.old/fib_hash.c 2007-04-18 12:50:11.000000000 +0200 +++ net/ipv4/fib_hash.c 2007-04-18 12:39:49.081369320 +0200 @@ -443,7 +443,6 @@ if (cfg->fc_nlflags & NLM_F_REPLACE) { struct fib_info *fi_drop; u8 state; - write_lock_bh(&fib_hash_lock); fi_drop = fa->fa_info; fa->fa_info = fi; @@ -457,6 +456,8 @@ fib_release_info(fi_drop); if (state & FA_S_ACCESSED) rt_cache_flush(-1); + rtmsg_fib(RTM_NEWROUTE, key, fa, cfg->fc_dst_len, tb->tb_id, + &cfg->fc_nlinfo, NLM_F_REPLACE); return 0; } @@ -524,7 +525,7 @@ rt_cache_flush(-1); rtmsg_fib(RTM_NEWROUTE, key, new_fa, cfg->fc_dst_len, tb->tb_id, - &cfg->fc_nlinfo); + &cfg->fc_nlinfo, 0); return 0; out_free_new_fa: @@ -590,7 +591,7 @@ fa = fa_to_delete; rtmsg_fib(RTM_DELROUTE, key, fa, cfg->fc_dst_len, - tb->tb_id, &cfg->fc_nlinfo); + tb->tb_id, &cfg->fc_nlinfo, 0); kill_fn = 0; write_lock_bh(&fib_hash_lock); --- net/ipv4.old/fib_trie.c 2007-04-18 12:50:11.000000000 +0200 +++ net/ipv4/fib_trie.c 2007-04-18 12:42:29.423993536 +0200 @@ -1205,6 +1205,9 @@ fib_release_info(fi_drop); if (state & FA_S_ACCESSED) rt_cache_flush(-1); + rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, tb->tb_id, + &cfg->fc_nlinfo, NLM_F_REPLACE); + goto succeeded; } /* Error if we find a perfect match which @@ -1256,7 +1259,7 @@ rt_cache_flush(-1); rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, tb->tb_id, - &cfg->fc_nlinfo); + &cfg->fc_nlinfo, 0); succeeded: return 0; @@ -1599,7 +1602,7 @@ fa = fa_to_delete; rtmsg_fib(RTM_DELROUTE, htonl(key), fa, plen, tb->tb_id, - &cfg->fc_nlinfo); + &cfg->fc_nlinfo, 0); l = fib_find_node(t, key); li = find_leaf_info(l, plen); --- net/ipv4.old/fib_semantics.c 2007-04-18 12:50:11.000000000 +0200 +++ net/ipv4/fib_semantics.c 2007-04-18 12:40:54.807377448 +0200 @@ -301,7 +301,7 @@ } void rtmsg_fib(int event, __be32 key, struct fib_alias *fa, - int dst_len, u32 tb_id, struct nl_info *info) + int dst_len, u32 tb_id, struct nl_info *info, unsigned int nlm_flags) { struct sk_buff *skb; u32 seq = info->nlh ? info->nlh->nlmsg_seq : 0; @@ -313,7 +313,7 @@ err = fib_dump_info(skb, info->pid, seq, event, tb_id, fa->fa_type, fa->fa_scope, key, dst_len, - fa->fa_tos, fa->fa_info, 0); + fa->fa_tos, fa->fa_info, nlm_flags); /* failure implies BUG in fib_nlmsg_size() */ BUG_ON(err < 0); --- net/ipv4.old/fib_lookup.h 2007-04-18 12:50:11.000000000 +0200 +++ net/ipv4/fib_lookup.h 2007-04-18 12:43:42.377902856 +0200 @@ -30,7 +30,7 @@ int dst_len, u8 tos, struct fib_info *fi, unsigned int); extern void rtmsg_fib(int event, __be32 key, struct fib_alias *fa, - int dst_len, u32 tb_id, struct nl_info *info); + int dst_len, u32 tb_id, struct nl_info *info, unsigned int nlm_flags); extern struct fib_alias *fib_find_alias(struct list_head *fah, u8 tos, u32 prio); extern int fib_detect_death(struct fib_info *fi, int order, Milan Koci Patch was included in 2.6.22. |
Most recent kernel where this bug did *NOT* occur: I think all 2.6 kernels Distribution: Debian (with vanilla kernel) Hardware Environment: PC Software Environment: Debian Problem Description: When you replace route (via ip r r ), no netlink message is sent. Or is it feature? Steps to reproduce: 1. run 'ip monitor all' on one console 2. do 'ip r r EXISTING_ROUTE via DST' on second console 3. no message on console one Small patch for fib_hash (tested) but use carefully I am newbie :-) : --- fib_hash.c.old 2007-04-11 10:39:34.895667672 +0200 +++ fib_hash.c 2007-04-11 10:41:34.623466280 +0200 @@ -457,6 +457,8 @@ fib_release_info(fi_drop); if (state & FA_S_ACCESSED) rt_cache_flush(-1); + rtmsg_fib(RTM_NEWROUTE, key, fa, cfg->fc_dst_len, tb->tb_id, + &cfg->fc_nlinfo); return 0; } And for fib_trie (not tested): --- fib_trie.c.old 2007-04-11 10:39:22.728517360 +0200 +++ fib_trie.c 2007-04-11 10:40:40.778651936 +0200 @@ -1205,6 +1205,8 @@ fib_release_info(fi_drop); if (state & FA_S_ACCESSED) rt_cache_flush(-1); + rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, tb->tb_id, + &cfg->fc_nlinfo); goto succeeded; }