Re: [RFC PATCH net v3 1/1] af_packet: avoid waiting for TPACKET_V3 copies at retirement

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] af_packet: premature packet drops in prb_retire_current_block() on SMP
--

commit ed9927a1021a4ff736d3e13140014c9d82d1ba49
Author: jingxuanxie <jingxuanxie@xxxxxxxxxxxx>

af_packet: avoid waiting for TPACKET_V3 copies at retirement

This commit replaces blocking write_lock() calls on blk_fill_in_prog_lock
with write_trylock() in the TPACKET_V3 block retirement paths so that
callers holding sk_receive_queue.lock do not block waiting for in-progress
packet copies. When write_trylock() fails during frame lookup,
prb_retire_current_block() returns false and the packet capture record is
dropped.

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index cf6429f094772..8fd6372ab59bd 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
[ ... ]
> @@ -946,8 +948,8 @@ static void *prb_dispatch_next_block(struct tpacket_kbdq_core *pkc,
>  	return (void *)pkc->nxt_offset;
>  }
>  
> -static void prb_retire_current_block(struct tpacket_kbdq_core *pkc,
> -		struct packet_sock *po, unsigned int status)
> +static bool 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);
>  
> @@ -963,13 +965,14 @@ static void prb_retire_current_block(struct tpacket_kbdq_core *pkc,
>  		 * 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);
> +			/* Do not wait for copies while holding the queue lock. */
> +			if (!write_trylock(&pkc->blk_fill_in_prog_lock))
> +				return false;
>  			write_unlock(&pkc->blk_fill_in_prog_lock);
>  		}
>  		prb_close_block(pkc, pbd, po, status);
> -		return;
>  	}
> +	return true;
>  }
>  
>  static int prb_curr_blk_in_use(struct tpacket_block_desc *pbd)
> @@ -1097,7 +1100,8 @@ static void *__packet_lookup_frame_in_block(struct packet_sock *po,
>  	}
>  
>  	/* Ok, close the current block */
> -	prb_retire_current_block(pkc, po, 0);
> +	if (!prb_retire_current_block(pkc, po, 0))
> +		return NULL;

[Severity: Medium]
Does using write_trylock() here cause premature packet drops and delayed
block retirement during normal multi-core traffic, even on non-PREEMPT_RT
kernels and when free blocks are available in the ring?

In tpacket_rcv(), a CPU reserves a slot in the current block via:

  tpacket_rcv()
    packet_current_rx_frame()
      __packet_lookup_frame_in_block()
        prb_fill_curr_block()
          read_lock(&pkc->blk_fill_in_prog_lock);

and then drops sk->sk_receive_queue.lock before running skb_copy_bits(),
tpacket_get_timestamp(), and dev_parse_header() concurrently with other
CPUs.

While CPU 0 is mid-copy on the last packet of a block, CPU 1 can arrive in
tpacket_rcv() with a packet that does not fit in the current block and call
__packet_lookup_frame_in_block() -> prb_retire_current_block(). Even though
CPU 0 has not been preempted (and on non-PREEMPT_RT kernels read_lock()
disables preemption), write_trylock() on CPU 1 will fail immediately,
causing tpacket_rcv() to drop the packet, increment tp_drops (setting
TP_STATUS_LOSING), and skip dispatching the next free block.

Furthermore, when CPU 0 finishes its copy and calls
prb_clear_blk_fill_status(), it only releases the read lock without closing
the block. If the dropped packet on CPU 1 was the end of a burst, won't the
full block remain unretired in TP_STATUS_KERNEL until
prb_retire_rx_blk_timer_expired() fires (and could be delayed by another
full interval_ktime if write_trylock() in the timer handler also coincides
with an active copy)?

Finally, when __packet_lookup_frame_in_block() returns NULL here,
tpacket_rcv() jumps to drop_n_account:

  drop_n_account:
        spin_unlock(&sk->sk_receive_queue.lock);
        atomic_inc(&po->tp_drops);
        drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR;

        sk->sk_data_ready(sk);

which wakes user-space waiters even though no block was retired to
TP_STATUS_USER. On PREEMPT_RT, could waking a higher-priority user-space
reader thread on every dropped packet preempt the copier thread holding
blk_fill_in_prog_lock and prolong the state where write_trylock() keeps
failing?

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