Commit 7845914f authored by Florian Westphal's avatar Florian Westphal

netfilter: nf_tables: don't fail inserts if duplicate has expired

nftables selftests fail:
run-tests.sh testcases/sets/0044interval_overlap_0
Expected: 0-2 . 0-3, got:
W: [FAILED]     ./testcases/sets/0044interval_overlap_0: got 1

Insertion must ignore duplicate but expired entries.

Moreover, there is a strange asymmetry in nft_pipapo_activate:

It refetches the current element, whereas the other ->activate callbacks
(bitmap, hash, rhash, rbtree) use elem->priv.
Same for .remove: other set implementations take elem->priv,
nft_pipapo_remove fetches elem->priv, then does a relookup,
remove this.

I suspect this was the reason for the change that prompted the
removal of the expired check in pipapo_get() in the first place,
but skipping exired elements there makes no sense to me, this helper
is used for normal get requests, insertions (duplicate check)
and deactivate callback.

In first two cases expired elements must be skipped.

For ->deactivate(), this gets called for DELSETELEM, so it
seems to me that expired elements should be skipped as well, i.e.
delete request should fail with -ENOENT error.

Fixes: 24138933 ("netfilter: nf_tables: don't skip expired elements during walk")
Signed-off-by: default avatarFlorian Westphal <fw@strlen.de>
parent 90e5b346
...@@ -566,6 +566,8 @@ static struct nft_pipapo_elem *pipapo_get(const struct net *net, ...@@ -566,6 +566,8 @@ static struct nft_pipapo_elem *pipapo_get(const struct net *net,
goto out; goto out;
if (last) { if (last) {
if (nft_set_elem_expired(&f->mt[b].e->ext))
goto next_match;
if ((genmask && if ((genmask &&
!nft_set_elem_active(&f->mt[b].e->ext, genmask))) !nft_set_elem_active(&f->mt[b].e->ext, genmask)))
goto next_match; goto next_match;
...@@ -600,17 +602,8 @@ static struct nft_pipapo_elem *pipapo_get(const struct net *net, ...@@ -600,17 +602,8 @@ static struct nft_pipapo_elem *pipapo_get(const struct net *net,
static void *nft_pipapo_get(const struct net *net, const struct nft_set *set, static void *nft_pipapo_get(const struct net *net, const struct nft_set *set,
const struct nft_set_elem *elem, unsigned int flags) const struct nft_set_elem *elem, unsigned int flags)
{ {
struct nft_pipapo_elem *ret; return pipapo_get(net, set, (const u8 *)elem->key.val.data,
ret = pipapo_get(net, set, (const u8 *)elem->key.val.data,
nft_genmask_cur(net)); nft_genmask_cur(net));
if (IS_ERR(ret))
return ret;
if (nft_set_elem_expired(&ret->ext))
return ERR_PTR(-ENOENT);
return ret;
} }
/** /**
...@@ -1743,11 +1736,7 @@ static void nft_pipapo_activate(const struct net *net, ...@@ -1743,11 +1736,7 @@ static void nft_pipapo_activate(const struct net *net,
const struct nft_set *set, const struct nft_set *set,
const struct nft_set_elem *elem) const struct nft_set_elem *elem)
{ {
struct nft_pipapo_elem *e; struct nft_pipapo_elem *e = elem->priv;
e = pipapo_get(net, set, (const u8 *)elem->key.val.data, 0);
if (IS_ERR(e))
return;
nft_set_elem_change_active(net, set, &e->ext); nft_set_elem_change_active(net, set, &e->ext);
} }
...@@ -1961,10 +1950,6 @@ static void nft_pipapo_remove(const struct net *net, const struct nft_set *set, ...@@ -1961,10 +1950,6 @@ static void nft_pipapo_remove(const struct net *net, const struct nft_set *set,
data = (const u8 *)nft_set_ext_key(&e->ext); data = (const u8 *)nft_set_ext_key(&e->ext);
e = pipapo_get(net, set, data, 0);
if (IS_ERR(e))
return;
while ((rules_f0 = pipapo_rules_same_key(m->f, first_rule))) { while ((rules_f0 = pipapo_rules_same_key(m->f, first_rule))) {
union nft_pipapo_map_bucket rulemap[NFT_PIPAPO_MAX_FIELDS]; union nft_pipapo_map_bucket rulemap[NFT_PIPAPO_MAX_FIELDS];
const u8 *match_start, *match_end; const u8 *match_start, *match_end;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment