netdev - Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Jul 2018 08:16:12 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Daniel Borkmann <daniel@...earbox.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Eyal Birger <eyal.birger@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Shmulik Ladkani <shmulik.ladkani@...il.com>
Subject: Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

+Cc Shmulik

Paolo - please also run the tdc tests (and add anymore if you
feel they dont do coverage to your changes)

On 24/07/18 04:06 PM, Paolo Abeni wrote:
> This is similar TC_ACT_REDIRECT, but with a slightly different
> semantic:
> - on ingress the mirred skbs are passed to the target device
> network stack without any additional check not scrubbing.
> - the rcu-protected stats provided via the tcf_result struct
>    are updated on error conditions.
> 
> This new tcfa_action value is not exposed to the user-space
> and can be used only internally by clsact.
> 
> v1 -> v2: do not touch TC_ACT_REDIRECT code path, introduce
>   a new action type instead
> 
> v2 -> v3:
>   - rename the new action value TC_ACT_REINJECT, update the
>     helper accordingly
>   - take care of uncloned reinjected packets in XDP generic
>     hook
> 
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>   include/net/pkt_cls.h     |  3 +++
>   include/net/sch_generic.h | 19 +++++++++++++++++++
>   net/core/dev.c            |  6 +++++-
>   3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 2081e4219f81..36ccfe2a303a 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -7,6 +7,9 @@
>   #include <net/sch_generic.h>
>   #include <net/act_api.h>
>   
> +/* TC action not accessible from user space */
> +#define TC_ACT_REINJECT		(TC_ACT_VALUE_MAX + 1)

Lets say in the future we add a new opcode.
Will old kernel, new iproute2 (new value) work?
Maybe use a negative number below -1.
I am honestly still unclear about introducing this new
code.
Could you not use the result code to carry sufficient
info to indicate the intent?

cheers,
jamal

> +
>   /* Basic packet classifier frontend definitions. */
>   
>   struct tcf_walker {
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 056dc1083aa3..95e81a70f549 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -235,6 +235,12 @@ struct tcf_result {
>   			u32		classid;
>   		};
>   		const struct tcf_proto *goto_tp;
> +
> +		/* used by the TC_ACT_REINJECT action */
> +		struct {
> +			bool		ingress;
> +			struct gnet_stats_queue *qstats;
> +		};
>   	};
>   };
>   
> @@ -1091,4 +1097,17 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
>   void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
>   			  struct mini_Qdisc __rcu **p_miniq);
>   
> +static inline void skb_tc_reinject(struct sk_buff *skb, struct tcf_result *res)
> +{
> +	struct gnet_stats_queue *stats = res->qstats;
> +	int ret;
> +
> +	if (res->ingress)
> +		ret = netif_receive_skb(skb);
> +	else
> +		ret = dev_queue_xmit(skb);
> +	if (ret && stats)
> +		qstats_overlimit_inc(res->qstats);
> +}
> +
>   #endif
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 14a748ee8cc9..826ec74fe1d9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4252,7 +4252,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>   	/* Reinjected packets coming from act_mirred or similar should
>   	 * not get XDP generic processing.
>   	 */
> -	if (skb_cloned(skb))
> +	if (skb_cloned(skb) || skb->tc_redirected)
>   		return XDP_PASS;
>   
>   	/* XDP packets must be linear and must have sufficient headroom
> @@ -4602,6 +4602,10 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
>   		__skb_push(skb, skb->mac_len);
>   		skb_do_redirect(skb);
>   		return NULL;
> +	case TC_ACT_REINJECT:
> +		/* this does not scrub the packet, and updates stats on error */
> +		skb_tc_reinject(skb, &cl_res);
> +		return NULL;
>   	default:
>   		break;
>   	}
>