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; }
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,
Patch was included in 2.6.22.