Re: [PATCH nf] netfilter: ipset: fix double free on kernel-side delete during resize

[Xue Boyang <fuchen.dust@gmail.com>]

Hi Jozsef,

thanks for looking at the patch!

> Thank you for your patch! Could you address the issues sashiko raised at
> https://sashiko.dev/#/patchset/20261012163353.4093-1-fuchen.dust%40gmail.com
> about it?

Sure - v2 follows right after this mail, quoting the findings and what I
did about each of them:

> Can a memory allocation failure bypass the backlog cleanup block and leak
> the deferred operations? [Medium]

The exact path claimed cannot: every early exit is taken before the table
is marked resizing, so the backlog is still empty there. The leak is real
one retry later though: on -EAGAIN the cleanup path jumps to the retry
before draining the backlog, and an early failure of the next attempt
(its own allocation failures are again before the mark) strands it - and
with the deferred delete being the only record of the delete, the
elements would stay in the set while the packet path believes them
deleted. v2 drains the backlog on all failure exits, replaying it
against the original table.

> Does the backlog replay ordering during resize incorrectly resurrect
> deleted elements? [High]

Yes, it does - the list_add() prepend of deferred deletes predates this
patch. Since the fix now relies on the backlog order, v2 queues the
deferred deletes with list_add_tail() as well, so the whole backlog
replays in arrival order, on the failure paths too.

> Does this skip clear_bit(i, n->used) and leave the element fully active
> in the original table? If a subsequent ADD for the same element occurs
> before the resize completes, will mtype_add() incorrectly see the
> element as still existing, return -IPSET_ERR_EXIST, and fail to queue
> its own operation to the backlog? [High]

Exactly, and fixed the other way around: clearing the used bit is not an
option, the extension of an element the resize copy loop has not reached
yet would never be freed (the old table is destroyed with
ext_destroy=false to protect the extensions shared with the copied
elements). v2 defers the kernel-side add of an element still present in
the current table instead, so the replay applies the del and the add in
arrival order and the add is no longer lost.

The remaining Medium concern (leak of the extensions of elements which
expired during the resize) is preexisting and not touched by v2 - I can
look at it separately. The five dismissed concerns look like false
positives to me, e.g. the lockless orig->ad walk after the table swap is
serialized by the synchronize_rcu().

Also included in v2: the deferred delete now reports -ENOMEM when its
backlog entry cannot be allocated, instead of returning success.

v2 was tested with the same reproducer as v1 (no double free in repeated
runs) plus a new ordering regression test: packet-path del-then-add
pairs race continuous resizes and the set membership is checked right
after a resize completes, with the traffic source frozen so nothing can
paper over the outcome. On v1 the element deleted and re-added during
the window comes out missing (the lost add); an instrumented v1 kernel
confirmed a single ~80ms resize window deferring 253 packet-path
deletes and dropping the corresponding adds with -EEXIST, replaying the
deletes in LIFO order. On v2 the element is present in every sampled
resize across repeated runs.

Best regards,
Xue Boyang