[PATCH] 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 [off-list ref] for discovering and analysing
this bug.

Michal Ludvig
-- 
SUSE Labs                    mludvig@suse.cz
(+420) 296.542.396        http://www.suse.cz
Personal homepage http://www.logix.cz/michal

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

On Fri, 2004-12-03 at 12:43, Michal Ludvig wrote:

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 [off-list ref] for discovering and analysing
this bug.

Michal Ludvig

* Michal Ludvig [off-list ref] 2004-12-03 18:43

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.

--- iproute2-2.6.9.orig/ip/ipaddress.c	2004-10-19 22:49:02.000000000 +0200
+++ iproute2-2.6.9/ip/ipaddress.c	2004-12-06 14:55:58.000000000 +0100

@@ -330,6 +330,8 @@
 				return 0;
 		}
 	}
+	if (filter.family && filter.family != ifa->ifa_family)
+		return 0;
 
 	if (filter.flushb) {
 		struct nlmsghdr *fn;

On Mon, 2004-12-06 at 09:02, Thomas Graf wrote:

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

* jamal [off-list ref] 2004-12-06 21:27

On Mon, 2004-12-06 at 09:02, Thomas Graf wrote:

quoted

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.

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.

On Tue, 2004-12-07 at 07:49, Thomas Graf wrote:

* jamal [off-list ref] 2004-12-06 21:27

quoted

On Mon, 2004-12-06 at 09:02, Thomas Graf wrote:

quoted

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.

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

quoted

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.

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

Not sure, I will check that.

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.

On Tue, 2004-12-07 at 08:17, Thomas Graf wrote:

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

* jamal [off-list ref] 2004-12-07 08:20

On Tue, 2004-12-07 at 08:17, Thomas Graf wrote:

quoted

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.

* Thomas Graf [off-list ref] 2004-12-07 15:10

* jamal [off-list ref] 2004-12-07 08:20

quoted

On Tue, 2004-12-07 at 08:17, Thomas Graf wrote:

quoted

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.

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?

--- linux-2.6.10-rc2-bk13.orig/include/net/act_api.h	2004-11-30 14:01:11.000000000 +0100
+++ linux-2.6.10-rc2-bk13/include/net/act_api.h	2004-12-07 17:49:50.000000000 +0100

@@ -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);

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?

They're not in bk://developer.osdl.org/iproute2 so I guess not. I've put
a iproute2 including all my changes into a tarball at:
	http://people.suug.ch/~tgr/iproute2/iproute2-2.6.9-tgr.tar.gz

On Tue, 7 Dec 2004 18:52:59 +0100
Thomas Graf [off-list ref] wrote:

http://people.suug.ch/~tgr/iproute2/iproute2-2.6.9-tgr.tar.gz

Thanks, I'm behind on iproute2