net_sched: cls_flow: use proto_ports_offset() to support AH message

PATCH Applied Changli Gao 3 participants 7 messages Aug 18, 2010 – Aug 20, 2010

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Changli Gao

author

2010-08-18 05:05 UTC #0 lore raw

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

Signed-off-by Changli Gao <[email protected]>

---
 net/sched/cls_flow.c |   67 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 29 deletions(-)

net/sched/cls_flow.c +38 ~~-29~~

index e17096e..cd709f1 100644
@@ -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;
 	}
 	}

jamal

2010-08-18 11:02 UTC #1 lore raw

Quoted 23 lines · Changli Gao

net/sched/cls_flow.c +1 ~~-15~~

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

net/sched/cls_flow.c static u32 flow_get_proto(struct sk_buff *skb) · line 127

Quoted 10 lines

net/sched/cls_flow.c +4 -4

 			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

Changli Gao

author

2010-08-18 12:42 UTC #2 lore raw

Quoted 24 lines · jamal

Quoted 22 lines · Changli Gao
net/sched/cls_flow.c +0 ~~-15~~
-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;

Quoted 17 lines

Quoted 11 lines
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.

Signature · 2 lines

Regards,
Changli Gao([email protected])

jamal

2010-08-18 12:54 UTC #3 lore raw

Quoted 5 lines · Changli Gao

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

Changli Gao

author

2010-08-18 13:18 UTC #4 lore raw

Quoted 6 lines · jamal

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.

Signature · 2 lines

Regards,
Changli Gao([email protected])

jamal

2010-08-18 13:53 UTC #5 lore raw

Quoted 2 lines · Changli Gao

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

Quoted 1 line

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

David Miller

maintainer

2010-08-20 00:17 UTC #6 lore raw

From: jamal <[email protected]>
Date: Wed, 18 Aug 2010 09:53:09 -0400

Quoted 5 lines · Changli Gao

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

Quoted 8 lines

That works well actually and it is the LinuxWay(tm);->
Quoted 1 line
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.