Re: [RFC net v2 2/2] af_packet: retire TPACKET_V3 blocks without waiting for copiers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] af_packet: prb_retire_current_block() trusts user-mapped BLOCK_NUM_PKTS
- [Medium] af_packet: tpacket_rcv() bypasses ROOM_NONE drop when in_flight is non-zero
--

commit 9f9d78a929f32b0ce1f7614bea46eb96c69e1ab8
Author: jingxuanxie <jingxuanxie@xxxxxxxxxxxx>

af_packet: retire TPACKET_V3 blocks without waiting for copiers

This commit replaces the TPACKET_V3 ring-wide read/write lock with per-block
in-flight copy accounting in struct pgv so that full or expired blocks can
be sealed and advanced without waiting for active copiers to complete.
Sealed blocks are subsequently published in order under the receive queue
lock once their in_flight count reaches zero.

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7b3e1e27f7771..5bc486326be93 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
[ ... ]
> @@ -950,31 +949,24 @@ static void prb_retire_current_block(struct tpacket_kbdq_core *pkc,
>  		struct packet_sock *po, unsigned int status)
>  {
>  	struct tpacket_block_desc *pbd = GET_CURR_PBLOCK_DESC_FROM_CORE(pkc);
> +	struct pgv *block = &pkc->pkbdq[pkc->kactive_blk_num];
>  
> -	/* retire/close the current block */
> -	if (likely(TP_STATUS_KERNEL == BLOCK_STATUS(pbd))) {
> -		/*
> -		 * Plug the case where copy_bits() is in progress on
> -		 * cpu-0 and tpacket_rcv() got invoked on cpu-1, didn't
> -		 * have space to copy the pkt in the current block and
> -		 * called prb_retire_current_block()
> -		 *
> -		 * We don't need to worry about the TMO case because
> -		 * the timer-handler already handled this case.
> -		 */
> -		if (!(status & TP_STATUS_BLK_TMO)) {
> -			/* Waiting for skb_copy_bits to finish... */
> -			write_lock(&pkc->blk_fill_in_prog_lock);
> -			write_unlock(&pkc->blk_fill_in_prog_lock);
> -		}
> -		prb_close_block(pkc, pbd, po, status);
> +	if (!BLOCK_NUM_PKTS(pbd))
>  		return;
> -	}
> +	block->retire_status = status;
> +	WRITE_ONCE(block->last_pkt, (struct tpacket3_hdr *)pkc->prev);
> +	pkc->pending_blocks++;
> +	pkc->kactive_blk_num = GET_NEXT_PRB_BLK_NUM(pkc);
> +	prb_publish_blocks(pkc, po);
>  }

[Severity: High]
Can a user process with the RX ring mapped read-write corrupt the kernel's
block tracking state by writing 0 to pbd->hdr.bh1.num_pkts
(BLOCK_NUM_PKTS(pbd)) while packets are in flight on the current block?

Because struct tpacket_block_desc lives in the shared packet_mmap() buffer,
user space can zero BLOCK_NUM_PKTS(pbd) while tpacket_rcv() copiers are
active on the block (or between the check in
prb_retire_rx_blk_timer_expired() and the check in
prb_retire_current_block()). When that happens, prb_retire_current_block()
returns early without setting block->last_pkt, incrementing
pkc->pending_blocks, or advancing pkc->kactive_blk_num.

Both callers (__packet_lookup_frame_in_block() and
prb_retire_rx_blk_timer_expired()) immediately call
prb_dispatch_next_block(pkc, po) on the next line:

	prb_retire_current_block(pkc, po, 0);

	/* Now, try to dispatch the next block */
	curr = (char *)prb_dispatch_next_block(pkc, po);

Since pkc->kactive_blk_num was not advanced and block->last_pkt is still
NULL, prb_dispatch_next_block() sees prb_curr_blk_in_use(pkc) == 0 on the
same active block and calls prb_open_block(pkc, pbd), which resets
pkc->nxt_offset to the start of the block and runs:

	atomic_set(&block->in_flight, 0);

while the earlier tpacket_rcv() copiers are still writing into that block.
When those earlier copiers finish and call
atomic_dec_return_release(&copy_block->in_flight), block->in_flight can hit
0 prematurely while a newly dispatched copier is still running, or underflow
to -1 and permanently wedge prb_publish_blocks().

[ ... ]
> @@ -2306,8 +2295,10 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>  	if (!res)
>  		goto drop_n_restore;
>  
> -	/* If we are flooded, just give up */
> -	if (__packet_rcv_has_room(po, skb) == ROOM_NONE) {
> +	/* V3 allocation must get a chance to publish completed pending blocks. */
> +	if (__packet_rcv_has_room(po, skb) == ROOM_NONE &&
> +	    (po->tp_version != TPACKET_V3 ||
> +	     !READ_ONCE(po->rx_ring.prb_bdqc.pending_blocks))) {
>  		atomic_inc(&po->tp_drops);
>  		goto drop_n_restore;
>  	}

[Severity: Medium]
When the ring is full and a copier on the oldest pending block (at
publish_blk_num) is preempted or still in flight, does this check bypass the
lockless ROOM_NONE drop path on every arriving packet even though
prb_publish_blocks() cannot publish anything?

In prb_publish_blocks(), publication stops as soon as the block at
publish_blk_num has a non-zero in_flight count:

	while (pending--) {
		struct pgv *block = &pkc->pkbdq[pkc->publish_blk_num];
		struct tpacket_block_desc *pbd = (void *)block->buffer;

		/* Pairs with the release decrement after every completed copy. */
		if (atomic_read_acquire(&block->in_flight))
			break;

If a copier on publish_blk_num is preempted and subsequent traffic seals the
remaining blocks in the ring, __packet_rcv_has_room() returns ROOM_NONE
while pending_blocks > 0. Because tpacket_rcv() only checks
!READ_ONCE(po->rx_ring.prb_bdqc.pending_blocks) without checking whether the
head pending block's in_flight count has reached 0, every arriving packet
will take sk->sk_receive_queue.lock, call prb_publish_blocks() which
immediately breaks out of the loop, fail packet_current_rx_frame(), and jump
to drop_n_account where it triggers sk->sk_data_ready(sk) on every dropped
packet.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030081704.251652-1-jingxuanxie@xxxxxxxxxxxx?part=2