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: <20190426142715.GB2249@nanopsycho.orion>
Date:   Fri, 26 Apr 2019 16:27:15 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Pablo Neira Ayuso <pablo@...filter.org>
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

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.


> 		return 0;
> 	default:
> 		return -EOPNOTSUPP;
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index a00463c8cfa9..f7f6f42d58d1 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -714,6 +714,7 @@ struct tcf_block_cb {
> 	void (*release)(void *cb_priv);
> 	void *cb_ident;
> 	void *cb_priv;
>+	u32 block_index;
> 	unsigned int refcnt;
> };
> 
>@@ -730,12 +731,14 @@ EXPORT_SYMBOL(tcf_block_cb_priv);
> 
> static LIST_HEAD(tcf_block_cb_list);
> 
>-struct tcf_block_cb *tcf_block_cb_lookup(struct tcf_block *block,
>-					 tc_setup_cb_t *cb, void *cb_ident)
>+struct tcf_block_cb *tcf_block_cb_lookup(u32 block_index, tc_setup_cb_t *cb,
>+					 void *cb_ident)
> {	struct tcf_block_cb *block_cb;
> 
>-	list_for_each_entry(block_cb, &block->cb_list, list)
>-		if (block_cb->cb == cb && block_cb->cb_ident == cb_ident)
>+	list_for_each_entry(block_cb, &tcf_block_cb_list, list)
>+		if (block_cb->block_index == block_index &&
>+		    block_cb->cb == cb &&
>+		    block_cb->cb_ident == cb_ident)
> 			return block_cb;
> 	return NULL;
> }

[...]