Skip to contentSkip to conversation

rtnetlink & address family problem

PATCH In review Michal Ludvig 4 participants 12 messages Dec 3, 2004 – Dec 7, 2004

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Michal Ludvig

author
#0 lore raw

PATCH 1/1 rtnetlink & address family problem

Hi,

running 'ip -6 addr flush dev eth0' on a kernel without IPv6 support flushes *all* addresses from the interface, even those IPv4 ones, because the unsupported protocol is substituted by PF_UNSPEC. IMHO it should better return with an error EAFNOSUPPORT.

Attached patch fixes it. Please apply.

BTW Credits to Jan Kara <[email protected]> for discovering and analysing this bug.

Michal Ludvig

Signature · 3 lines
SUSE Labs                    [email protected]
(+420) 296.542.396        http://www.suse.cz
Personal homepage http://www.logix.cz/michal

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/12/03 18:06:31+01:00 [email protected]
# Return EAFNOSUPPORT if requested operation on unsupported family.
#
# Signed-off-by: Michal Ludvig <[email protected]>
#

# net/core/rtnetlink.c
#   2004/12/03 18:05:49+01:00 [email protected] +4 -2
#   Return EAFNOSUPPORT if requested operation on unsupported family.
# 
diff -Nru a/net/core/rtnetlink.c b/net/core/rtnetlink.c
--- a/net/core/rtnetlink.c	2004-12-03 18:30:33 +01:00
+++ b/net/core/rtnetlink.c	2004-12-03 18:30:33 +01:00
@@ -477,8 +477,10 @@
 	}

link_tab = rtnetlink_links[family];
- if (link_tab == NULL)
- link_tab = rtnetlink_links[PF_UNSPEC];
+ if (link_tab == NULL) {
+ *errp = -EAFNOSUPPORT;
+ return -1;
+ }
link = &link_tab[type];

sz_idx = type>>2;

#1 lore raw

I think this patch will break more than it fixes. You need to do a lot more testing to verify it doesnt. Actually you should probably fix whats being invoked for the ifa messages when PF_UNSPEC is selected to check that it only flushes v6 addresses when V6 is on and reject when it is not compiled in.

cheers,
jamal

Quoted 13 lines · Michal Ludvig

Hi,

running 'ip -6 addr flush dev eth0' on a kernel without IPv6 support flushes *all* addresses from the interface, even those IPv4 ones, because the unsupported protocol is substituted by PF_UNSPEC. IMHO it should better return with an error EAFNOSUPPORT.

Attached patch fixes it. Please apply.

BTW Credits to Jan Kara <[email protected]> for discovering and analysing this bug.

Michal Ludvig

#2 lore raw
In reply to Michal Ludvig

* Michal Ludvig <[email protected]> 2004-12-03 18:43

Quoted 11 lines

running 'ip -6 addr flush dev eth0' on a kernel without IPv6 support flushes *all* addresses from the interface, even those IPv4 ones, because the unsupported protocol is substituted by PF_UNSPEC. IMHO it should better return with an error EAFNOSUPPORT.

diff -Nru a/net/core/rtnetlink.c b/net/core/rtnetlink.c

net/core/rtnetlink.c +0 -0
@@ -477,8 +477,10 @@
 	}

net/core/rtnetlink.c line 486

Quoted 10 lines
net/core/rtnetlink.c +4 -2
 	link_tab = rtnetlink_links[family];
-	if (link_tab == NULL)
-		link_tab = rtnetlink_links[PF_UNSPEC];
+	if (link_tab == NULL) {
+		*errp = -EAFNOSUPPORT;
+		return -1;
+	}
 	link = &link_tab[type];
 
 	sz_idx = type>>2;

Your patch would fix this issue but might break various things. The actual problem is that iproute2 doesn't check the family in its filter. It blindly assumes that the kernel only returns addresses of the kind it has requested. I can understand if you think the current behaviour is wrong but we shouldn't change it in the middle of a stable tree.

ip/ipaddress.c +2 -0
@@ -330,6 +330,8 @@
 				return 0;
 		}
 	}
+	if (filter.family && filter.family != ifa->ifa_family)
+		return 0;
 
 	if (filter.flushb) {
 		struct nlmsghdr *fn;
#3 lore raw
Quoted 5 lines · Thomas Graf

Your patch would fix this issue but might break various things. The actual problem is that iproute2 doesn't check the family in its filter. It blindly assumes that the kernel only returns addresses of the kind it has requested. I can understand if you think the current behaviour is wrong but we shouldn't change it in the middle of a stable tree.

Why would it be wrong? The PF_UNSPEC is there for a purpose.

If user space decides it wants to flush ipv4 addresses blindly that user spaces fault. The patch you attached seems legit. did you verify it?

BTW, Stephen - are you still updating iproute2?

cheers,
jamal

#4 lore raw

* jamal <[email protected]> 2004-12-06 21:27

Quoted 9 lines · Thomas Graf
Quoted 6 lines

Your patch would fix this issue but might break various things. The actual problem is that iproute2 doesn't check the family in its filter. It blindly assumes that the kernel only returns addresses of the kind it has requested. I can understand if you think the current behaviour is wrong but we shouldn't change it in the middle of a stable tree.

Why would it be wrong? The PF_UNSPEC is there for a purpose.

I don't think it is wrong myself but I understand if someone does. If one sends a GETADDR request for PF_INET6 one might expect to either receive all ipv6 addresses or none and to only receive all addresess of any type if PF_UNSPEC was specified.

Quoted 2 lines

If user space decides it wants to flush ipv4 addresses blindly that user spaces fault. The patch you attached seems legit. did you verify it?

Not yet, it probably has to be applied to iproute.c as well. I'll have a look at it and do some testing.

#5 lore raw
Quoted 9 lines · Thomas Graf

* jamal <[email protected]> 2004-12-06 21:27

Quoted 8 lines · Thomas Graf
Quoted 6 lines

Your patch would fix this issue but might break various things. The actual problem is that iproute2 doesn't check the family in its filter. It blindly assumes that the kernel only returns addresses of the kind it has requested. I can understand if you think the current behaviour is wrong but we shouldn't change it in the middle of a stable tree.

Quoted 8 lines
Quoted 1 line

Why would it be wrong? The PF_UNSPEC is there for a purpose.

I don't think it is wrong myself but I understand if someone does.
If
one sends a GETADDR request for PF_INET6 one might expect to either receive all ipv6 addresses or none and to only receive all addresess of any type if PF_UNSPEC was specified.

Thats debatable.
Its user space that issues the flushing after a response from the kernel. It happens to be flushing IPV4 addresses.
Thats why your filter in ip is the answer.

BTW, did the gnet_stats patches to iproute2 ever get merged? If you have cycles, can you please look at that hang being reported using older tc with 2.6.10-rc3?

cheers,
jamal

#6 lore raw
Quoted 10 lines
Quoted 5 lines

I don't think it is wrong myself but I understand if someone does. If one sends a GETADDR request for PF_INET6 one might expect to either receive all ipv6 addresses or none and to only receive all addresess of any type if PF_UNSPEC was specified.

Thats debatable.
Its user space that issues the flushing after a response from the kernel. It happens to be flushing IPV4 addresses.
Thats why your filter in ip is the answer.

Agreed.

Quoted 1 line

BTW, did the gnet_stats patches to iproute2 ever get merged?

Not sure, I will check that.

Quoted 2 lines

If you have cycles, can you please look at that hang being reported using older tc with 2.6.10-rc3?

It's not really related to the gnet_stats code. stats_lock isn't set in the action code when using an older iproute2. I haven't tested this case because it was marked as broken anyway. I compiled an older version of iproute2 and will look into it today.

#7 lore raw
Quoted 3 lines · Thomas Graf

It's not really related to the gnet_stats code. stats_lock isn't set in the action code when using an older iproute2. I haven't tested this case because it was marked as broken anyway.

Can you ping my memory on this? Is this tc with initial support for actions or something much older than that.

cheers,
jamal

#8 lore raw

* jamal <[email protected]> 2004-12-07 08:20

Quoted 8 lines · Thomas Graf
Quoted 4 lines

It's not really related to the gnet_stats code. stats_lock isn't set in the action code when using an older iproute2. I haven't tested this case because it was marked as broken anyway.

Can you ping my memory on this? Is this tc with initial support for actions or something much older than that.

I'm not sure, I'm testing with a version having no action support at all. It should be fairly easy to find the bug once I have the time to really look into it. I'm still getting interrupted all the time at the moment.

All actions created via tcf_hash_create, tcf_police_locate, and tcf_act_police_locate should be fine. There must be some bogus path related to older tc versions.

#9 lore raw

* Thomas Graf <[email protected]> 2004-12-07 15:10

Quoted 7 lines

* jamal <[email protected]> 2004-12-07 08:20

Quoted 6 lines · Thomas Graf
Quoted 4 lines

It's not really related to the gnet_stats code. stats_lock isn't set in the action code when using an older iproute2. I haven't tested this case because it was marked as broken anyway.

Quoted 7 lines
Quoted 2 lines

Can you ping my memory on this? Is this tc with initial support for actions or something much older than that.

I'm not sure, I'm testing with a version having no action support at all. It should be fairly easy to find the bug once I have the time to really look into it. I'm still getting interrupted all the time at the moment.

One major problem is that the tc_dump_action path doesn't take care of TCA_OLD_COMPAT resulting in calling tcf_action_copy_stats for policers which is a bad thing since their a->priv is set to tcf_police instead of the generic header and thus causes random behaviour.

One solution would be to make tcf_police compatible to tca_gen.

Thoughts?

include/net/act_api.h +30 -25
@@ -8,15 +8,42 @@
 #include <net/sch_generic.h>
 #include <net/pkt_sched.h>
 
+#ifdef CONFIG_NET_CLS_ACT
+
+#define ACT_P_CREATED 1
+#define ACT_P_DELETED 1
+#define tca_gen(name) \
+struct tcf_##name *next; \
+	u32 index; \
+	int refcnt; \
+	int bindcnt; \
+	u32 capab; \
+	int action; \
+	struct tcf_t tm; \
+	struct gnet_stats_basic bstats; \
+	struct gnet_stats_queue qstats; \
+	struct gnet_stats_rate_est rate_est; \
+	spinlock_t *stats_lock; \
+	spinlock_t lock
+
+#endif
+
 struct tcf_police
 {
+#ifdef CONFIG_NET_CLS_ACT
+	tca_gen(police);
+#else
 	struct tcf_police *next;
 	int		refcnt;
-#ifdef CONFIG_NET_CLS_ACT
-	int		bindcnt;
-#endif
 	u32		index;
 	int		action;
+	spinlock_t	lock;
+	struct gnet_stats_basic bstats;
+	struct gnet_stats_queue qstats;
+	struct gnet_stats_rate_est rate_est;
+	spinlock_t	*stats_lock;
+#endif
+
 	int		result;
 	u32		ewma_rate;
 	u32		burst;

@@ -24,34 +51,12 @@
 	u32		toks;
 	u32		ptoks;
 	psched_time_t	t_c;
-	spinlock_t	lock;
 	struct qdisc_rate_table *R_tab;
 	struct qdisc_rate_table *P_tab;
-
-	struct gnet_stats_basic bstats;
-	struct gnet_stats_queue qstats;
-	struct gnet_stats_rate_est rate_est;
-	spinlock_t	*stats_lock;
 };
 
 #ifdef CONFIG_NET_CLS_ACT
 
-#define ACT_P_CREATED 1
-#define ACT_P_DELETED 1
-#define tca_gen(name) \
-struct tcf_##name *next; \
-	u32 index; \
-	int refcnt; \
-	int bindcnt; \
-	u32 capab; \
-	int action; \
-	struct tcf_t tm; \
-	struct gnet_stats_basic bstats; \
-	struct gnet_stats_queue qstats; \
-	struct gnet_stats_rate_est rate_est; \
-	spinlock_t *stats_lock; \
-	spinlock_t lock
-
 struct tcf_act_hdr
 {
 	tca_gen(act_hdr);