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

Reply via email to