netdev - Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180620144541.GA6106@localhost.localdomain>
Date:   Wed, 20 Jun 2018 11:45:41 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     "Fu, Qiaobin" <qiaobinf@...edu>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        Michel Machado <michel@...irati.com.br>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
Subject: Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to
 skbedit

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@...atatu.com>
> 
> Signed-off-by: Qiaobin Fu <qiaobinf@...edu>
> Reviewed-by: Michel Machado <michel@...irati.com.br>

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.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))