[PATCH net-next] net:sched: add action inheritdsfield to skbmod

Subsystems: networking [general], tc subsystem, the rest

STALE2991d REVIEWED: 1 (0M)

1 review trailer.

8 messages, 4 authors, 2018-05-25 · open the first message on its own page

[PATCH net-next] net:sched: add action inheritdsfield to skbmod

From: Fu, Qiaobin <hidden>
Date: 2018-05-17 19:33:10

net/sched: add action inheritdsfield to skbmod

The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->prioriry. This enables
later classification of packets based on the DS field.

Original idea by Jamal Hadi Salim [off-list ref]

Signed-off-by: Qiaobin Fu <redacted>
Reviewed-by: Michel Machado <redacted>
---

Note that the motivation for this patch is found in the following discussion:
https://www.spinics.net/lists/netdev/msg501061.html
---

diff --git a/include/uapi/linux/tc_act/tc_skbmod.h b/include/uapi/linux/tc_act/tc_skbmod.h
index 38c072f..0718b48 100644
--- a/include/uapi/linux/tc_act/tc_skbmod.h
+++ b/include/uapi/linux/tc_act/tc_skbmod.h

@@ -19,6 +19,7 @@
 #define SKBMOD_F_SMAC	0x2
 #define SKBMOD_F_ETYPE	0x4
 #define SKBMOD_F_SWAPMAC 0x8
+#define SKBMOD_F_INHERITDSFIELD 0x10
 
 struct tc_skbmod {
 	tc_gen;

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7..21d5bec 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c

@@ -16,6 +16,9 @@
 #include <linux/rtnetlink.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
+#include <net/dsfield.h>
 
 #include <linux/tc_act/tc_skbmod.h>
 #include <net/tc_act/tc_skbmod.h>

@@ -72,6 +75,25 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
 		ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
 	}
 
