Skip to contentSkip to conversation

IPV6 source routing patch is still broken?

DISCUSSION Chuck Ebbert 3 participants 10 messages Apr 26, 2007 – Apr 27, 2007

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

Chuck Ebbert

author
#0 lore raw

Looking at the patch that went into 2.6.20.9, I can't see how type 2 packets get through at all. Shouldn't this part read:

+ case IPV6_SRCRT_TYPE_2:
+ if (accept_source_route >= 0)
+ break;
+ kfree_skb(skb);
+ return -1;
+ case IPV6_SRCRT_TYPE_0:
+ if (accept_source_route > 0)
+ break;
+ kfree_skb(skb);
+ return -1;

And what does this do?

+ switch (hdr->type) {
+#ifdef CONFIG_IPV6_MIP6
+ break;
+#endif

break by itself like that doesn't do anything. If it did then people with mobile ipv6 enabled would always accept all source routing. (They should at least know that's a problem.)

maintainer
#1 lore raw

From: Chuck Ebbert <[email protected]>
Date: Thu, 26 Apr 2007 17:53:58 -0400

Quoted 10 lines

And what does this do?

(quoted diff) +4 -0
+	switch (hdr->type) {
+#ifdef CONFIG_IPV6_MIP6
+		break;
+#endif

break by itself like that doesn't do anything. If it did then people with mobile ipv6 enabled would always accept all source routing. (They should at least know that's a problem.)

We are aware of this part already, and I've sent the fix to the -stable folks and Linus so we can figure out how to deal with this and the netlink OOPS'er too.

To be honest I don't blame anyone but the compiler, it shouldn't accept that code without at least a warning.

author
#2 lore raw
Quoted 10 lines · David Miller

From: Chuck Ebbert <[email protected]>
Date: Thu, 26 Apr 2007 17:53:58 -0400

Quoted 7 lines

And what does this do?

(quoted diff) +4 -0
+	switch (hdr->type) {
+#ifdef CONFIG_IPV6_MIP6
+		break;
+#endif
Quoted 8 lines
Quoted 3 lines

break by itself like that doesn't do anything. If it did then people with mobile ipv6 enabled would always accept all source routing. (They should at least know that's a problem.)

We are aware of this part already, and I've sent the fix to the -stable folks and Linus so we can figure out how to deal with this and the netlink OOPS'er too.

Another question: is it a good idea to even be shipping release kernels with Mobile IPV6 enabled? Unfortunately, "experimental" isn't enough to go by...

#3 lore raw

In article <[email protected]> (at Thu, 26 Apr 2007 19:10:56 -0400), Chuck Ebbert <[email protected]> says:

Quoted 3 lines

Another question: is it a good idea to even be shipping release kernels with Mobile IPV6 enabled? Unfortunately, "experimental" isn't enough to go by...

