Re: [PATCH net v2 1/1] packet: synchronize pressure clearing with ring reconfiguration

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Ren Wei wrote:
> From: Zihan Xi <zihanx@nebusec.ai>
> 
> packet_set_ring() updates the RX ring state under sk_receive_queue.lock,
> but publishes the tpacket receive mode through po->prot_hook.func after
> releasing that lock. packet_poll() and packet_recvmsg() can therefore run
> the pressure clearing path after the ring has been cleared while still
> seeing tpacket_rcv, causing __packet_rcv_has_room() to dereference stale
> or NULL ring storage.
> 
> Serialize pressure clearing with RX ring reconfiguration and update the
> receive hook while holding the same queue lock when changing the RX ring.
> Keep packet_poll() on the unlocked helper, 

Because it already holds the lock.

> and let packet_recvmsg()
> take the queue lock only when PACKET_SOCK_PRESSURE is already set 

which is sufficient, because if the socket moves from tpacket_rcv to
packet_rcv, the socket is detached and a synchronize_net has passed,
so no new packets could arrive to set PACKET_SOCK_PRESSURE if it was
unset.

> so the
> fix avoids an unconditional extra lock acquisition on the normal recv
> path.
> 
> Fixes: 2ccdbaa6d55b ("packet: rollover lock contention avoidance")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Assisted-by: Codex:gpt-5.4
> Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
> Signed-off-by: Ren Wei <enjou1224z@gmail.com>
> ---
> changes in v2:
>   - only take sk_receive_queue.lock in packet_recvmsg() when
>     PACKET_SOCK_PRESSURE is already set
>   - keep packet_poll() on the unlocked helper under its existing queue lock
>   - v1 Link: https://lore.kernel.org/all/24f7311aed0c9ff06b8ea982647b82bf543ec369.1784454542.git.xizh2024@lzu.edu.cn/
> 
>  net/packet/af_packet.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 8e6f3a734ba0..0107e55de6ff 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1315,13 +1315,25 @@ static int packet_rcv_has_room(struct packet_sock *po, struct sk_buff *skb)
>  	return ret;
>  }
>  
> -static void packet_rcv_try_clear_pressure(struct packet_sock *po)
> +static void __packet_rcv_try_clear_pressure(struct packet_sock *po)
>  {
>  	if (packet_sock_flag(po, PACKET_SOCK_PRESSURE) &&
>  	    __packet_rcv_has_room(po, NULL) == ROOM_NORMAL)
>  		packet_sock_flag_set(po, PACKET_SOCK_PRESSURE, false);
>  }
>  
> +static void packet_rcv_try_clear_pressure(struct packet_sock *po)
> +{
> +	struct sock *sk = &po->sk;
> +
> +	if (!packet_sock_flag(po, PACKET_SOCK_PRESSURE))
> +		return;
> +
> +	spin_lock_bh(&sk->sk_receive_queue.lock);
> +	__packet_rcv_try_clear_pressure(po);
> +	spin_unlock_bh(&sk->sk_receive_queue.lock);
> +}
> +
>  static void packet_sock_destruct(struct sock *sk)
>  {
>  	skb_queue_purge(&sk->sk_error_queue);
> @@ -4304,7 +4316,7 @@ static __poll_t packet_poll(struct file *file, struct socket *sock,
>  			TP_STATUS_KERNEL))
>  			mask |= EPOLLIN | EPOLLRDNORM;
>  	}
> -	packet_rcv_try_clear_pressure(po);
> +	__packet_rcv_try_clear_pressure(po);
>  	spin_unlock_bh(&sk->sk_receive_queue.lock);
>  	spin_lock_bh(&sk->sk_write_queue.lock);
>  	if (po->tx_ring.pg_vec) {
> @@ -4544,14 +4556,15 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
>  		rb->frame_max = (req->tp_frame_nr - 1);
>  		rb->head = 0;
>  		rb->frame_size = req->tp_frame_size;
> +		if (!tx_ring)
> +			po->prot_hook.func = po->rx_ring.pg_vec ?
> +						tpacket_rcv : packet_rcv;

Same question: why this new condition on !tx_ring here, that is
missing below. It looks benign to me, but especially for fixes only
make necessary changes.

>  		spin_unlock_bh(&rb_queue->lock);
>  
>  		swap(rb->pg_vec_order, order);
>  		swap(rb->pg_vec_len, req->tp_block_nr);
>  
>  		rb->pg_vec_pages = req->tp_block_size/PAGE_SIZE;
> -		po->prot_hook.func = (po->rx_ring.pg_vec) ?
> -						tpacket_rcv : packet_rcv;
>  		skb_queue_purge(rb_queue);
>  		if (atomic_long_read(&po->mapped))
>  			pr_err("packet_mmap: vma is busy: %ld\n",
> -- 
> 2.43.0