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

zihan xi wrote:

On Tue, Jul 28, 2026 at 9:31 PM Willem de Bruijn
[off-list ref] wrote:

quoted

zihan xi wrote:

quoted

On Tue, Jul 28, 2026 at 7:44 PM Willem de Bruijn
[off-list ref] wrote:

quoted

Ren Wei wrote:

quoted

From: Zihan Xi <redacted>

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.

quoted

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.

quoted

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 <redacted>
Assisted-by: Codex:gpt-5.4
Signed-off-by: Zihan Xi <redacted>
Signed-off-by: Ren Wei <redacted>
---
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/ (local)

 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.

quoted

              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

Hi Willem,

Thanks. Yes, that is the intended reasoning for the packet_recvmsg()
fast path.

If PACKET_SOCK_PRESSURE is already clear when packet_recvmsg() reaches
the pressure-clearing path, then after the RX hook switches away from
tpacket_rcv the socket has already been detached and synchronize_net()
has completed, so no new packet input can arrive and set the flag
again. In that case skipping the extra lock acquisition is sufficient.

Thanks. Please add this context to the commit message. It's not
entirely obvious. Will be helpful next time someone does a git blame.

quoted

For the !tx_ring condition: po->prot_hook.func is RX-side state, and
this fix is meant to publish it under the same sk_receive_queue.lock
domain that protects RX ring reconfiguration.

A TX ring reconfiguration does not change po->rx_ring.pg_vec, so it
does not need to rewrite po->prot_hook.func. Keeping the assignment
under !tx_ring avoids an unnecessary write on a TX-only path and keeps
the moved publication strictly tied to the RX ring transition that
actually needs serialization.

Still, this is a change that is not strictly required for the fix. It
may be relatively easy to reason that it has no unintentional side
effects. But it is even easier to do so by avoiding such unnecessary
changes altogether. It would not be the first NOOP that proves to not
be entirely NOOP. I suggest moving the original code as is, instead.

Thanks, agreed.

I'll make this change in v3: keep the receive hook assignment otherwise
unchanged and only move the existing assignment into the
sk_receive_queue.lock section. That avoids the extra !tx_ring behavior
change while still keeping the hook publication serialized with the ring
state update.

I'll also add the packet_recvmsg() fast-path reasoning to the v3 commit
message.

Great, thanks.