On Fri, 24 May 2019 17:05:46 +0100
John Hurley <john.hur...@netronome.com> wrote:
> TC hooks allow the application of filters and actions to packets at both
> ingress and egress of the network stack. It is possible, with poor
> configuration, that this can produce loops whereby an ingress hook calls
> a mirred egress action that has an egress hook that redirects back to
> the first ingress etc. The TC core classifier protects against loops when
> doing reclassifies but, as yet, there is no protection against a packet
> looping between multiple hooks. This can lead to stack overflow panics.
>
> Add a per cpu counter that tracks recursion of packets through TC hooks.
> The packet will be dropped if a recursive limit is passed and the counter
> reset for the next packet.
>
> Signed-off-by: John Hurley <john.hur...@netronome.com>
> Reviewed-by: Simon Horman <simon.hor...@netronome.com>
> ---
> net/core/dev.c | 62
> +++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b6b8505..a6d9ed7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -154,6 +154,9 @@
> /* This should be increased if a protocol with a bigger head is added. */
> #define GRO_MAX_HEAD (MAX_HEADER + 128)
>
> +#define SCH_RECURSION_LIMIT 4
> +static DEFINE_PER_CPU(int, sch_recursion_level);
Maybe use unsigned instead of int?
> +
> static DEFINE_SPINLOCK(ptype_lock);
> static DEFINE_SPINLOCK(offload_lock);
> struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
> @@ -3598,16 +3601,42 @@ int dev_loopback_xmit(struct net *net, struct sock
> *sk, struct sk_buff *skb)
> }
> EXPORT_SYMBOL(dev_loopback_xmit);
>
> +static inline int sch_check_inc_recur_level(void)
> +{
> + int rec_level = __this_cpu_inc_return(sch_recursion_level);
> +
> + if (rec_level >= SCH_RECURSION_LIMIT) {
unlikely here?
> + net_warn_ratelimited("Recursion limit reached on TC datapath,
> probable configuration error\n");
It would be good to know which device this was on.
> + return -ELOOP;
> + }
> +
> + return 0;
> +}
> +
> +static inline void sch_dec_recur_level(void)
> +{
> + __this_cpu_dec(sch_recursion_level);
Decrement of past 0 is an error that should be trapped and logged.
> +}
> +
> #ifdef CONFIG_NET_EGRESS
> static struct sk_buff *
> sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
> {
> struct mini_Qdisc *miniq = rcu_dereference_bh(dev->miniq_egress);
> struct tcf_result cl_res;
> + int err;
>
> if (!miniq)
> return skb;
>
> + err = sch_check_inc_recur_level();
> + if (err) {
> + sch_dec_recur_level();
You should have sch_check_inc_recur_level do the unwind on error.
That would simplify the error paths.
> + *ret = NET_XMIT_DROP;
> + consume_skb(skb);
> + return NULL;
> + }
> +
