netdev - Re: [PATCH net-next,v4 07/12] net: sched: use flow block API

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: <b45709c7-38b5-2dcb-3db1-0c2fca1840be@solarflare.com>
Date:   Wed, 14 Aug 2019 17:32:34 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
CC:     netdev <netdev@...r.kernel.org>, <netfilter-devel@...r.kernel.org>
Subject: Re: [PATCH net-next,v4 07/12] net: sched: use flow block API

On 09/07/2019 21:55, Pablo Neira Ayuso wrote:
> This patch adds tcf_block_setup() which uses the flow block API.
>
> This infrastructure takes the flow block callbacks coming from the
> driver and register/unregister to/from the cls_api core.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> ---
> <snip>
> @@ -796,13 +804,20 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
>  				 struct netlink_ext_ack *extack)
>  {
>  	struct tc_block_offload bo = {};
> +	int err;
>  
>  	bo.net = dev_net(dev);
>  	bo.command = command;
>  	bo.binder_type = ei->binder_type;
>  	bo.block = block;
>  	bo.extack = extack;
> -	return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK, &bo);
> +	INIT_LIST_HEAD(&bo.cb_list);
> +
> +	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK, &bo);
> +	if (err < 0)
> +		return err;
> +
> +	return tcf_block_setup(block, &bo);
>  }
>  
>  static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
> @@ -1636,6 +1651,77 @@ void tcf_block_cb_unregister(struct tcf_block *block,
>  }
>  EXPORT_SYMBOL(tcf_block_cb_unregister);
>  
> +static int tcf_block_bind(struct tcf_block *block,
> +			  struct flow_block_offload *bo)
> +{
> +	struct flow_block_cb *block_cb, *next;
> +	int err, i = 0;
> +
> +	list_for_each_entry(block_cb, &bo->cb_list, list) {
> +		err = tcf_block_playback_offloads(block, block_cb->cb,
> +						  block_cb->cb_priv, true,
> +						  tcf_block_offload_in_use(block),
> +						  bo->extack);
> +		if (err)
> +			goto err_unroll;
> +
> +		i++;
> +	}
> +	list_splice(&bo->cb_list, &block->cb_list);
> +
> +	return 0;
> +
> +err_unroll:
> +	list_for_each_entry_safe(block_cb, next, &bo->cb_list, list) {
> +		if (i-- > 0) {
> +			list_del(&block_cb->list);
> +			tcf_block_playback_offloads(block, block_cb->cb,
> +						    block_cb->cb_priv, false,
> +						    tcf_block_offload_in_use(block),
> +						    NULL);
> +		}
> +		flow_block_cb_free(block_cb);
> +	}
> +
> +	return err;
> +}
Why has the replay been moved from the function called by the driver
 (__tcf_block_cb_register()) to work done by the driver's caller based on
 what the driver has left on this flow_block_offload.cb_list?  This makes
 it impossible for the driver to (say) unregister a block outside of an
 explicit request from ndo_setup_tc().
In my under-development driver, I have a teardown path called on PCI
 remove, which calls tcf_block_cb_unregister() on all my block bindings
 (of which the driver keeps track), to ensure that no flow rules are still
 in place when unregister_netdev() is called; this is needed because some
 of the driver's state for certain rules involves taking a reference on
 the netdevice (dev_hold()).  Your structural changes here make that
 impossible; is there any reason why they're necessary?

-Ed