IPV6 source routing patch is still broken?

IPV6 source routing patch is still broken?

[Chuck Ebbert]

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.)

Re: IPV6 source routing patch is still broken?

[David Miller]

From: Chuck Ebbert <cebbert@redhat.com>
Date: Thu, 26 Apr 2007 17:53:58 -0400

> 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.)

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.

Re: IPV6 source routing patch is still broken?

[David Miller]

From: Chuck Ebbert <cebbert@redhat.com>
Date: Thu, 26 Apr 2007 17:53:58 -0400

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

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?

Re: IPV6 source routing patch is still broken?

[Chuck Ebbert]

David Miller wrote:
>> +       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.

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.

Re: IPV6 source routing patch is still broken?

[Chuck Ebbert]

David Miller wrote:
> From: Chuck Ebbert <cebbert@redhat.com>
> Date: Thu, 26 Apr 2007 17:53:58 -0400
> 
>> 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.)
> 
> 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...

Re: IPV6 source routing patch is still broken?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <46313180.6040809@redhat.com> (at Thu, 26 Apr 2007 19:10:56 -0400), Chuck Ebbert <cebbert@redhat.com> says:

> 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

Re: IPV6 source routing patch is still broken?

[David Miller]

From: Chuck Ebbert <cebbert@redhat.com>
Date: Thu, 26 Apr 2007 19:10:56 -0400

> 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.

Re: IPV6 source routing patch is still broken?

[David Miller]

From: Chuck Ebbert <cebbert@redhat.com>
Date: Thu, 26 Apr 2007 18:57:06 -0400

> David Miller wrote:
> >> +       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.
> 
> 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;
	}

Re: IPV6 source routing patch is still broken?

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20070427.020805.88703639.davem@davemloft.net> (at Fri, 27 Apr 2007 02:08:05 -0700 (PDT)), David Miller <davem@davemloft.net> says:

> 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

Re: IPV6 source routing patch is still broken?

[David Miller]

From: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org>
Date: Fri, 27 Apr 2007 18:36:35 +0900 (JST)

> In article <20070427.020805.88703639.davem@davemloft.net> (at Fri, 27 Apr 2007 02:08:05 -0700 (PDT)), David Miller <davem@davemloft.net> says:
> 
> > 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.