Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
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.
v5:
*Update the drop counter for TC_ACT_SHOT.
v4:
*Not allow setting flags other than the expected ones.
*Allow dumping the pure flags.
v3:
*Use optional flags, so that it won't break old versions of tc.
*Allow users to set both SKBEDIT_F_PRIORITY and SKBEDIT_F_INHERITDSFIELD flags.
v2:
*Fix the style issue
*Move the code from skbmod to skbedit
Original idea by Jamal Hadi Salim
Signed-off-by: Qiaobin Fu
Reviewed-by: Michel Machado
Acked-by: Jamal Hadi Salim
Reviewed-by: Marcelo Ricardo Leitner
---
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_skbedit.h
b/include/uapi/linux/tc_act/tc_skbedit.h
index fbcfe27a4e6c..6de6071ebed6 100644
--- a/include/uapi/linux/tc_act/tc_skbedit.h
+++ b/include/uapi/linux/tc_act/tc_skbedit.h
@@ -30,6 +30,7 @@
#define SKBEDIT_F_MARK 0x4
#define SKBEDIT_F_PTYPE0x8
#define SKBEDIT_F_MASK 0x10
+#define SKBEDIT_F_INHERITDSFIELD 0x20
struct tc_skbedit {
tc_gen;
@@ -45,6 +46,7 @@ enum {
TCA_SKBEDIT_PAD,
TCA_SKBEDIT_PTYPE,
TCA_SKBEDIT_MASK,
+ TCA_SKBEDIT_FLAGS,
__TCA_SKBEDIT_MAX
};
#define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 6138d1d71900..dfaf5d8028dd 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -23,6 +23,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
tc_action *a,
if (d->flags & SKBEDIT_F_PRIORITY)
skb->priority = d->priority;
+ if (d->flags & SKBEDIT_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))
+ goto err;
+ 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))
+ goto err;
+ skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+ break;
+ }
+ }
if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
skb->dev->real_num_tx_queues > d->queue_mapping)
skb_set_queue_mapping(skb, d->queue_mapping);
@@ -53,6 +75,11 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
tc_action *a,
spin_unlock(>tcf_lock);
return d->tcf_action;
+
+err:
+ d->tcf_qstats.drops++;
+ spin_unlock(>tcf_lock);
+ return TC_ACT_SHOT;
}
static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
@@ -62,6 +89,7 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX
+ 1] = {
[TCA_SKBEDIT_MARK] = { .len = sizeof(u32) },
[TCA_SKBEDIT_PTYPE] = { .len = sizeof(u16) },
[TCA_SKBEDIT_MASK] = { .len = sizeof(u32) },
+ [TCA_SKBEDIT_FLAGS] = { .len = sizeof(u64) },
};
static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
@@ -114,6 +142,13 @@ static int tcf_skbedit_init(struct net *net, struct nlattr
*nla,
mask = nla_data(tb[TCA_SKBEDIT_MASK]);
}
+ if (tb[TCA_SKBEDIT_FLAGS] != NULL) {
+ u64 *pure_flags = nla_data(tb[TCA_SKBEDIT_FLAGS]);
+
+ if (*pure_flags & SKBEDIT_F_INHERITDSFIELD)
+ flags |= SKBEDIT_F_INHERITDSFIELD;
+ }
+
parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
exists = tcf_idr_check(tn, parm->index, a, bind);
@@ -178,6 +213,7 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct
tc_action *a,
.action = d->tcf_action,
};
struct tcf_t t;
+ u64 pure_flags = 0;
if (nla_put(skb, TCA_SKBEDIT_PARMS, sizeof(opt), ))
goto nla_put_failure;
@@ -196,6 +232,11 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct
tc_action *a,
if ((d->flags & SKBEDIT_F_MASK) &&
nla_put_u32(skb, TCA_SKBEDIT_MASK, d->mask))
goto nla_put_failure;
+ if (d->flags & SKBEDIT_F_INHERITDSFIELD)
+ pure_flags |= SKBEDIT_F_INHERITDSFIELD;
+ if (pure_flags != 0 &&
+ nla_put(skb, TCA_SKBEDIT_FLAGS, sizeof(pure_flags), _flags))
+ goto nla_put_failure;
tcf_tm_dump(, >tcf_tm);
if (nla_put_64bit(skb,
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
On Wed, Jun 20, 2018 at 07:02:41PM +0200, Davide Caratti wrote: ... > > I agree that we should update the drop counter, but given that > > you're already converting the stats to be per-cpu counters, whatever we > > add now will be just symbolic since you're going to change it anyway. It wouldn't be symbolic. One thing is to convert a given increment into something else, another is to start increasing it for some (new) reason. > > that's ok for me also, as I can use the current v4 code for the rebase > (and not wait for another respin) _ but let's hear what reviewers think. > > > If > > reviewers think that Qiaobin's patch must add the update line, could you > > provide the exact line and location so we avoid going to v6 of this patch? > > In case, I was thinking of something like: > > https://elixir.bootlin.com/linux/v4.18-rc1/source/net/sched/act_ipt.c#L249 > > so, between 'err:' and 'spin_unlock(>tcf_lock)', insert a line like: > > d->tcf_qstats.drop++; I prefer the more complete version. Then it will have a more complete (git) history and help people when troubleshooting. Thanks, Marcelo
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
On Wed, 2018-06-20 at 12:47 -0400, Michel Machado wrote:
> On 06/20/2018 12:08 PM, Davide Caratti wrote:
> > On Tue, 2018-06-12 at 15:42 +, Fu, Qiaobin 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.
> > >
> > > v4:
> > > *Not allow setting flags other than the expected ones.
> > >
> > > *Allow dumping the pure flags.
> > >
> > > Original idea by Jamal Hadi Salim
> > >
> > > Signed-off-by: Qiaobin Fu
> > > Reviewed-by: Michel Machado
> > > ---
> > >
> >
> > [...]
> >
> > > @@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const
> > > struct tc_action *a,
> > >
> > > if (d->flags & SKBEDIT_F_PRIORITY)
> > > skb->priority = d->priority;
> > > + if (d->flags & SKBEDIT_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))
> > > + goto err;
> > > + 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))
> > > + goto err;
> > > + skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
> > > + break;
> > > + }
> > > + }
> > > if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
> > > skb->dev->real_num_tx_queues > d->queue_mapping)
> > > skb_set_queue_mapping(skb, d->queue_mapping);
> > > @@ -53,6 +75,10 @@ static int tcf_skbedit(struct sk_buff *skb, const
> > > struct tc_action *a,
> > >
> > > spin_unlock(>tcf_lock);
> > > return d->tcf_action;
> > > +
> > > +err:
> > > + spin_unlock(>tcf_lock);
> > > + return TC_ACT_SHOT;
> > > }
> > >
> >
> > sorry for asking this when the patch is a v4...
> >
> > I spotted this, as I'm rebasing a small series that removes the tcf_lock
> > from the data plane of skbedit to gain some speed, and it converts the
> > stats to be per-cpu counters.
> >
> > in the code above, you are catching failures of pskb_may_pull(skb) and
> > then you return TC_ACT_SHOT. That's OK, but I think you should update the
> > drop counter, like other TC actions do.
> >
> > If you (author / reviewers) think this is a minor issue, like I do think,
> > then I can add the missing update in my series and post it when net-next
> > reopens.
> >
> > WDYT?
> >
> > thank you in advance!
> > regards,
> >
>
> Hi Davide,
>
> I agree that we should update the drop counter, but given that
> you're already converting the stats to be per-cpu counters, whatever we
> add now will be just symbolic since you're going to change it anyway.
that's ok for me also, as I can use the current v4 code for the rebase
(and not wait for another respin) _ but let's hear what reviewers think.
> If
> reviewers think that Qiaobin's patch must add the update line, could you
> provide the exact line and location so we avoid going to v6 of this patch?
In case, I was thinking of something like:
https://elixir.bootlin.com/linux/v4.18-rc1/source/net/sched/act_ipt.c#L249
so, between 'err:' and 'spin_unlock(>tcf_lock)', insert a line like:
d->tcf_qstats.drop++;
regards,
--
davide
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
On 06/20/2018 12:08 PM, Davide Caratti wrote:
On Tue, 2018-06-12 at 15:42 +, Fu, Qiaobin 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.
v4:
*Not allow setting flags other than the expected ones.
*Allow dumping the pure flags.
Original idea by Jamal Hadi Salim
Signed-off-by: Qiaobin Fu
Reviewed-by: Michel Machado
---
[...]
@@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
tc_action *a,
if (d->flags & SKBEDIT_F_PRIORITY)
skb->priority = d->priority;
+ if (d->flags & SKBEDIT_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))
+ goto err;
+ 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))
+ goto err;
+ skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+ break;
+ }
+ }
if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
skb->dev->real_num_tx_queues > d->queue_mapping)
skb_set_queue_mapping(skb, d->queue_mapping);
@@ -53,6 +75,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
tc_action *a,
spin_unlock(>tcf_lock);
return d->tcf_action;
+
+err:
+ spin_unlock(>tcf_lock);
+ return TC_ACT_SHOT;
}
sorry for asking this when the patch is a v4...
I spotted this, as I'm rebasing a small series that removes the tcf_lock
from the data plane of skbedit to gain some speed, and it converts the
stats to be per-cpu counters.
in the code above, you are catching failures of pskb_may_pull(skb) and
then you return TC_ACT_SHOT. That's OK, but I think you should update the
drop counter, like other TC actions do.
If you (author / reviewers) think this is a minor issue, like I do think,
then I can add the missing update in my series and post it when net-next
reopens.
WDYT?
thank you in advance!
regards,
Hi Davide,
I agree that we should update the drop counter, but given that
you're already converting the stats to be per-cpu counters, whatever we
add now will be just symbolic since you're going to change it anyway. If
reviewers think that Qiaobin's patch must add the update line, could you
provide the exact line and location so we avoid going to v6 of this patch?
[ ]'s
Michel Machado
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
On Tue, 2018-06-12 at 15:42 +, Fu, Qiaobin 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.
>
> v4:
> *Not allow setting flags other than the expected ones.
>
> *Allow dumping the pure flags.
>
> Original idea by Jamal Hadi Salim
>
> Signed-off-by: Qiaobin Fu
> Reviewed-by: Michel Machado
> ---
>
[...]
> @@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
> tc_action *a,
>
> if (d->flags & SKBEDIT_F_PRIORITY)
> skb->priority = d->priority;
> + if (d->flags & SKBEDIT_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))
> + goto err;
> + 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))
> + goto err;
> + skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
> + break;
> + }
> + }
> if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
> skb->dev->real_num_tx_queues > d->queue_mapping)
> skb_set_queue_mapping(skb, d->queue_mapping);
> @@ -53,6 +75,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
> tc_action *a,
>
> spin_unlock(>tcf_lock);
> return d->tcf_action;
> +
> +err:
> + spin_unlock(>tcf_lock);
> + return TC_ACT_SHOT;
> }
>
sorry for asking this when the patch is a v4...
I spotted this, as I'm rebasing a small series that removes the tcf_lock
from the data plane of skbedit to gain some speed, and it converts the
stats to be per-cpu counters.
in the code above, you are catching failures of pskb_may_pull(skb) and
then you return TC_ACT_SHOT. That's OK, but I think you should update the
drop counter, like other TC actions do.
If you (author / reviewers) think this is a minor issue, like I do think,
then I can add the missing update in my series and post it when net-next
reopens.
WDYT?
thank you in advance!
regards,
--
davide
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
On Tue, Jun 12, 2018 at 03:42:55PM +, Fu, Qiaobin 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.
>
> v4:
> *Not allow setting flags other than the expected ones.
>
> *Allow dumping the pure flags.
>
> Original idea by Jamal Hadi Salim
>
> Signed-off-by: Qiaobin Fu
> Reviewed-by: Michel Machado
Reviewed-by: Marcelo Ricardo Leitner
> ---
>
> 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_skbedit.h
> b/include/uapi/linux/tc_act/tc_skbedit.h
> index fbcfe27a4e6c..6de6071ebed6 100644
> --- a/include/uapi/linux/tc_act/tc_skbedit.h
> +++ b/include/uapi/linux/tc_act/tc_skbedit.h
> @@ -30,6 +30,7 @@
> #define SKBEDIT_F_MARK 0x4
> #define SKBEDIT_F_PTYPE 0x8
> #define SKBEDIT_F_MASK 0x10
> +#define SKBEDIT_F_INHERITDSFIELD 0x20
>
> struct tc_skbedit {
> tc_gen;
> @@ -45,6 +46,7 @@ enum {
> TCA_SKBEDIT_PAD,
> TCA_SKBEDIT_PTYPE,
> TCA_SKBEDIT_MASK,
> + TCA_SKBEDIT_FLAGS,
> __TCA_SKBEDIT_MAX
> };
> #define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index 6138d1d71900..9adbcfa3f5fe 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -23,6 +23,9 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> #include
> #include
> @@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
> tc_action *a,
>
> if (d->flags & SKBEDIT_F_PRIORITY)
> skb->priority = d->priority;
> + if (d->flags & SKBEDIT_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))
> + goto err;
> + 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))
> + goto err;
> + skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
> + break;
> + }
> + }
> if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
> skb->dev->real_num_tx_queues > d->queue_mapping)
> skb_set_queue_mapping(skb, d->queue_mapping);
> @@ -53,6 +75,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
> tc_action *a,
>
> spin_unlock(>tcf_lock);
> return d->tcf_action;
> +
> +err:
> + spin_unlock(>tcf_lock);
> + return TC_ACT_SHOT;
> }
>
> static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
> @@ -62,6 +88,7 @@ static const struct nla_policy
> skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
> [TCA_SKBEDIT_MARK] = { .len = sizeof(u32) },
> [TCA_SKBEDIT_PTYPE] = { .len = sizeof(u16) },
> [TCA_SKBEDIT_MASK] = { .len = sizeof(u32) },
> + [TCA_SKBEDIT_FLAGS] = { .len = sizeof(u64) },
> };
>
> static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> @@ -73,6 +100,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr
> *nla,
> struct tc_skbedit *parm;
> struct tcf_skbedit *d;
> u32 flags = 0, *priority = NULL, *mark = NULL, *mask = NULL;
> + u64 *pure_flags = NULL;
> u16 *queue_mapping = NULL, *ptype = NULL;
> bool exists = false;
> int ret = 0, err;
> @@ -114,6 +142,12 @@ static int tcf_skbedit_init(struct net *net, struct
> nlattr *nla,
> mask = nla_data(tb[TCA_SKBEDIT_MASK]);
> }
>
> + if (tb[TCA_SKBEDIT_FLAGS] != NULL) {
> + pure_flags = nla_data(tb[TCA_SKBEDIT_FLAGS]);
> + if (*pure_flags & SKBEDIT_F_INHERITDSFIELD)
> + flags |= SKBEDIT_F_INHERITDSFIELD;
> + }
> +
> parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
>
> exists = tcf_idr_check(tn, parm->index, a, bind);
> @@ -178,6 +212,7 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct
> tc_action *a,
> .action = d->tcf_action,
> };
> struct tcf_t t;
> + u64 pure_flags = 0;
>
> if (nla_put(skb, TCA_SKBEDIT_PARMS, sizeof(opt), ))
> goto nla_put_failure;
> @@ -196,6 +231,11 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct
> tc_action *a,
> if ((d->flags & SKBEDIT_F_MASK) &&
> nla_put_u32(skb, TCA_SKBEDIT_MASK, d->mask))
> goto nla_put_failure;
> + if (d->flags & SKBEDIT_F_INHERITDSFIELD)
> +
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
On 06/20/2018 07:53 AM, Jamal Hadi Salim wrote: On 19/06/18 08:39 AM, Michel Machado wrote: Notice that, different from skbmod, there's no field parm->flags in skbedit. Skbedit infers the flags in d->flags from the presence of the parameters of each of its actions. But SKBEDIT_F_INHERITDSFIELD has no parameter and adding field parm->flags breaks backward compatibility with user space as pointed out by Marcelo Ricardo Leitner. Our solution was to add TCA_SKBEDIT_FLAGS, so SKBEDIT_F_INHERITDSFIELD and future flag-only actions can be added. Ok, that makes sense - thanks. I am not so sure about using 64 bits (32 bits would have been fine to match the size of the kernel flags), but other than that LGTM. The choice for the u64 is meant to keep the interface between kernel and user space the same for hopefully a longer time than it would be with a u32. Changing from u32 to u64 in the kernel, when the need arrives, won't impact applications. This interface choice was motivated by the backward compatibility issue mentioned above. Thank you for the review, Jamal. [ ]'s Michel Machado
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
On 19/06/18 08:39 AM, Michel Machado wrote: Notice that, different from skbmod, there's no field parm->flags in skbedit. Skbedit infers the flags in d->flags from the presence of the parameters of each of its actions. But SKBEDIT_F_INHERITDSFIELD has no parameter and adding field parm->flags breaks backward compatibility with user space as pointed out by Marcelo Ricardo Leitner. Our solution was to add TCA_SKBEDIT_FLAGS, so SKBEDIT_F_INHERITDSFIELD and future flag-only actions can be added. Ok, that makes sense - thanks. I am not so sure about using 64 bits (32 bits would have been fine to match the size of the kernel flags), but other than that LGTM. Acked-by: Jamal Hadi Salim cheers, jamal
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
On 06/19/2018 08:01 AM, Jamal Hadi Salim wrote: Per my previous comments, why do we need the TCA_SKBEDIT_FLAGS TLV? Isnt SKBEDIT_F_INHERITDSFIELD sufficient? i.e in tcf_skbedit_init() check for d->flags_F_INHERITDSFIELD then set skb->priority and flags|=SKBEDIT_F_INHERITDSFIELD Notice that, different from skbmod, there's no field parm->flags in skbedit. Skbedit infers the flags in d->flags from the presence of the parameters of each of its actions. But SKBEDIT_F_INHERITDSFIELD has no parameter and adding field parm->flags breaks backward compatibility with user space as pointed out by Marcelo Ricardo Leitner. Our solution was to add TCA_SKBEDIT_FLAGS, so SKBEDIT_F_INHERITDSFIELD and future flag-only actions can be added. [ ]'s Michel Machado
Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
Hi Qiaobin,
Per my previous comments, why do we need the
TCA_SKBEDIT_FLAGS TLV? Isnt SKBEDIT_F_INHERITDSFIELD
sufficient? i.e in tcf_skbedit_init() check for
d->flags_F_INHERITDSFIELD then set skb->priority
and flags|=SKBEDIT_F_INHERITDSFIELD
Side note:
Infact the whole flags setting in the init function
seems to be a redundant given all the TLVs have
d->flags also set by user space.
But thats a different patch.
cheers,
jamal
On 12/06/18 11:42 AM, Fu, Qiaobin 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.
v4:
*Not allow setting flags other than the expected ones.
*Allow dumping the pure flags.
Original idea by Jamal Hadi Salim
Signed-off-by: Qiaobin Fu
Reviewed-by: Michel Machado
---
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_skbedit.h
b/include/uapi/linux/tc_act/tc_skbedit.h
index fbcfe27a4e6c..6de6071ebed6 100644
--- a/include/uapi/linux/tc_act/tc_skbedit.h
+++ b/include/uapi/linux/tc_act/tc_skbedit.h
@@ -30,6 +30,7 @@
#define SKBEDIT_F_MARK0x4
#define SKBEDIT_F_PTYPE 0x8
#define SKBEDIT_F_MASK0x10
+#define SKBEDIT_F_INHERITDSFIELD 0x20
struct tc_skbedit {
tc_gen;
@@ -45,6 +46,7 @@ enum {
TCA_SKBEDIT_PAD,
TCA_SKBEDIT_PTYPE,
TCA_SKBEDIT_MASK,
+ TCA_SKBEDIT_FLAGS,
__TCA_SKBEDIT_MAX
};
#define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 6138d1d71900..9adbcfa3f5fe 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -23,6 +23,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
tc_action *a,
if (d->flags & SKBEDIT_F_PRIORITY)
skb->priority = d->priority;
+ if (d->flags & SKBEDIT_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))
+ goto err;
+ 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))
+ goto err;
+ skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
+ break;
+ }
+ }
if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
skb->dev->real_num_tx_queues > d->queue_mapping)
skb_set_queue_mapping(skb, d->queue_mapping);
@@ -53,6 +75,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct
tc_action *a,
spin_unlock(>tcf_lock);
return d->tcf_action;
+
+err:
+ spin_unlock(>tcf_lock);
+ return TC_ACT_SHOT;
}
static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
@@ -62,6 +88,7 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX
+ 1] = {
[TCA_SKBEDIT_MARK] = { .len = sizeof(u32) },
[TCA_SKBEDIT_PTYPE] = { .len = sizeof(u16) },
[TCA_SKBEDIT_MASK] = { .len = sizeof(u32) },
+ [TCA_SKBEDIT_FLAGS] = { .len = sizeof(u64) },
};
static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
@@ -73,6 +100,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr
*nla,
struct tc_skbedit *parm;
struct tcf_skbedit *d;
u32 flags = 0, *priority = NULL, *mark = NULL, *mask = NULL;
+ u64 *pure_flags = NULL;
u16 *queue_mapping = NULL, *ptype = NULL;
bool exists = false;
int ret = 0, err;
@@ -114,6 +142,12 @@ static int tcf_skbedit_init(struct net *net, struct nlattr
*nla,
mask = nla_data(tb[TCA_SKBEDIT_MASK]);
}
+ if (tb[TCA_SKBEDIT_FLAGS] != NULL) {
+ pure_flags = nla_data(tb[TCA_SKBEDIT_FLAGS]);
+ if (*pure_flags & SKBEDIT_F_INHERITDSFIELD)
+ flags |= SKBEDIT_F_INHERITDSFIELD;
+ }
+
parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
exists = tcf_idr_check(tn, parm->index, a, bind);
@@ -178,6 +212,7 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct
tc_action *a,
.action = d->tcf_action,
};
struct tcf_t t;
+ u64 pure_flags = 0;
if (nla_put(skb, TCA_SKBEDIT_PARMS, sizeof(opt), ))
goto nla_put_failure;
@@ -196,6 +231,11 @@ static int
