Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert
@ 2020-06-14 21:42 Stefano Brivio
  2020-06-17 20:00 ` Pablo Neira Ayuso
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Stefano Brivio @ 2020-06-14 21:42 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal

In nft_pipapo_insert(), we need to reallocate scratch maps that will
be used for matching by lookup functions, if they have never been
allocated or if the bucket size changes as a result of the insertion.

As pipapo_realloc_scratch() provides a pair of fresh, zeroed out
maps, there's no need to select a particular one after reallocation.

Other than being useless, the existing assignment was also troubled
by the fact that the index was set only on the CPU performing the
actual insertion, as spotted by Florian.

Simply drop the assignment.

Reported-by: Florian Westphal <fw@strlen.de>
Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
This might cause a conflict on merge from nf.git with commit
c3829285b2e6 ("netfilter: nft_set_pipapo: Disable preemption before
getting per-CPU pointer") -- the resolution is to just drop the call
to this_cpu_write() like this patch does. I can send an explicit
conflict resolution patch if needed.

 net/netfilter/nft_set_pipapo.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
index 8b5acc6910fd..9aa2bb3982e8 100644
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -1247,8 +1247,6 @@ static int nft_pipapo_insert(const struct net *net, const struct nft_set *set,
 		if (err)
 			return err;
 
-		this_cpu_write(nft_pipapo_scratch_index, false);
-
 		m->bsize_max = bsize_max;
 	}
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert
  2020-06-14 21:42 [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert Stefano Brivio
@ 2020-06-17 20:00 ` Pablo Neira Ayuso
  2020-06-17 20:07 ` Pablo Neira Ayuso
  2020-06-30 16:44 ` Pablo Neira Ayuso
  2 siblings, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2020-06-17 20:00 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: netfilter-devel, Florian Westphal

On Sun, Jun 14, 2020 at 11:42:07PM +0200, Stefano Brivio wrote:
> In nft_pipapo_insert(), we need to reallocate scratch maps that will
> be used for matching by lookup functions, if they have never been
> allocated or if the bucket size changes as a result of the insertion.
> 
> As pipapo_realloc_scratch() provides a pair of fresh, zeroed out
> maps, there's no need to select a particular one after reallocation.
> 
> Other than being useless, the existing assignment was also troubled
> by the fact that the index was set only on the CPU performing the
> actual insertion, as spotted by Florian.
> 
> Simply drop the assignment.

I have just refreshed nf-next to pull in your fix dependency.

Please, rebase and resubmit this patch on top of nf-next.

Thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert
  2020-06-14 21:42 [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert Stefano Brivio
  2020-06-17 20:00 ` Pablo Neira Ayuso
@ 2020-06-17 20:07 ` Pablo Neira Ayuso
  2020-06-17 20:17   ` Stefano Brivio
  2020-06-30 16:44 ` Pablo Neira Ayuso
  2 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2020-06-17 20:07 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: netfilter-devel, Florian Westphal

On Sun, Jun 14, 2020 at 11:42:07PM +0200, Stefano Brivio wrote:
> In nft_pipapo_insert(), we need to reallocate scratch maps that will
> be used for matching by lookup functions, if they have never been
> allocated or if the bucket size changes as a result of the insertion.
> 
> As pipapo_realloc_scratch() provides a pair of fresh, zeroed out
> maps, there's no need to select a particular one after reallocation.
> 
> Other than being useless, the existing assignment was also troubled
> by the fact that the index was set only on the CPU performing the
> actual insertion, as spotted by Florian.
> 
> Simply drop the assignment.
> 
> Reported-by: Florian Westphal <fw@strlen.de>
> Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")

Hm.

It has a Fixes: tag.

Probably route this through nf.git instead?

Thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert
  2020-06-17 20:07 ` Pablo Neira Ayuso
@ 2020-06-17 20:17   ` Stefano Brivio
  0 siblings, 0 replies; 5+ messages in thread
From: Stefano Brivio @ 2020-06-17 20:17 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Florian Westphal

On Wed, 17 Jun 2020 22:07:05 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Sun, Jun 14, 2020 at 11:42:07PM +0200, Stefano Brivio wrote:
> > In nft_pipapo_insert(), we need to reallocate scratch maps that will
> > be used for matching by lookup functions, if they have never been
> > allocated or if the bucket size changes as a result of the insertion.
> > 
> > As pipapo_realloc_scratch() provides a pair of fresh, zeroed out
> > maps, there's no need to select a particular one after reallocation.
> > 
> > Other than being useless, the existing assignment was also troubled
> > by the fact that the index was set only on the CPU performing the
> > actual insertion, as spotted by Florian.
> > 
> > Simply drop the assignment.
> > 
> > Reported-by: Florian Westphal <fw@strlen.de>
> > Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")  
> 
> Hm.
> 
> It has a Fixes: tag.
> 
> Probably route this through nf.git instead?

I wouldn't, because it just removes a redundant assignment (so I
consider it a fix) but it doesn't fix any functional issue. Is there a
specific reason why I should?

-- 
Stefano


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert
  2020-06-14 21:42 [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert Stefano Brivio
  2020-06-17 20:00 ` Pablo Neira Ayuso
  2020-06-17 20:07 ` Pablo Neira Ayuso
@ 2020-06-30 16:44 ` Pablo Neira Ayuso
  2 siblings, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2020-06-30 16:44 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: netfilter-devel, Florian Westphal

On Sun, Jun 14, 2020 at 11:42:07PM +0200, Stefano Brivio wrote:
> In nft_pipapo_insert(), we need to reallocate scratch maps that will
> be used for matching by lookup functions, if they have never been
> allocated or if the bucket size changes as a result of the insertion.
> 
> As pipapo_realloc_scratch() provides a pair of fresh, zeroed out
> maps, there's no need to select a particular one after reallocation.
> 
> Other than being useless, the existing assignment was also troubled
> by the fact that the index was set only on the CPU performing the
> actual insertion, as spotted by Florian.
> 
> Simply drop the assignment.

Applied to nf-next, thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-14 21:42 [PATCH nf-next] nft_set_pipapo: Drop useless assignment of scratch map index on insert Stefano Brivio
2020-06-17 20:00 ` Pablo Neira Ayuso
2020-06-17 20:07 ` Pablo Neira Ayuso
2020-06-17 20:17   ` Stefano Brivio
2020-06-30 16:44 ` Pablo Neira Ayuso

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git