+	if (flags & SKBMOD_F_INHERITDSFIELD) {
+		int wlen = skb_network_offset(skb);
+		switch (tc_skb_protocol(skb)) {
+		case htons(ETH_P_IP):
+			wlen += sizeof(struct iphdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
+			break;
+
+		case htons(ETH_P_IPV6):
+			wlen += sizeof(struct ipv6hdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+			break;
+		}
+	}
+
 	return action;
 }

@@ -127,6 +149,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
 	if (parm->flags & SKBMOD_F_SWAPMAC)
 		lflags = SKBMOD_F_SWAPMAC;
 
+	if (parm->flags & SKBMOD_F_INHERITDSFIELD)
+		lflags |= SKBMOD_F_INHERITDSFIELD;
+
 	exists = tcf_idr_check(tn, parm->index, a, bind);
 	if (exists && bind)
 		return 0;

Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: 2018-05-18 13:06:28

On 17/05/18 03:33 PM, Fu, Qiaobin wrote:

net/sched: add action inheritdsfield to skbmod

The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->prioriry. This enables
later classification of packets based on the DS field.

Original idea by Jamal Hadi Salim [off-list ref]

Signed-off-by: Qiaobin Fu <redacted>
Reviewed-by: Michel Machado <redacted>

LGTM. Thanks for putting the effort.
As a tradition we also require that you post the iproute2
patch and make sure this works. Can you please do that?
Also: If you can add at least a test on tdc it would help
immensely. Other than that:

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2018-05-23 21:06:34

Hi,

Some style fixes:

On Thu, May 17, 2018 at 07:33:08PM +0000, Fu, Qiaobin wrote:

net/sched: add action inheritdsfield to skbmod

This extra line above should not be here.

The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->prioriry. This enables

                              typo -----^

quoted hunk

later classification of packets based on the DS field.

Original idea by Jamal Hadi Salim [off-list ref]

Signed-off-by: Qiaobin Fu <redacted>
Reviewed-by: Michel Machado <redacted>
---

Note that the motivation for this patch is found in the following discussion:
https://www.spinics.net/lists/netdev/msg501061.html
---

diff --git a/include/uapi/linux/tc_act/tc_skbmod.h b/include/uapi/linux/tc_act/tc_skbmod.h
index 38c072f..0718b48 100644
--- a/include/uapi/linux/tc_act/tc_skbmod.h
+++ b/include/uapi/linux/tc_act/tc_skbmod.h

@@ -19,6 +19,7 @@
 #define SKBMOD_F_SMAC	0x2
 #define SKBMOD_F_ETYPE	0x4
 #define SKBMOD_F_SWAPMAC 0x8
+#define SKBMOD_F_INHERITDSFIELD 0x10
 
 struct tc_skbmod {
 	tc_gen;

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7..21d5bec 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c

@@ -16,6 +16,9 @@
 #include <linux/rtnetlink.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
+#include <net/dsfield.h>
 
 #include <linux/tc_act/tc_skbmod.h>
 #include <net/tc_act/tc_skbmod.h>

@@ -72,6 +75,25 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
 		ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
 	}
 
+	if (flags & SKBMOD_F_INHERITDSFIELD) {
+		int wlen = skb_network_offset(skb);

You need a blank line here, between var declaration and the rest.

quoted hunk

+		switch (tc_skb_protocol(skb)) {
+		case htons(ETH_P_IP):
+			wlen += sizeof(struct iphdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
+			break;
+
+		case htons(ETH_P_IPV6):
+			wlen += sizeof(struct ipv6hdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+			break;
+		}
+	}
+
 	return action;
 }

@@ -127,6 +149,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
 	if (parm->flags & SKBMOD_F_SWAPMAC)
 		lflags = SKBMOD_F_SWAPMAC;
 
+	if (parm->flags & SKBMOD_F_INHERITDSFIELD)
+		lflags |= SKBMOD_F_INHERITDSFIELD;
+
 	exists = tcf_idr_check(tn, parm->index, a, bind);
 	if (exists && bind)
 		return 0;

Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod

From: Fu, Qiaobin <hidden>
Date: 2018-05-25 05:45:05

Hi Marcelo,

Thanks for pointing out these style issues. Below is the updated version:

---
The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->priority. This enables
later classification of packets based on the DS field.

Original idea by Jamal Hadi Salim [off-list ref]

Signed-off-by: Qiaobin Fu <redacted>
Reviewed-by: Michel Machado <redacted>
---

Note that the motivation for this patch is found in the following discussion:
https://www.spinics.net/lists/netdev/msg501061.html
---

diff --git a/include/uapi/linux/tc_act/tc_skbmod.h b/include/uapi/linux/tc_act/tc_skbmod.h
index 38c072f..0718b48 100644
--- a/include/uapi/linux/tc_act/tc_skbmod.h
+++ b/include/uapi/linux/tc_act/tc_skbmod.h

@@ -19,6 +19,7 @@
 #define SKBMOD_F_SMAC	0x2
 #define SKBMOD_F_ETYPE	0x4
 #define SKBMOD_F_SWAPMAC 0x8
+#define SKBMOD_F_INHERITDSFIELD 0x10
 
 struct tc_skbmod {
 	tc_gen;

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7..e2082f6 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c

@@ -16,6 +16,9 @@
 #include <linux/rtnetlink.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
+#include <net/dsfield.h>
 
 #include <linux/tc_act/tc_skbmod.h>
 #include <net/tc_act/tc_skbmod.h>

@@ -72,6 +75,26 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
 		ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
 	}
 
+	if (flags & SKBMOD_F_INHERITDSFIELD) {
+		int wlen = skb_network_offset(skb);
+
+		switch (tc_skb_protocol(skb)) {
+		case htons(ETH_P_IP):
+			wlen += sizeof(struct iphdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
+			break;
+
+		case htons(ETH_P_IPV6):
+			wlen += sizeof(struct ipv6hdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+			break;
+		}
+	}
+
 	return action;
 }

@@ -127,6 +150,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
 	if (parm->flags & SKBMOD_F_SWAPMAC)
 		lflags = SKBMOD_F_SWAPMAC;
 
+	if (parm->flags & SKBMOD_F_INHERITDSFIELD)
+		lflags |= SKBMOD_F_INHERITDSFIELD;
+
 	exists = tcf_idr_check(tn, parm->index, a, bind);
 	if (exists && bind)
 		return 0;

On May 23, 2018, at 2:06 PM, Marcelo Ricardo Leitner [off-list ref] wrote:

Hi,

Some style fixes:

On Thu, May 17, 2018 at 07:33:08PM +0000, Fu, Qiaobin wrote:

quoted

net/sched: add action inheritdsfield to skbmod

This extra line above should not be here.

quoted

The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->prioriry. This enables

                             typo -----^

quoted

later classification of packets based on the DS field.

Original idea by Jamal Hadi Salim [off-list ref]

Signed-off-by: Qiaobin Fu <redacted>
Reviewed-by: Michel Machado <redacted>
---

Note that the motivation for this patch is found in the following discussion:
https://www.spinics.net/lists/netdev/msg501061.html
---

diff --git a/include/uapi/linux/tc_act/tc_skbmod.h b/include/uapi/linux/tc_act/tc_skbmod.h
index 38c072f..0718b48 100644
--- a/include/uapi/linux/tc_act/tc_skbmod.h
+++ b/include/uapi/linux/tc_act/tc_skbmod.h

@@ -19,6 +19,7 @@

#define SKBMOD_F_SMAC	0x2
#define SKBMOD_F_ETYPE	0x4
#define SKBMOD_F_SWAPMAC 0x8
+#define SKBMOD_F_INHERITDSFIELD 0x10

struct tc_skbmod {
	tc_gen;

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7..21d5bec 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c

@@ -16,6 +16,9 @@

#include <linux/rtnetlink.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
+#include <net/dsfield.h>

#include <linux/tc_act/tc_skbmod.h>
#include <net/tc_act/tc_skbmod.h>

@@ -72,6 +75,25 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,

		ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
	}

+	if (flags & SKBMOD_F_INHERITDSFIELD) {
+		int wlen = skb_network_offset(skb);

You need a blank line here, between var declaration and the rest.

quoted

+		switch (tc_skb_protocol(skb)) {
+		case htons(ETH_P_IP):
+			wlen += sizeof(struct iphdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
+			break;
+
+		case htons(ETH_P_IPV6):
+			wlen += sizeof(struct ipv6hdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+			break;
+		}
+	}
+
	return action;
}

@@ -127,6 +149,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,

	if (parm->flags & SKBMOD_F_SWAPMAC)
		lflags = SKBMOD_F_SWAPMAC;

+	if (parm->flags & SKBMOD_F_INHERITDSFIELD)
+		lflags |= SKBMOD_F_INHERITDSFIELD;
+
	exists = tcf_idr_check(tn, parm->index, a, bind);
	if (exists && bind)
		return 0;

Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2018-05-25 14:34:36

On Fri, May 25, 2018 at 05:45:03AM +0000, Fu, Qiaobin wrote:

Hi Marcelo,

Thanks for pointing out these style issues. Below is the updated version:

Hi Qiaobin,

Looks good to me. Now you have to submit it like you submitted the
original patch, but add the version tag to the summary. Like '[PATCH
v2 net-next] ....'
And without the text before the changelog.

Thanks.

quoted hunk

---
The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->priority. This enables
later classification of packets based on the DS field.

Original idea by Jamal Hadi Salim [off-list ref]

Signed-off-by: Qiaobin Fu <redacted>
Reviewed-by: Michel Machado <redacted>
---

Note that the motivation for this patch is found in the following discussion:
https://www.spinics.net/lists/netdev/msg501061.html
---

diff --git a/include/uapi/linux/tc_act/tc_skbmod.h b/include/uapi/linux/tc_act/tc_skbmod.h
index 38c072f..0718b48 100644
--- a/include/uapi/linux/tc_act/tc_skbmod.h
+++ b/include/uapi/linux/tc_act/tc_skbmod.h

@@ -19,6 +19,7 @@
 #define SKBMOD_F_SMAC	0x2
 #define SKBMOD_F_ETYPE	0x4
 #define SKBMOD_F_SWAPMAC 0x8
+#define SKBMOD_F_INHERITDSFIELD 0x10
 
 struct tc_skbmod {
 	tc_gen;

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7..e2082f6 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c

@@ -16,6 +16,9 @@
 #include <linux/rtnetlink.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
+#include <net/dsfield.h>
 
 #include <linux/tc_act/tc_skbmod.h>
 #include <net/tc_act/tc_skbmod.h>

@@ -72,6 +75,26 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
 		ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
 	}
 
+	if (flags & SKBMOD_F_INHERITDSFIELD) {
+		int wlen = skb_network_offset(skb);
+
+		switch (tc_skb_protocol(skb)) {
+		case htons(ETH_P_IP):
+			wlen += sizeof(struct iphdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
+			break;
+
+		case htons(ETH_P_IPV6):
+			wlen += sizeof(struct ipv6hdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+			break;
+		}
+	}
+
 	return action;
 }

@@ -127,6 +150,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
 	if (parm->flags & SKBMOD_F_SWAPMAC)
 		lflags = SKBMOD_F_SWAPMAC;
 
+	if (parm->flags & SKBMOD_F_INHERITDSFIELD)
+		lflags |= SKBMOD_F_INHERITDSFIELD;
+
 	exists = tcf_idr_check(tn, parm->index, a, bind);
 	if (exists && bind)
 		return 0;

quoted

On May 23, 2018, at 2:06 PM, Marcelo Ricardo Leitner [off-list ref] wrote:

Hi,

Some style fixes:

On Thu, May 17, 2018 at 07:33:08PM +0000, Fu, Qiaobin wrote:

quoted

net/sched: add action inheritdsfield to skbmod

This extra line above should not be here.

quoted

The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->prioriry. This enables

                             typo -----^

quoted

later classification of packets based on the DS field.

Original idea by Jamal Hadi Salim [off-list ref]

Signed-off-by: Qiaobin Fu <redacted>
Reviewed-by: Michel Machado <redacted>
---

Note that the motivation for this patch is found in the following discussion:
https://www.spinics.net/lists/netdev/msg501061.html
---

diff --git a/include/uapi/linux/tc_act/tc_skbmod.h b/include/uapi/linux/tc_act/tc_skbmod.h
index 38c072f..0718b48 100644
--- a/include/uapi/linux/tc_act/tc_skbmod.h
+++ b/include/uapi/linux/tc_act/tc_skbmod.h

@@ -19,6 +19,7 @@

#define SKBMOD_F_SMAC	0x2
#define SKBMOD_F_ETYPE	0x4
#define SKBMOD_F_SWAPMAC 0x8
+#define SKBMOD_F_INHERITDSFIELD 0x10

struct tc_skbmod {
	tc_gen;

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7..21d5bec 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c

@@ -16,6 +16,9 @@

#include <linux/rtnetlink.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
+#include <net/dsfield.h>

#include <linux/tc_act/tc_skbmod.h>
#include <net/tc_act/tc_skbmod.h>

@@ -72,6 +75,25 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,

		ether_addr_copy(eth_hdr(skb)->h_source, (u8 *)tmpaddr);
	}

+	if (flags & SKBMOD_F_INHERITDSFIELD) {
+		int wlen = skb_network_offset(skb);

You need a blank line here, between var declaration and the rest.

quoted

+		switch (tc_skb_protocol(skb)) {
+		case htons(ETH_P_IP):
+			wlen += sizeof(struct iphdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
+			break;
+
+		case htons(ETH_P_IPV6):
+			wlen += sizeof(struct ipv6hdr);
+			if (!pskb_may_pull(skb, wlen))
+				return TC_ACT_SHOT;
+			skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+			break;
+		}
+	}
+
	return action;
}

@@ -127,6 +149,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,

	if (parm->flags & SKBMOD_F_SWAPMAC)
		lflags = SKBMOD_F_SWAPMAC;

+	if (parm->flags & SKBMOD_F_INHERITDSFIELD)
+		lflags |= SKBMOD_F_INHERITDSFIELD;
+
	exists = tcf_idr_check(tn, parm->index, a, bind);
	if (exists && bind)
		return 0;

Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod

From: Cong Wang <hidden>
Date: 2018-05-25 18:48:05

On Thu, May 24, 2018 at 10:45 PM, Fu, Qiaobin [off-list ref] wrote:

The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->priority. This enables
later classification of packets based on the DS field.

Please move it to skbedit.

Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod

From: Cong Wang <hidden>
Date: 2018-05-23 23:01:38

On Thu, May 17, 2018 at 12:33 PM, Fu, Qiaobin [off-list ref] wrote:

net/sched: add action inheritdsfield to skbmod

The new action inheritdsfield copies the field DS of
IPv4 and IPv6 packets into skb->prioriry. This enables
later classification of packets based on the DS field.

Original idea by Jamal Hadi Salim [off-list ref]

Signed-off-by: Qiaobin Fu <redacted>
Reviewed-by: Michel Machado <redacted>

Hmm, but skbedit seems better than skbmod for this job,
given:

1) It already modifies skb->priority, although with a given value

2) skbmod doesn't change skb metadata, it only changes payload

I am _not_ saying there is strict rule for what skbmod can or can't
change, it calls itself "data modifier", so I am saying we probably
need to follow this existing practice.

Re: [PATCH net-next] net:sched: add action inheritdsfield to skbmod

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: 2018-05-24 12:11:56

On 23/05/18 07:01 PM, Cong Wang wrote:

On Thu, May 17, 2018 at 12:33 PM, Fu, Qiaobin [off-list ref] wrote:

Hmm, but skbedit seems better than skbmod for this job,
given:

1) It already modifies skb->priority, although with a given value

2) skbmod doesn't change skb metadata, it only changes payload

I am _not_ saying there is strict rule for what skbmod can or can't
change, it calls itself "data modifier", so I am saying we probably
need to follow this existing practice.

I am indifferent - you can move it to skbedit.
Note: I have patches which i will send out at some point when
I get the chance on pedit for implementing the concept of
"copy data to metadata" and  "copy metadata to data"
for pedit it made a lot of sense to add the feature there.
In this case skbedit makes more sense.

cheers,
jamal

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help