Well, we have still 2 missing pieces in 2.6.21, and we are trying to push them (source address selection, which is already in net-2.6 and xfrm subpolicy, which is still on dave's backlog?) to 2.6.22. After 2.6.23, we would say yes.

--yoshfuji

maintainer
#4 lore raw
In reply to Chuck Ebbert

From: Chuck Ebbert <[email protected]>
Date: Thu, 26 Apr 2007 19:10:56 -0400

Quoted 3 lines

Another question: is it a good idea to even be shipping release kernels with Mobile IPV6 enabled? Unfortunately, "experimental" isn't enough to go by...

There are no known issues in the mobile-ipv6 code to my knowledge. At least no OOPS'ers or anything like that.

maintainer
#5 lore raw
In reply to Chuck Ebbert

From: Chuck Ebbert <[email protected]>
Date: Thu, 26 Apr 2007 17:53:58 -0400

Quoted 4 lines

Looking at the patch that went into 2.6.20.9, I can't see how type 2 packets get through at all. Shouldn't this part read:

Review

Quoted 10 lines
(quoted diff) +10 -0
+       case IPV6_SRCRT_TYPE_2:
+               if (accept_source_route >= 0)
+                       break;
+               kfree_skb(skb);
+               return -1;
+       case IPV6_SRCRT_TYPE_0:
+               if (accept_source_route > 0)
+                       break;
+               kfree_skb(skb);
+               return -1;

Yes, that looks like it matches the sysctl documentation more closely:

accept_source_route - INTEGER
Accept source routing (routing extension header).

> 0: Accept routing header.
= 0: Accept only routing header type 2.
< 0: Do not accept routing header.

Type 2 packets should get through as long as the value of the sysctl is not negative.

Hmmm?

author
#6 lore raw
Quoted 16 lines · David Miller

Review

Quoted 10 lines
(quoted diff) +10 -0
+       case IPV6_SRCRT_TYPE_2:
+               if (accept_source_route >= 0)
+                       break;
+               kfree_skb(skb);
+               return -1;
+       case IPV6_SRCRT_TYPE_0:
+               if (accept_source_route > 0)
+                       break;
+               kfree_skb(skb);
+               return -1;

Yes, that looks like it matches the sysctl documentation more closely:

accept_source_route - INTEGER
Accept source routing (routing extension header).

Quoted 6 lines

> 0: Accept routing header.
= 0: Accept only routing header type 2.
< 0: Do not accept routing header.

Type 2 packets should get through as long as the value of the sysctl is not negative.

It was Sergey Vlasov who first found this. I had tried to find his original message but I was searching the wrong place.

Sergey, you should send networking-related messages to netdev.

maintainer
#7 lore raw

From: Chuck Ebbert <[email protected]>
Date: Thu, 26 Apr 2007 18:57:06 -0400

Quoted 21 lines
Quoted 20 lines · David Miller

Review

Quoted 10 lines
(quoted diff) +10 -0
+       case IPV6_SRCRT_TYPE_2:
+               if (accept_source_route >= 0)
+                       break;
+               kfree_skb(skb);
+               return -1;
+       case IPV6_SRCRT_TYPE_0:
+               if (accept_source_route > 0)
+                       break;
+               kfree_skb(skb);
+               return -1;

Yes, that looks like it matches the sysctl documentation more closely:

accept_source_route - INTEGER
Accept source routing (routing extension header).

> 0: Accept routing header.
= 0: Accept only routing header type 2.
< 0: Do not accept routing header.

Quoted 5 lines
Quoted 2 lines

Type 2 packets should get through as long as the value of the sysctl is not negative.

It was Sergey Vlasov who first found this. I had tried to find his original message but I was searching the wrong place.

Actually, earlier in the function accept_source_route is verified, and if it is negative ipv6_rthdr_rcv() returns immediately. This is done by the initial code which reads:

	if (accept_source_route < 0 ||
	    ((idev = in6_dev_get(skb->dev)) == NULL)) {
		kfree_skb(skb);
		return -1;
	}
	if (idev->cnf.accept_source_route < 0) {
		in6_dev_put(idev);
		kfree_skb(skb);
		return -1;
	}

then the function proceeds to use the largest of 'accept_source_route' and 'idev->cnf.accept_source_route' for further checks.

So when we get to the switch statement in question, we know it will be a positive value, so none of the purely negative cases need to be considered.

So with Yoshifuji-sans small fix, the switch statement covers all the cases properly:

	switch (hdr->type) {
#ifdef CONFIG_IPV6_MIP6
	case IPV6_SRCRT_TYPE_2:
		break;
#endif
	case IPV6_SRCRT_TYPE_0:
		if (accept_source_route > 0)
			break;
		kfree_skb(skb);
		return -1;
	default:
		IP6_INC_STATS_BH(ip6_dst_idev(skb->dst),
				 IPSTATS_MIB_INHDRERRORS);
		icmpv6_param_prob(skb, ICMPV6_HDR_FIELD, (&hdr->type) - skb->nh.raw);
		return -1;
	}
#8 lore raw

In article <[email protected]> (at Fri, 27 Apr 2007 02:08:05 -0700 (PDT)), David Miller <[email protected]> says:

Quoted 3 lines

then the function proceeds to use the largest of 'accept_source_route' and 'idev->cnf.accept_source_route' for further checks.

Smallest:

        if (accept_source_route > idev->cnf.accept_source_route)
                accept_source_route = idev->cnf.accept_source_route;

--yoshufji

maintainer
#9 lore raw

From: YOSHIFUJI Hideaki / 吉藤英明 <[email protected]>
Date: Fri, 27 Apr 2007 18:36:35 +0900 (JST)

Quoted 9 lines

In article <[email protected]> (at Fri, 27 Apr 2007 02:08:05 -0700 (PDT)), David Miller <[email protected]> says:

Quoted 3 lines

then the function proceeds to use the largest of 'accept_source_route' and 'idev->cnf.accept_source_route' for further checks.

Smallest:

        if (accept_source_route > idev->cnf.accept_source_route)
                accept_source_route = idev->cnf.accept_source_route;

Correct, my mistake.