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

[Yiqi Sun]

Hi,

I discovered the same double-free during ipset hash resize
and confirmed its exploitability by successfully exploiting it
in my test environment.

While reviewing this fix, I noticed two remaining issues with the
deferred operations in net/netfilter/ipset/ip_set_hash_gen.h:

1. Failure-path replay can resurrect a deleted element.

In mtype_resize(), cleanup clears orig->resizing before reaching the new
drain path. Packet-path operations can then modify the original table
directly, while earlier operations are still waiting in the backlog.

For example, starting with A absent:

  - During resize, a packet-path ADD inserts A into the original table
    and also queues an ADD in orig->ad.
  - A bucket allocation in the resize copy loop fails. cleanup clears
    orig->resizing and waits for an RCU grace period.
  - Before drain replays the backlog, a later packet-path DEL removes A
    from the original table and returns success without queuing a DEL.
  - drain replays the earlier ADD, inserting A again.

The final state therefore contradicts the later successful DEL. Before
this change, the failure path discarded the queued ADD, so this sequence
left A absent.

The set->lock around list_splice_init() protects the list, but does not
serialize replay with packet-path updates under the region locks. The
RCU grace period does not prevent new updates either. Could failure-path
replay preserve ordering with those updates as well as within the backlog?

2. The new deferred-ADD branch can return success without queuing the ADD.

In mtype_add(), the new branch for an existing element sets ret = 0 and
jumps to resize. If allocation of struct mtype_resize_ad fails there,
the existing "Don't bother" path goes to out with ret still zero.

For example, starting with A present:

  - During resize, a packet-path DEL queues a deletion of A, leaving A
    in the original table.
  - A subsequent packet-path ADD of A, without IPSET_FLAG_EXIST, takes
    the new branch.
  - Backlog allocation fails, but the ADD returns success.
  - Replaying the queued DEL leaves A absent because no ADD was queued.

This branch relies entirely on the backlog to preserve the ADD after
the pending DEL. Allocation failure should at least return -ENOMEM for
this deferred-only case, consistent with the deferred-DEL path.

Validation of the two review findings above used a userspace harness
containing the extracted add/delete functions and failure-path replay
code, with lock/RCU stubs. Both sequences produced the incorrect final
state with v3; the corresponding unpatched baseline cases did not.
The baseline was mainline commit 05441d697f7f ("Merge tag 'cocci-7.4-rc1'
of git://git.kernel.org/pub/scm/linux/kernel/git/jlawall/linux"). These
are deterministic control-flow checks, not reproductions on a running
kernel.

This review and the harness were prepared with AI assistance.

Thanks.