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

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

Ren Wei wrote:
> From: Zihan Xi <xizh2024@lzu.edu.cn>
> 
> 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.
> This keeps the receive hook decision consistent with the ring state used
> by the tpacket room checks.

Thanks for the report and proposed fix.

Ideally we can avoid taking sk_receive_queue.lock an extra time in
packet_recvmsg.

packet_rcv_try_clear_pressure only accesses the ring if flag
PACKET_SOCK_PRESSURE is set. One option may be to clear that in
packet_set_ring, after detaching the socket (and thus after any input
could set it again) and before swapping prot_hook.func (with a barrier
to guarantee that). E.g.,:

@@ -4528,6 +4528,7 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
        WRITE_ONCE(po->num, 0);
        if (was_running)
                __unregister_prot_hook(sk, false);
+       packet_sock_flag_set(po, PACKET_SOCK_PRESSURE, false);
 
        spin_unlock(&po->bind_lock);


> 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 <xizh2024@lzu.edu.cn>
> Reviewed-by: Ren Wei <enjou1224z@gmail.com>
> ---
>  net/packet/af_packet.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 8e6f3a734ba0..b369f44b4065 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1315,13 +1315,22 @@ 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;
> +
> +	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 +4313,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 +4553,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;
>  		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