netdev - Re: [patch net-next v4 05/10] net: sched: keep track of offloaded filters and check tc offload feature

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]

Date:   Sat, 23 Dec 2017 18:20:45 -0800
From:   Jakub Kicinski <kubakici@...pl>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jhs@...atatu.com,
        xiyou.wangcong@...il.com, mlxsw@...lanox.com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        michael.chan@...adcom.com, ganeshgr@...lsio.com,
        saeedm@...lanox.com, matanb@...lanox.com, leonro@...lanox.com,
        idosch@...lanox.com, simon.horman@...ronome.com,
        pieter.jansenvanvuuren@...ronome.com, john.hurley@...ronome.com,
        alexander.h.duyck@...el.com, ogerlitz@...lanox.com,
        john.fastabend@...il.com, daniel@...earbox.net, dsahern@...il.com
Subject: Re: [patch net-next v4 05/10] net: sched: keep track of offloaded
 filters and check tc offload feature

On Sat, 23 Dec 2017 16:54:31 +0100, Jiri Pirko wrote:
> -static void tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
> -				   struct tcf_block_ext_info *ei)
> +static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
> +				  struct tcf_block_ext_info *ei)
>  {
> -	tcf_block_offload_cmd(block, q, ei, TC_BLOCK_BIND);
> +	struct net_device *dev = q->dev_queue->dev;
> +	int err;
> +
> +	if (!dev->netdev_ops->ndo_setup_tc)
> +		return 0;
> +
> +	/* If tc offload feature is disabled and the block we try to bind
> +	 * to already has some offloaded filters, forbid to bind.
> +	 */
> +	if (!tc_can_offload(dev) && tcf_block_offload_in_use(block))
> +		return -EOPNOTSUPP;
> +
> +	err = tcf_block_offload_cmd(block, dev, ei, TC_BLOCK_BIND);
> +	if (err == -EOPNOTSUPP)
> +		/* Driver does not support binding. */
> +		return 0;
> +	return err;
>  }

Would you mind explaining why those return 0s are safe?

Say I have 2 netdevs, one dumb NIC without ndo_setup_tc (dnic) and one
NIC that can offload everything (enic).  I can share a block between
them (before or after adding any filters) and adding filters with
skip_sw will succeed, even though dnic will not see them ever.  There
is only one callback for enic, "all callbacks" != "all devices". 
It's fine to share the block in such case, but that block can never
accept a skip_sw filter.  Don't we need something like (reverse) patch 3
for keeping track of netdevs sharing the block which are not OK with
offloads?

Am I misunderstanding how this is supposed to work?  Or simply too nit
picky about providing predictable behaviour?

Quick hack to illustrate the idea (untested):

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 22a3a1d22ffa..e61e59161243 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -289,6 +289,7 @@ struct tcf_block {
        struct list_head cb_list;
        struct list_head owner_list;
        bool keep_dst;
+       bool nonoffload_taint;
        unsigned int offloadcnt;
 };
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 37eea70d1d72..4e017cbbf356 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -290,7 +290,7 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
        int err;
 
        if (!dev->netdev_ops->ndo_setup_tc)
-               return 0;
+               goto mark_no_offload;
 
        /* If tc offload feature is disabled and the block we try to bind
         * to already has some offloaded filters, forbid to bind.
@@ -300,9 +300,14 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
 
        err = tcf_block_offload_cmd(block, dev, ei, TC_BLOCK_BIND);
        if (err == -EOPNOTSUPP)
-               /* Driver does not support binding. */
-               return 0;
+               goto mark_no_offload;
        return err;
+
+mark_no_offload:
+       if (tcf_block_offload_in_use(block))
+               return -EOPNOTSUPP;
+       block->nonoffload_taint = true;
+       return 0;
 }
 
 static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
@@ -1492,6 +1497,10 @@ int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
        int ok_count;
        int ret;
 
+       /* Make sure all netdevs sharing this block are offload-capable */
+       if (block->nonoffload_taint && err_stop)
+               return -EOPNOTSUPP;
+
        ret = tcf_block_cb_call(block, type, type_data, err_stop);
        if (ret < 0)
                return ret;


Here a block once tainted with a bad netdev will never be offloadable
again, so tracking a'la patch 3 would be nicer..