* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-12 15:42 ` [PATCH v4 " Fu, Qiaobin
@ 2018-06-19 12:01 ` Jamal Hadi Salim
2018-06-19 12:39 ` Michel Machado
2018-06-20 14:45 ` Marcelo Ricardo Leitner
2018-06-20 16:08 ` Davide Caratti
2 siblings, 1 reply; 20+ messages in thread
From: Jamal Hadi Salim @ 2018-06-19 12:01 UTC (permalink / raw)
To: Fu, Qiaobin, davem
Cc: netdev, Michel Machado, Marcelo Ricardo Leitner, xiyou.wangcong
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&SKBEDIT_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 <jhs@mojatatu.com>
>
> Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
> Reviewed-by: Michel Machado <michel@digirati.com.br>
> ---
>
> 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 <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_skbedit.h>
> #include <net/tc_act/tc_skbedit.h>
> @@ -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(&d->tcf_lock);
> return d->tcf_action;
> +
> +err:
> + spin_unlock(&d->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), &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)
> + pure_flags |= SKBEDIT_F_INHERITDSFIELD;
> + if (pure_flags != 0 &&
> + nla_put(skb, TCA_SKBEDIT_FLAGS, sizeof(pure_flags), &pure_flags))
> + goto nla_put_failure;
>
> tcf_tm_dump(&t, &d->tcf_tm);
> if (nla_put_64bit(skb, TCA_SKBEDIT_TM, sizeof(t), &t, TCA_SKBEDIT_PAD))
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-19 12:01 ` Jamal Hadi Salim
@ 2018-06-19 12:39 ` Michel Machado
2018-06-20 11:53 ` Jamal Hadi Salim
0 siblings, 1 reply; 20+ messages in thread
From: Michel Machado @ 2018-06-19 12:39 UTC (permalink / raw)
To: Jamal Hadi Salim, Fu, Qiaobin, davem
Cc: netdev, Marcelo Ricardo Leitner, xiyou.wangcong
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&SKBEDIT_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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-19 12:39 ` Michel Machado
@ 2018-06-20 11:53 ` Jamal Hadi Salim
2018-06-20 12:42 ` Michel Machado
0 siblings, 1 reply; 20+ messages in thread
From: Jamal Hadi Salim @ 2018-06-20 11:53 UTC (permalink / raw)
To: Michel Machado, Fu, Qiaobin, davem
Cc: netdev, Marcelo Ricardo Leitner, xiyou.wangcong
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 <jhs@mojatatu.com>
cheers,
jamal
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-20 11:53 ` Jamal Hadi Salim
@ 2018-06-20 12:42 ` Michel Machado
0 siblings, 0 replies; 20+ messages in thread
From: Michel Machado @ 2018-06-20 12:42 UTC (permalink / raw)
To: Jamal Hadi Salim, Fu, Qiaobin, davem
Cc: netdev, Marcelo Ricardo Leitner, xiyou.wangcong
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-12 15:42 ` [PATCH v4 " Fu, Qiaobin
2018-06-19 12:01 ` Jamal Hadi Salim
@ 2018-06-20 14:45 ` Marcelo Ricardo Leitner
2018-06-20 16:08 ` Davide Caratti
2 siblings, 0 replies; 20+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-06-20 14:45 UTC (permalink / raw)
To: Fu, Qiaobin; +Cc: davem, netdev, jhs, Michel Machado, xiyou.wangcong
On Tue, Jun 12, 2018 at 03:42:55PM +0000, 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 <jhs@mojatatu.com>
>
> Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
> Reviewed-by: Michel Machado <michel@digirati.com.br>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
>
> 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 <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_skbedit.h>
> #include <net/tc_act/tc_skbedit.h>
> @@ -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(&d->tcf_lock);
> return d->tcf_action;
> +
> +err:
> + spin_unlock(&d->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), &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)
> + pure_flags |= SKBEDIT_F_INHERITDSFIELD;
> + if (pure_flags != 0 &&
> + nla_put(skb, TCA_SKBEDIT_FLAGS, sizeof(pure_flags), &pure_flags))
> + goto nla_put_failure;
>
> tcf_tm_dump(&t, &d->tcf_tm);
> if (nla_put_64bit(skb, TCA_SKBEDIT_TM, sizeof(t), &t, TCA_SKBEDIT_PAD))
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-12 15:42 ` [PATCH v4 " Fu, Qiaobin
2018-06-19 12:01 ` Jamal Hadi Salim
2018-06-20 14:45 ` Marcelo Ricardo Leitner
@ 2018-06-20 16:08 ` Davide Caratti
2018-06-20 16:47 ` Michel Machado
2 siblings, 1 reply; 20+ messages in thread
From: Davide Caratti @ 2018-06-20 16:08 UTC (permalink / raw)
To: Fu, Qiaobin, davem
Cc: netdev, jhs, Michel Machado, Marcelo Ricardo Leitner, xiyou.wangcong
On Tue, 2018-06-12 at 15:42 +0000, 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 <jhs@mojatatu.com>
>
> Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
> Reviewed-by: Michel Machado <michel@digirati.com.br>
> ---
>
[...]
> @@ -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(&d->tcf_lock);
> return d->tcf_action;
> +
> +err:
> + spin_unlock(&d->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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-20 16:08 ` Davide Caratti
@ 2018-06-20 16:47 ` Michel Machado
2018-06-20 17:02 ` Davide Caratti
0 siblings, 1 reply; 20+ messages in thread
From: Michel Machado @ 2018-06-20 16:47 UTC (permalink / raw)
To: Davide Caratti, Fu, Qiaobin, davem
Cc: netdev, jhs, Marcelo Ricardo Leitner, xiyou.wangcong
On 06/20/2018 12:08 PM, Davide Caratti wrote:
> On Tue, 2018-06-12 at 15:42 +0000, 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 <jhs@mojatatu.com>
>>
>> Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
>> Reviewed-by: Michel Machado <michel@digirati.com.br>
>> ---
>>
>
> [...]
>
>> @@ -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(&d->tcf_lock);
>> return d->tcf_action;
>> +
>> +err:
>> + spin_unlock(&d->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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-20 16:47 ` Michel Machado
@ 2018-06-20 17:02 ` Davide Caratti
2018-06-20 18:40 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 20+ messages in thread
From: Davide Caratti @ 2018-06-20 17:02 UTC (permalink / raw)
To: Michel Machado, Fu, Qiaobin, davem
Cc: netdev, jhs, Marcelo Ricardo Leitner, xiyou.wangcong
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 +0000, 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 <jhs@mojatatu.com>
> > >
> > > Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
> > > Reviewed-by: Michel Machado <michel@digirati.com.br>
> > > ---
> > >
> >
> > [...]
> >
> > > @@ -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(&d->tcf_lock);
> > > return d->tcf_action;
> > > +
> > > +err:
> > > + spin_unlock(&d->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(&d->tcf_lock)', insert a line like:
d->tcf_qstats.drop++;
regards,
--
davide
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-20 17:02 ` Davide Caratti
@ 2018-06-20 18:40 ` Marcelo Ricardo Leitner
2018-06-21 15:46 ` Fu, Qiaobin
0 siblings, 1 reply; 20+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-06-20 18:40 UTC (permalink / raw)
To: Davide Caratti
Cc: Michel Machado, Fu, Qiaobin, davem, netdev, jhs, xiyou.wangcong
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(&d->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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-20 18:40 ` Marcelo Ricardo Leitner
@ 2018-06-21 15:46 ` Fu, Qiaobin
2018-06-21 15:50 ` [PATCH v5 " Fu, Qiaobin
0 siblings, 1 reply; 20+ messages in thread
From: Fu, Qiaobin @ 2018-06-21 15:46 UTC (permalink / raw)
To: davem
Cc: Marcelo Ricardo Leitner, Davide Caratti, Michel Machado, netdev,
jhs, xiyou.wangcong
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 <jhs@mojatatu.com>
Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
Reviewed-by: Michel Machado <michel@digirati.com.br>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
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..dfaf5d8028dd 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -23,6 +23,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_skbedit.h>
#include <net/tc_act/tc_skbedit.h>
@@ -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(&d->tcf_lock);
return d->tcf_action;
+
+err:
+ d->tcf_qstats.drops++;
+ spin_unlock(&d->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), &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), &pure_flags))
+ goto nla_put_failure;
tcf_tm_dump(&t, &d->tcf_tm);
if (nla_put_64bit(skb, TCA_SKBEDIT_TM, sizeof(t), &t, TCA_SKBEDIT_PAD))
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-21 15:46 ` Fu, Qiaobin
@ 2018-06-21 15:50 ` Fu, Qiaobin
2018-06-21 16:13 ` Davide Caratti
2018-06-26 15:58 ` Fu, Qiaobin
0 siblings, 2 replies; 20+ messages in thread
From: Fu, Qiaobin @ 2018-06-21 15:50 UTC (permalink / raw)
To: davem
Cc: Marcelo Ricardo Leitner, Davide Caratti, Michel Machado, netdev,
jhs, xiyou.wangcong
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 <jhs@mojatatu.com>
Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
Reviewed-by: Michel Machado <michel@digirati.com.br>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
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..dfaf5d8028dd 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -23,6 +23,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_skbedit.h>
#include <net/tc_act/tc_skbedit.h>
@@ -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(&d->tcf_lock);
return d->tcf_action;
+
+err:
+ d->tcf_qstats.drops++;
+ spin_unlock(&d->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), &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), &pure_flags))
+ goto nla_put_failure;
tcf_tm_dump(&t, &d->tcf_tm);
if (nla_put_64bit(skb, TCA_SKBEDIT_TM, sizeof(t), &t, TCA_SKBEDIT_PAD))
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-21 15:50 ` [PATCH v5 " Fu, Qiaobin
@ 2018-06-21 16:13 ` Davide Caratti
2018-06-26 15:58 ` Fu, Qiaobin
1 sibling, 0 replies; 20+ messages in thread
From: Davide Caratti @ 2018-06-21 16:13 UTC (permalink / raw)
To: Fu, Qiaobin, davem
Cc: Marcelo Ricardo Leitner, Michel Machado, netdev, jhs, xiyou.wangcong
On Thu, 2018-06-21 at 15:50 +0000, 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.
>
> v5:
> *Update the drop counter for TC_ACT_SHOT
Acked-by: Davide Caratti <dcaratti@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-21 15:50 ` [PATCH v5 " Fu, Qiaobin
2018-06-21 16:13 ` Davide Caratti
@ 2018-06-26 15:58 ` Fu, Qiaobin
2018-06-28 7:05 ` David Miller
1 sibling, 1 reply; 20+ messages in thread
From: Fu, Qiaobin @ 2018-06-26 15:58 UTC (permalink / raw)
To: davem
Cc: Marcelo Ricardo Leitner, Davide Caratti, Michel Machado, netdev,
jhs, xiyou.wangcong
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 <jhs@mojatatu.com>
Signed-off-by: Qiaobin Fu <qiaobinf@bu.edu>
Reviewed-by: Michel Machado <michel@digirati.com.br>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Acked-by: Davide Caratti <dcaratti@redhat.com>
---
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..dfaf5d8028dd 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -23,6 +23,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_skbedit.h>
#include <net/tc_act/tc_skbedit.h>
@@ -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(&d->tcf_lock);
return d->tcf_action;
+
+err:
+ d->tcf_qstats.drops++;
+ spin_unlock(&d->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), &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), &pure_flags))
+ goto nla_put_failure;
tcf_tm_dump(&t, &d->tcf_tm);
if (nla_put_64bit(skb, TCA_SKBEDIT_TM, sizeof(t), &t, TCA_SKBEDIT_PAD))
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 net-next] net:sched: add action inheritdsfield to skbedit
2018-06-26 15:58 ` Fu, Qiaobin
@ 2018-06-28 7:05 ` David Miller
0 siblings, 0 replies; 20+ messages in thread
From: David Miller @ 2018-06-28 7:05 UTC (permalink / raw)
To: qiaobinf; +Cc: marcelo.leitner, dcaratti, michel, netdev, jhs, xiyou.wangcong
From: "Fu, Qiaobin" <qiaobinf@bu.edu>
Date: Tue, 26 Jun 2018 15:58:35 +0000
> 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
This patch is heavily corrupted by your email client.
Please fix this, email a test patch to yourself, and do not resubmit
this patch to the list until you can successfully apply the patch
in that test email.
^ permalink raw reply [flat|nested] 20+ messages in thread