netfilter: nft_set_hash: skip duplicated elements pending gc run

rhashtable does not provide stable walk, duplicated elements are
possible in case of resizing. I considered that checking for errors when
calling rhashtable_walk_next() was sufficient to detect the resizing.
However, rhashtable_walk_next() returns -EAGAIN only at the end of the
iteration, which is too late, because a gc work containing duplicated
elements could have been already scheduled for removal to the worker.

Add a u32 gc worker sequence number per set, bump it on every workqueue
run. Annotate gc worker sequence number on the expired element. Use it
to skip those already seen in this gc workqueue run.

Note that this new field is never reset in case gc transaction fails, so
next gc worker run on the expired element overrides it. Wraparound of gc
worker sequence number should not be an issue with stale gc worker
sequence number in the element, that would just postpone the element
removal in one gc run.

Note that it is not possible to use flags to annotate that element is
pending gc run to detect duplicates, given that gc transaction can be
invalidated in case of update from the control plane, therefore, not
allowing to clear such flag.

On x86_64, pahole reports no changes in the size of nft_rhash_elem.

Fixes: f6c383b8c31a ("netfilter: nf_tables: adapt set backend to use GC transaction API")
Reported-by: Laurent Fasnacht <laurent.fasnacht@proton.ch>
Tested-by: Laurent Fasnacht <laurent.fasnacht@proton.ch>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
index 65bd291318f2ac4a813feaadbf06e09bbca044e3..8bfac4185ac797fa69a0e5c92e690d92db4c097a 100644 (file)
--- a/net/netfilter/nft_set_hash.c
+++ b/net/netfilter/nft_set_hash.c
@@ -24,11 +24,13 @@
 struct nft_rhash {
        struct rhashtable               ht;
        struct delayed_work             gc_work;
+       u32                             wq_gc_seq;
 };
 
 struct nft_rhash_elem {
        struct nft_elem_priv            priv;
        struct rhash_head               node;
+       u32                             wq_gc_seq;
        struct nft_set_ext              ext;
 };
 
@@ -338,6 +340,10 @@ static void nft_rhash_gc(struct work_struct *work)
        if (!gc)
                goto done;
 
+       /* Elements never collected use a zero gc worker sequence number. */
+       if (unlikely(++priv->wq_gc_seq == 0))
+               priv->wq_gc_seq++;
+
        rhashtable_walk_enter(&priv->ht, &hti);
        rhashtable_walk_start(&hti);
 
@@ -355,6 +361,14 @@ static void nft_rhash_gc(struct work_struct *work)
                        goto try_later;
                }
 
+               /* rhashtable walk is unstable, already seen in this gc run?
+                * Then, skip this element. In case of (unlikely) sequence
+                * wraparound and stale element wq_gc_seq, next gc run will
+                * just find this expired element.
+                */
+               if (he->wq_gc_seq == priv->wq_gc_seq)
+                       continue;
+
                if (nft_set_elem_is_dead(&he->ext))
                        goto dead_elem;
 
@@ -371,6 +385,8 @@ dead_elem:
                if (!gc)
                        goto try_later;
 
+               /* annotate gc sequence for this attempt. */
+               he->wq_gc_seq = priv->wq_gc_seq;
                nft_trans_gc_elem_add(gc, he);
        }