Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
Milan Kocián wrote: ok, here is new version. Sign is in patch. Is it correct? --- a/net/ipv4/fib_hash.c 2007-04-18 12:50:11.0 +0200 +++ b/net/ipv4/fib_hash.c 2007-04-19 10:21:04.267136960 +0200 [...] Signed-off-by: Milan Kocian [EMAIL PROTECTED] Looks good, thanks. Acked-by: Patrick McHardy [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
On Thu, 19 Apr 2007 14:12:19 +0200 Patrick McHardy [EMAIL PROTECTED] wrote: Milan Kocián wrote: ok, here is new version. Sign is in patch. Is it correct? --- a/net/ipv4/fib_hash.c 2007-04-18 12:50:11.0 +0200 +++ b/net/ipv4/fib_hash.c 2007-04-19 10:21:04.267136960 +0200 [...] Signed-off-by: Milan Kocian [EMAIL PROTECTED] Looks good, thanks. Acked-by: Patrick McHardy [EMAIL PROTECTED] Can we please have a final version of this with appropriate Subject: and complete changelogging? Thanks. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
On Tue, 2007-04-17 at 14:58 +0200, Patrick McHardy wrote:
David Miller wrote:
From: Patrick McHardy [EMAIL PROTECTED]
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.0 +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.0 +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.c2007-04-18 12:50:11.0 +0200
+++ net/ipv4/fib_semantics.c2007-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.0 +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,
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
Milan Kocián wrote:
On Tue, 2007-04-17 at 14:58 +0200, Patrick McHardy wrote:
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 :-))
Looks good, but your mailer corrupted long lines. Please resend as
attachment and sign off the patch.
--- net/ipv4.old/fib_hash.c 2007-04-18 12:50:11.0 +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;
-
And please drop this unrelated whitespace change.
--- net/ipv4.old/fib_semantics.c 2007-04-18 12:50:11.0 +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)
This should go on a new line since it exceeds 80 characters.
--- net/ipv4.old/fib_lookup.h 2007-04-18 12:50:11.0 +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);
Same here.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
David Miller wrote: From: Patrick McHardy [EMAIL PROTECTED] 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? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
From: Patrick McHardy [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
Milan Kocián wrote: 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 :). Looking at some old code of mine, it would treat a simple RTM_NEWROUTE without deletion in advance incorrectly, but it also would ignore NLM_F_REPLACE. Quagga doing the right thing seems to be a result of the fact that it doesn't care about some of the routes attributes and treats NEWROUTE messages as replacements as long as the attributes it cares about match. 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
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án [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
On Wed, 11 Apr 2007 02:37:01 -0700 [EMAIL PROTECTED] 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: [EMAIL PROTECTED] Submitter: [EMAIL PROTECTED] 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 ;) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 8320] New: replacing route in kernel doesn't send netlink message
Andrew Morton wrote: On Wed, 11 Apr 2007 02:37:01 -0700 [EMAIL PROTECTED] 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: [EMAIL PROTECTED] Submitter: [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
