Re: [PATCH RFC net 0/3] netfilter: nfnetlink_queue: handle packet ID reuse and large gaps

[Florian Westphal <fw@strlen.de>]

Alireza Asgari via B4 Relay <devnull+alireza.asgari.net@kernel.org> wrote:
> This series addresses packet-ID reuse and batch-verdict ordering in
> NFQUEUE. I am posting it as an RFC to discuss the handling of ambiguous
> batch boundaries and the additional sequence state used to resolve them.

-v.  I see 0 bugs here.  Only lots of code churn.

> An old packet can remain queued while newer packets receive individual
> verdicts.

Not really, that would be a userspace bug.  Its not adviseable to do
out-of-order verdicts.  I know some people do that though :-/

> Queue length therefore does not bound the distance between
> pending packet IDs. After a complete 32-bit ID cycle, two pending packets
> can have the same ID, making verdict lookup ambiguous. Even before reuse,
> the signed 32-bit comparison in the batch handler can select the wrong
> FIFO prefix when IDs are separated by half an ID cycle or more.

WTF, whats the point of doing that?

> Patch 1 rejects an incoming duplicate ID before list insertion or userspace
> notification, preserving the existing pending packet. The incoming packet
> follows the existing enqueue-failure policy: drop by default, or accept
> with FAIL_OPEN.

OK, but really... who needs this?

> Patch 2 uses an exact queued boundary to select the FIFO prefix. For a
> boundary no longer queued, it uses a 64-bit queue sequence and a per-entry
> 32-bit epoch to recover issuance order. Ambiguous missing boundaries
> return -ERANGE without changing the queue. The wire format is unchanged,
> but the rejection behavior is userspace-visible. This does not distinguish
> stale verdicts after an ID has been released and reused.

Insane... tons of code added for what?  Batch verdicts are incompatible
with out-of-order verdicts.  I fail to understand why the kernel needs
to become more complex and more complex and more complex.

LLMs make it oh-so-easy but whats left is even more unmaintanable than
the current state.

> I would appreciate feedback on:
> 
>   - Whether -ERANGE with the queue unchanged is the appropriate response
>     for an ambiguous missing batch boundary.
>   - Whether the per-entry epoch and widened queue counter are appropriate
>     for handling boundaries that have already been removed.

Sorry, I have no idea what these patches should accomplish.

>   - Whether the existing drop/FAIL_OPEN policy is the preferred way to
>     handle an incoming packet whose ID is still pending.

That one is fine, even though I would really like to understand what use
case requires holding on to a packet for 2**32 (!) Ids to pass.