netdev - Re: [PATCH net-next,RFC 7/9] net: use tcf_block

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: <20190426161216.36i6aqv3hdk3jwnj@salvia>
Date:   Fri, 26 Apr 2019 18:12:16 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org, jiri@...lanox.com,
        john.hurley@...ronome.com, jakub.kicinski@...ronome.com,
        ogerlitz@...lanox.com
Subject: Re: [PATCH net-next,RFC 7/9] net: use tcf_block_setup()
 infrastructure

On Fri, Apr 26, 2019 at 04:27:15PM +0200, Jiri Pirko wrote:
> Fri, Apr 26, 2019 at 02:33:44AM CEST, pablo@...filter.org wrote:
> >This allows us to register / unregister tcf_block_cb objects from the
> >core. The idea is to allocate the tcf_block_cb object from the driver,
> >attach it to the tc_block_offload->cb_list, then the core registers
> >them.
> >
> >Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> >---
> 
> [...]
> 
> 
> >--- a/net/dsa/slave.c
> >+++ b/net/dsa/slave.c
> >@@ -902,6 +902,7 @@ static int dsa_slave_setup_tc_block_cb_eg(enum tc_setup_type type,
> > static int dsa_slave_setup_tc_block(struct net_device *dev,
> > 				    struct tc_block_offload *f)
> > {
> >+	struct tcf_block_cb *block_cb;
> > 	tc_setup_cb_t *cb;
> > 
> > 	if (f->binder_type == TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
> >@@ -913,9 +914,19 @@ static int dsa_slave_setup_tc_block(struct net_device *dev,
> > 
> > 	switch (f->command) {
> > 	case TC_BLOCK_BIND:
> >-		return tcf_block_cb_register(f->block, cb, dev, dev, f->extack);
> >+		block_cb = tcf_block_cb_alloc(f->block->index, cb, dev, dev,
> >+					      NULL);
> >+		if (!block_cb)
> >+			return -ENOMEM;
> >+
> >+		tcf_block_cb_list_add(block_cb, &f->cb_list);
> >+		return 0;
> > 	case TC_BLOCK_UNBIND:
> >-		tcf_block_cb_unregister(f->block, cb, dev);
> >+		block_cb = tcf_block_cb_lookup(f->block->index, cb, dev);
> >+		if (!block_cb)
> >+			return -ENOENT;
> >+
> >+		tcf_block_cb_list_move(block_cb, &f->cb_list);
> 
> What you are trying to achieve with list_move here, wrapped by
> tcf_block_cb_list_move() for some reason is a mystery for me.
> You are moving to &f->cb_list, but you are already there...
> 
> The whole work with lists here, including tcf_block_cb_list is very
> confusing for me.

The f->cb_list is a temporary list. The block_cb's are traveling back
to core through this list. Then, tcf_block_setup() takes these blocks
in this f->cb_list and place them in the right per-block and the
global block list.

So, instead of placing the block_cb's in the block list from the
driver, the driver just places block_cb in this temporary list, so the
core (either tc or netfilter) is responsible for attaching this to
either tcf_block or nft_chain.