Hi Florian, I have iterated several times over this batch, but I came up with one late question today, see below. On Thu, Apr 25, 2024 at 02:06:46PM +0200, Florian Westphal wrote: > This set type keeps two copies of the sets' content, > priv->match (live version, used to match from packet path) > priv->clone (work-in-progress version of the 'future' priv->match). > > All additions and removals are done on priv->clone. When transaction > completes, priv->clone becomes priv->match and a new clone is allocated > for use by next transaction. > > Problem is that the cloning requires GFP_KERNEL allocations but we > cannot fail at either commit or abort time. > > This patch defers the clone until we get an insertion or removal > request. This allows us to handle OOM situations correctly. > > This also allows to remove ->dirty in a followup change: > > If ->clone exists, ->dirty is always true > If ->clone is NULL, ->dirty is always false, no elements were added > or removed (except catchall elements which are external to the specific > set backend). > > Signed-off-by: Florian Westphal <fw@xxxxxxxxx> > Reviewed-by: Stefano Brivio <sbrivio@xxxxxxxxxx> > --- > net/netfilter/nft_set_pipapo.c | 73 ++++++++++++++++++++++++---------- > 1 file changed, 52 insertions(+), 21 deletions(-) > > diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c > index 9c8da9a0861d..2b1c91e56ca1 100644 > --- a/net/netfilter/nft_set_pipapo.c > +++ b/net/netfilter/nft_set_pipapo.c > @@ -615,6 +615,9 @@ nft_pipapo_get(const struct net *net, const struct nft_set *set, > struct nft_pipapo_match *m = priv->clone; > struct nft_pipapo_elem *e; > > + if (!m) > + m = rcu_dereference(priv->match); nft_pipapo_get() is called from rcu path via _GET netlink command. Is it safe to walk over priv->clone? Userspace could be updating (with mutex held) while a request to get an element can be done. That makes me think nft_pipapo_get() should always use priv->match? Thanks, and apologies for the late review.
