|
Hi Hyunwoo,
I reviewed the change and the reasoning looks correct to me.
nfqnl_recv_verdict() dequeues the entry using find_dequeue_entry(), which
transfers ownership of the nf_queue_entry to this function. After that point
the function becomes responsible for either reinjecting or freeing the entry.
In the PF_BRIDGE path the code calls nfqa_parse_bridge() to parse the VLAN
attributes coming from userspace. If the attribute set is malformed (for
example NFQA_VLAN present but NFQA_VLAN_TCI missing), nfqa_parse_bridge()
returns an error. Before this patch, the function would return immediately in
that situation.
Because the entry had already been dequeued, returning directly means the
nf_queue_entry object and its associated sk_buff are never released. That also
leaves any held references such as net_device and struct net references alive.
If a userspace program repeatedly sends malformed verdict messages, this path
could leak queue entries and eventually exhaust kernel memory.
Your change fixes this by calling nfqnl_reinject(entry, NF_DROP) before
returning. This matches the error handling pattern used elsewhere in the file:
once the entry is owned by the verdict handler, it must be reinjected or
dropped so the resources are released correctly.
So the logic now becomes:
1. dequeue the entry
2. attempt bridge attribute parsing
3. if parsing fails, explicitly drop the packet via nfqnl_reinject()
4. return the error to the caller
That ensures the queue entry and skb are properly handled even in the malformed
attribute case.
The Fixes tag also makes sense since the leak path was introduced when bridge
verdict handling started using NFQA_VLAN/NFQA_L2HDR.
Overall the change is small, consistent with the existing reinjection model,
and addresses a clear ownership leak in the error path.
Reviewed by : David Dull
|
