5.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Pablo Neira Ayuso pablo@netfilter.org
[ Upstream commit 7ffc7481153bbabf3332c6a19b289730c7e1edf5 ]
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 Signed-off-by: Sasha Levin sashal@kernel.org --- net/netfilter/nft_set_hash.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c index ec03d4c77b3df..200f837372adf 100644 --- a/net/netfilter/nft_set_hash.c +++ b/net/netfilter/nft_set_hash.c @@ -27,10 +27,12 @@ extern unsigned int nf_tables_net_id; struct nft_rhash { struct rhashtable ht; struct delayed_work gc_work; + u32 wq_gc_seq; };
struct nft_rhash_elem { struct rhash_head node; + u32 wq_gc_seq; struct nft_set_ext ext; };
@@ -325,6 +327,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);
@@ -342,6 +348,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;
@@ -362,6 +376,8 @@ static void nft_rhash_gc(struct work_struct *work) 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); }