diff mbox

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

Message ID 1282107908-3585-1-git-send-email-xiaosuo@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Changli Gao Aug. 18, 2010, 5:05 a.m. UTC 
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/sched/cls_flow.c |   67 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 29 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

jamal Aug. 18, 2010, 11:02 a.m. UTC | #1 
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Changli Gao Aug. 18, 2010, 12:42 p.m. UTC | #2 
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.
jamal Aug. 18, 2010, 12:54 p.m. UTC | #3 
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Changli Gao Aug. 18, 2010, 1:18 p.m. UTC | #4 
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.
jamal Aug. 18, 2010, 1:53 p.m. UTC | #5 
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Aug. 20, 2010, 12:17 a.m. UTC | #6 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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