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

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

[Ren Wei]

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.

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

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

[Willem de Bruijn]

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