[PATCH 5/8] net_sched: cls_flow: use proto_ports_offset() to support AH message

[PATCH 5/8] net_sched: cls_flow: use proto_ports_offset() to support AH message

[Changli Gao]

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/sched/cls_flow.c |   67 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 29 deletions(-)
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index e17096e..cd709f1 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -111,44 +111,41 @@ static u32 flow_get_proto(struct sk_buff *skb)
 	}
 }
 
-static int has_ports(u8 protocol)
-{
-	switch (protocol) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_UDPLITE:
-	case IPPROTO_SCTP:
-	case IPPROTO_DCCP:
-	case IPPROTO_ESP:
-		return 1;
-	default:
-		return 0;
-	}
-}
-
 static u32 flow_get_proto_src(struct sk_buff *skb)
 {
 	switch (skb->protocol) {
 	case htons(ETH_P_IP): {
 		struct iphdr *iph;
+		int poff;
 
 		if (!pskb_network_may_pull(skb, sizeof(*iph)))
 			break;
 		iph = ip_hdr(skb);
-		if (!(iph->frag_off&htons(IP_MF|IP_OFFSET)) &&
-		    has_ports(iph->protocol) &&
-		    pskb_network_may_pull(skb, iph->ihl * 4 + 2))
-			return ntohs(*(__be16 *)((void *)iph + iph->ihl * 4));
+		if (iph->frag_off & htons(IP_MF|IP_OFFSET))
+			break;
+		poff = proto_ports_offset(iph->protocol);
+		if (poff >= 0 &&
+		    pskb_network_may_pull(skb, iph->ihl * 4 + 2 + poff)) {
+			iph = ip_hdr(skb);
+			return ntohs(*(__be16 *)((void *)iph + iph->ihl * 4 +
+						 poff));
+		}
 		break;
 	}
 	case htons(ETH_P_IPV6): {
 		struct ipv6hdr *iph;
+		int poff;
 
-		if (!pskb_network_may_pull(skb, sizeof(*iph) + 2))
+		if (!pskb_network_may_pull(skb, sizeof(*iph)))
 			break;
 		iph = ipv6_hdr(skb);
-		if (has_ports(iph->nexthdr))
-			return ntohs(*(__be16 *)&iph[1]);
+		poff = proto_ports_offset(iph->nexthdr);
+		if (poff >= 0 &&
+		    pskb_network_may_pull(skb, sizeof(*iph) + poff + 2)) {
+			iph = ipv6_hdr(skb);
+			return ntohs(*(__be16 *)((void *)iph + sizeof(*iph) +
+						 poff));
+		}
 		break;
 	}
 	}
@@ -161,24 +158,36 @@ static u32 flow_get_proto_dst(struct sk_buff *skb)
 	switch (skb->protocol) {
 	case htons(ETH_P_IP): {
 		struct iphdr *iph;
+		int poff;
 
 		if (!pskb_network_may_pull(skb, sizeof(*iph)))
 			break;
 		iph = ip_hdr(skb);
-		if (!(iph->frag_off&htons(IP_MF|IP_OFFSET)) &&
-		    has_ports(iph->protocol) &&
-		    pskb_network_may_pull(skb, iph->ihl * 4 + 4))
-			return ntohs(*(__be16 *)((void *)iph + iph->ihl * 4 + 2));
+		if (iph->frag_off & htons(IP_MF|IP_OFFSET))
+			break;
+		poff = proto_ports_offset(iph->protocol);
+		if (poff >= 0 &&
+		    pskb_network_may_pull(skb, iph->ihl * 4 + 4 + poff)) {
+			iph = ip_hdr(skb);
+			return ntohs(*(__be16 *)((void *)iph + iph->ihl * 4 +
+						 2 + poff));
+		}
 		break;
 	}
 	case htons(ETH_P_IPV6): {
 		struct ipv6hdr *iph;
+		int poff;
 
-		if (!pskb_network_may_pull(skb, sizeof(*iph) + 4))
+		if (!pskb_network_may_pull(skb, sizeof(*iph)))
 			break;
 		iph = ipv6_hdr(skb);
-		if (has_ports(iph->nexthdr))
-			return ntohs(*(__be16 *)((void *)&iph[1] + 2));
+		poff = proto_ports_offset(iph->nexthdr);
+		if (poff >= 0 &&
+		    pskb_network_may_pull(skb, sizeof(*iph) + poff + 4)) {
+			iph = ipv6_hdr(skb);
+			return ntohs(*(__be16 *)((void *)iph + sizeof(*iph) +
+						 poff + 2));
+		}
 		break;
 	}
 	}

Re: [PATCH 5/8] net_sched: cls_flow: use proto_ports_offset() to support AH message

[jamal]

On Wed, 2010-08-18 at 13:05 +0800, Changli Gao wrote:

> -static int has_ports(u8 protocol)
> -{
> -	switch (protocol) {
> -	case IPPROTO_TCP:
> -	case IPPROTO_UDP:
> -	case IPPROTO_UDPLITE:
> -	case IPPROTO_SCTP:
> -	case IPPROTO_DCCP:
> -	case IPPROTO_ESP:
> -		return 1;
> -	default:
> -		return 0;
> -	}
> -}
> -
>  static u32 flow_get_proto_src(struct sk_buff *skb)
>  {
>  	switch (skb->protocol) {
>  	case htons(ETH_P_IP): {
>  		struct iphdr *iph;
> +		int poff;
>  
>  		if (!pskb_network_may_pull(skb, sizeof(*iph)))
>  			break;
>  		iph = ip_hdr(skb);
> -		if (!(iph->frag_off&htons(IP_MF|IP_OFFSET)) &&
> -		    has_ports(iph->protocol) &&
> -		    pskb_network_may_pull(skb, iph->ihl * 4 + 2))
> -			return ntohs(*(__be16 *)((void *)iph + iph->ihl * 4));
> +		if (iph->frag_off & htons(IP_MF|IP_OFFSET))
> +			break;
> +		poff = proto_ports_offset(iph->protocol);
> +		if (poff >= 0 &&


I dont think this maintains the same semantic. Ex: In the original code
AH returns 0. In your case it returns 4 and passes the above test.
Same with the other spot.

cheers,
jamal

Re: [PATCH 5/8] net_sched: cls_flow: use proto_ports_offset() to support AH message

[Changli Gao]

On Wed, Aug 18, 2010 at 7:02 PM, jamal <hadi@cyberus.ca> wrote:
> On Wed, 2010-08-18 at 13:05 +0800, Changli Gao wrote:
>
>> -static int has_ports(u8 protocol)
>> -{
>> -     switch (protocol) {
>> -     case IPPROTO_TCP:
>> -     case IPPROTO_UDP:
>> -     case IPPROTO_UDPLITE:
>> -     case IPPROTO_SCTP:
>> -     case IPPROTO_DCCP:
>> -     case IPPROTO_ESP:
>> -             return 1;
>> -     default:
>> -             return 0;
>> -     }
>> -}
>> -
>>  static u32 flow_get_proto_src(struct sk_buff *skb)
>>  {
>>       switch (skb->protocol) {
>>       case htons(ETH_P_IP): {
>>               struct iphdr *iph;
>> +             int poff;
>>
>>               if (!pskb_network_may_pull(skb, sizeof(*iph)))
>>                       break;
>>               iph = ip_hdr(skb);
>> -             if (!(iph->frag_off&htons(IP_MF|IP_OFFSET)) &&
>> -                 has_ports(iph->protocol) &&
>> -                 pskb_network_may_pull(skb, iph->ihl * 4 + 2))
>> -                     return ntohs(*(__be16 *)((void *)iph + iph->ihl * 4));
>> +             if (iph->frag_off & htons(IP_MF|IP_OFFSET))
>> +                     break;
>> +             poff = proto_ports_offset(iph->protocol);
>> +             if (poff >= 0 &&
>
>
> I dont think this maintains the same semantic. Ex: In the original code
> AH returns 0. In your case it returns 4 and passes the above test.
> Same with the other spot.
>

I suppose we want to spread the traffic as possible as we can. For
ESP, we use the SPI as a key. And I think we can also use SPI in the
AH header as a key. It does change the semantic slightly for AH, but I
should not hurt, as the only effect is that the AH traffic is
distributed into different flows according to their different SPI.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 5/8] net_sched: cls_flow: use proto_ports_offset() to support AH message

[jamal]

On Wed, 2010-08-18 at 20:42 +0800, Changli Gao wrote:

> I suppose we want to spread the traffic as possible as we can. For
> ESP, we use the SPI as a key. And I think we can also use SPI in the
> AH header as a key. It does change the semantic slightly for AH, but I
> should not hurt, as the only effect is that the AH traffic is
> distributed into different flows according to their different SPI.

Sounds sensible - but please go through the code and double check that
nothing breaks (since i dont think you tested and i sympathize it would
be very time consuming to test) and more importantly when you make such
fundamental changes at least make comments to that effect in the logs.

cheers,
jamal

Re: [PATCH 5/8] net_sched: cls_flow: use proto_ports_offset() to support AH message

[Changli Gao]

On Wed, Aug 18, 2010 at 8:54 PM, jamal <hadi@cyberus.ca> wrote:
>
> Sounds sensible - but please go through the code and double check that
> nothing breaks (since i dont think you tested and i sympathize it would
> be very time consuming to test) and more importantly when you make such
> fundamental changes at least make comments to that effect in the logs.
>

I had gone through the code before submitting this code. But it isn't
easy to test if it breaks sth. Would you please give a case to test ?
Thanks.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 5/8] net_sched: cls_flow: use proto_ports_offset() to support AH message

[jamal]

On Wed, 2010-08-18 at 21:18 +0800, Changli Gao wrote:

> I had gone through the code before submitting this code. But it isn't
> easy to test if it breaks sth. 

That works well actually and it is the LinuxWay(tm);-> 

> Would you please give a case to test ?

Just conformance testing. Since you added new AH SPI behavior
at least find some use case of what people normally use and make sure it
isnt broken. But if youve looked at code sufficiently i think thats good
enough.

cheers,
jamal

Re: [PATCH 5/8] net_sched: cls_flow: use proto_ports_offset() to support AH message

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Wed, 18 Aug 2010 09:53:09 -0400

> On Wed, 2010-08-18 at 21:18 +0800, Changli Gao wrote:
> 
>> I had gone through the code before submitting this code. But it isn't
>> easy to test if it breaks sth. 
> 
> That works well actually and it is the LinuxWay(tm);-> 
> 
>> Would you please give a case to test ?
> 
> Just conformance testing. Since you added new AH SPI behavior
> at least find some use case of what people normally use and make sure it
> isnt broken. But if youve looked at code sufficiently i think thats good
> enough.

I agree and I've applied his patch.