* Re: [PATCH v2] RDMA/rxe: Hold netdev reference for transmit skbs
2026-07-14 1:55 ` [PATCH v2] " weimin xiong
@ 2026-07-15 2:47 ` Zhu Yanjun
2026-07-15 6:41 ` [PATCH v3] " weimin xiong
1 sibling, 0 replies; 5+ messages in thread
From: Zhu Yanjun @ 2026-07-15 2:47 UTC (permalink / raw)
To: weimin xiong, linux-rdma, yanjun.zhu@linux.dev
Cc: jgg, xiongweimin, leon@kernel.org
在 2026/7/13 18:55, weimin xiong 写道:
> From: xiongweimin <xiongweimin@kylinos.cn>
>
> rxe_init_packet() assigns skb->dev from an RCU-protected GID attribute
> without holding a netdev reference. If the netdev is unregistered before
> the skb is freed, subsequent accesses to skb->dev are unsafe.
>
> Hold a reference with dev_hold() when the skb is initialized and release
> it from the transmit destructor or via rxe_put_skb() on error paths that
> run before the destructor is installed.
>
> To avoid blocking netdev unregistration on held skbs, flush all QPs to
> the error state on NETDEV_GOING_DOWN and NETDEV_UNREGISTER so pending TX
> work is drained and references can be dropped.
>
> Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
> ---
> drivers/infiniband/sw/rxe/rxe_loc.h | 1 +
> drivers/infiniband/sw/rxe/rxe_net.c | 48 ++++++++++++++++++++++++++--
> drivers/infiniband/sw/rxe/rxe_req.c | 2 +-
> drivers/infiniband/sw/rxe/rxe_resp.c | 4 +--
> 4 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index 64d636bf8..7c3cc48e8 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -92,6 +92,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
> /* rxe_net.c */
> struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> int paylen, struct rxe_pkt_info *pkt);
> +void rxe_put_skb(struct sk_buff *skb);
> int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
> struct sk_buff *skb);
> int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 3741b2c4b..44a16cb16 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -441,6 +441,22 @@ static void rxe_skb_tx_dtor(struct sk_buff *skb)
>
> rxe_put(qp);
> sock_put(skb->sk);
> + if (skb->dev) {
> + dev_put(skb->dev);
> + skb->dev = NULL;
> + }
Since rdma packets can be routed, during routing, the skb->dev can be
changed. That is, in the source host, the skb->dev is INT_A. This
dev_hold works on INT_A. But in the dst host, the skb->dev is INT_B. So
the dev_put works on INT_B. It is not correct.
Using skb->dev in the TX destructor (rxe_skb_tx_dtor) is fundamentally
incorrect because skb->dev is not guaranteed to remain unchanged
throughout the transmit path. When a packet traverses virtual devices
(e.g. VLAN, bonding, tunnels) or is forwarded through the IP stack, the
networking core may replace skb->dev with the actual egress device (for
example, ip_finish_output2() assigns skb->dev = dst->dev).
As a result, the destructor may no longer be operating on the original
RXE/RoCE device, leading to two serious issues:
Reference count corruption: dev_put(skb->dev) may drop the reference
count of the new egress device, even though no corresponding dev_hold()
was performed. This can result in a reference count underflow,
use-after-free, or even a kernel crash.
Reference leak: The reference acquired for the original RXE/RoCE
netdevice is never released, leaking the device reference. In practice,
this can prevent the device from being freed and lead to the familiar
"waiting for ethX to become free" hang during interface teardown.
Do not rely on skb->dev in the destructor. Instead, preserve the
original netdevice pointer explicitly (for example, in skb->cb, if
appropriate) or manage its lifetime through the associated QP/port
context. The corresponding dev_put() should always be performed on the
same pointer that was previously passed to dev_hold().
Zhu Yanjun
> +}
> +
> +/*
> + * Free an skb that still holds a netdev reference from rxe_init_packet()
> + * and does not yet have rxe_skb_tx_dtor() installed. Once the TX
> + * destructor is set, callers must use kfree_skb() instead.
> + */
> +void rxe_put_skb(struct sk_buff *skb)
> +{
> + if (skb->dev)
> + dev_put(skb->dev);
> + kfree_skb(skb);
> }
>
> static int rxe_send(struct sk_buff *skb, struct rxe_pkt_info *pkt)
> @@ -529,7 +545,7 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> goto done;
>
> drop:
> - kfree_skb(skb);
> + rxe_put_skb(skb);
> err = 0;
> done:
> return err;
> @@ -574,7 +590,7 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>
> skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
>
> - /* FIXME: hold reference to this netdev until life of this skb. */
> + dev_hold(ndev);
> skb->dev = ndev;
> rcu_read_unlock();
>
> @@ -710,6 +726,28 @@ void rxe_set_port_state(struct rxe_dev *rxe)
> dev_put(ndev);
> }
>
> +/*
> + * Move all QPs to the error state so pending send/recv work is drained and
> + * in-flight TX skbs (which hold a netdev reference) can be released. Called
> + * from the netdev notifier so unregister cannot stall on held skbs.
> + */
> +static void rxe_flush_qps(struct rxe_dev *rxe)
> +{
> + struct rxe_pool_elem *elem;
> + struct rxe_qp *qp;
> + unsigned long index;
> +
> + rcu_read_lock();
> + xa_for_each(&rxe->qp_pool.xa, index, elem) {
> + if (!elem || !kref_get_unless_zero(&elem->ref_cnt))
> + continue;
> + qp = elem->obj;
> + rxe_qp_error(qp);
> + rxe_put(qp);
> + }
> + rcu_read_unlock();
> +}
> +
> static int rxe_notify(struct notifier_block *not_blk,
> unsigned long event,
> void *arg)
> @@ -721,7 +759,12 @@ static int rxe_notify(struct notifier_block *not_blk,
> return NOTIFY_OK;
>
> switch (event) {
> + case NETDEV_GOING_DOWN:
> + /* Start draining TX queues before the netdev disappears. */
> + rxe_flush_qps(rxe);
> + break;
> case NETDEV_UNREGISTER:
> + rxe_flush_qps(rxe);
> ib_unregister_device_queued(&rxe->ib_dev);
> rxe_net_del(&rxe->ib_dev);
> break;
> @@ -735,7 +778,6 @@ static int rxe_notify(struct notifier_block *not_blk,
> rxe_counter_inc(rxe, RXE_CNT_LINK_DOWNED);
> break;
> case NETDEV_REBOOT:
> - case NETDEV_GOING_DOWN:
> case NETDEV_CHANGEADDR:
> case NETDEV_CHANGENAME:
> case NETDEV_FEAT_CHANGE:
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 12d03f390..927ef68de 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -796,7 +796,7 @@ int rxe_requester(struct rxe_qp *qp)
> wqe->status = IB_WC_LOC_PROT_ERR;
> else
> wqe->status = IB_WC_LOC_QP_OP_ERR;
> - kfree_skb(skb);
> + rxe_put_skb(skb);
> if (ah)
> rxe_put(ah);
> goto err;
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index d8cbdfa70..ee3630e4b 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -866,7 +866,7 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
>
> err = rxe_prepare(&qp->pri_av, ack, skb);
> if (err) {
> - kfree_skb(skb);
> + rxe_put_skb(skb);
> return NULL;
> }
>
> @@ -994,7 +994,7 @@ static enum resp_states read_reply(struct rxe_qp *qp,
> err = rxe_mr_copy(mr, res->read.va, payload_addr(&ack_pkt),
> payload, RXE_FROM_MR_OBJ);
> if (err) {
> - kfree_skb(skb);
> + rxe_put_skb(skb);
> state = RESPST_ERR_RKEY_VIOLATION;
> goto err_out;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs
2026-07-14 1:55 ` [PATCH v2] " weimin xiong
2026-07-15 2:47 ` Zhu Yanjun
@ 2026-07-15 6:41 ` weimin xiong
1 sibling, 0 replies; 5+ messages in thread
From: weimin xiong @ 2026-07-15 6:41 UTC (permalink / raw)
To: linux-rdma; +Cc: jgg, zyjzyj2000, xiongweimin
From: xiongweimin <xiongweimin@kylinos.cn>
rxe_init_packet() assigns skb->dev from an RCU-protected GID attribute
without holding a netdev reference. If the netdev is unregistered before
the skb is freed, subsequent accesses to skb->dev are unsafe.
Hold a reference with dev_hold() when the skb is initialized and release
it from the transmit destructor or via rxe_put_skb() on error paths that
run before the destructor is installed.
skb->dev can change on the TX path (VLAN/bond/tunnel, ip_finish_output2,
etc.), so put must use the same netdev that was held. Stash that pointer
in skb_shinfo()->destructor_arg: skb->cb is already used by
rxe_pkt_info and is rewritten by IP control blocks.
To avoid blocking netdev unregistration on held skbs, flush all QPs to
the error state on NETDEV_GOING_DOWN and NETDEV_UNREGISTER so pending TX
work is drained and references can be dropped.
Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
---
v3:
- put the held netdev from destructor_arg, not live skb->dev
(skb->dev may be rewritten by the TX stack)
v2:
- rebase on current mainline so the patch applies cleanly
- flush all QPs on NETDEV_GOING_DOWN and NETDEV_UNREGISTER to
release pending TX skbs before netdev unregister
---
drivers/infiniband/sw/rxe/rxe_loc.h | 1 +
drivers/infiniband/sw/rxe/rxe_net.c | 68 ++++++++++++++++++++++++++--
drivers/infiniband/sw/rxe/rxe_req.c | 2 +-
drivers/infiniband/sw/rxe/rxe_resp.c | 4 +-
4 files changed, 68 insertions(+), 7 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index 64d636bf8..7c3cc48e8 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -92,6 +92,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
/* rxe_net.c */
struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
int paylen, struct rxe_pkt_info *pkt);
+void rxe_put_skb(struct sk_buff *skb);
int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
struct sk_buff *skb);
int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index 3741b2c4b..86c9b19f6 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -429,6 +429,29 @@ int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
return err;
}
+/*
+ * skb->dev may be rewritten on the TX path (VLAN/bond/tunnel, etc.).
+ * The netdev we held in rxe_init_packet() is kept in destructor_arg so
+ * that put always matches hold. skb->cb cannot be used: it is already
+ * occupied by struct rxe_pkt_info and rewritten by IP control blocks.
+ */
+static void rxe_skb_set_held_ndev(struct sk_buff *skb, struct net_device *ndev)
+{
+ dev_hold(ndev);
+ skb->dev = ndev;
+ skb_shinfo(skb)->destructor_arg = ndev;
+}
+
+static void rxe_skb_put_held_ndev(struct sk_buff *skb)
+{
+ struct net_device *ndev = skb_shinfo(skb)->destructor_arg;
+
+ if (ndev) {
+ skb_shinfo(skb)->destructor_arg = NULL;
+ dev_put(ndev);
+ }
+}
+
static void rxe_skb_tx_dtor(struct sk_buff *skb)
{
struct rxe_qp *qp = skb->sk->sk_user_data;
@@ -441,6 +464,18 @@ static void rxe_skb_tx_dtor(struct sk_buff *skb)
rxe_put(qp);
sock_put(skb->sk);
+ rxe_skb_put_held_ndev(skb);
+}
+
+/*
+ * Free an skb that still holds a netdev reference from rxe_init_packet()
+ * and does not yet have rxe_skb_tx_dtor() installed. Once the TX
+ * destructor is set, callers must use kfree_skb() instead.
+ */
+void rxe_put_skb(struct sk_buff *skb)
+{
+ rxe_skb_put_held_ndev(skb);
+ kfree_skb(skb);
}
static int rxe_send(struct sk_buff *skb, struct rxe_pkt_info *pkt)
@@ -529,7 +564,7 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
goto done;
drop:
- kfree_skb(skb);
+ rxe_put_skb(skb);
err = 0;
done:
return err;
@@ -574,8 +609,7 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
- /* FIXME: hold reference to this netdev until life of this skb. */
- skb->dev = ndev;
+ rxe_skb_set_held_ndev(skb, ndev);
rcu_read_unlock();
if (av->network_type == RXE_NETWORK_TYPE_IPV4)
@@ -710,6 +744,28 @@ void rxe_set_port_state(struct rxe_dev *rxe)
dev_put(ndev);
}
+/*
+ * Move all QPs to the error state so pending send/recv work is drained and
+ * in-flight TX skbs (which hold a netdev reference) can be released. Called
+ * from the netdev notifier so unregister cannot stall on held skbs.
+ */
+static void rxe_flush_qps(struct rxe_dev *rxe)
+{
+ struct rxe_pool_elem *elem;
+ struct rxe_qp *qp;
+ unsigned long index;
+
+ rcu_read_lock();
+ xa_for_each(&rxe->qp_pool.xa, index, elem) {
+ if (!elem || !kref_get_unless_zero(&elem->ref_cnt))
+ continue;
+ qp = elem->obj;
+ rxe_qp_error(qp);
+ rxe_put(qp);
+ }
+ rcu_read_unlock();
+}
+
static int rxe_notify(struct notifier_block *not_blk,
unsigned long event,
void *arg)
@@ -721,7 +777,12 @@ static int rxe_notify(struct notifier_block *not_blk,
return NOTIFY_OK;
switch (event) {
+ case NETDEV_GOING_DOWN:
+ /* Start draining TX queues before the netdev disappears. */
+ rxe_flush_qps(rxe);
+ break;
case NETDEV_UNREGISTER:
+ rxe_flush_qps(rxe);
ib_unregister_device_queued(&rxe->ib_dev);
rxe_net_del(&rxe->ib_dev);
break;
@@ -735,7 +796,6 @@ static int rxe_notify(struct notifier_block *not_blk,
rxe_counter_inc(rxe, RXE_CNT_LINK_DOWNED);
break;
case NETDEV_REBOOT:
- case NETDEV_GOING_DOWN:
case NETDEV_CHANGEADDR:
case NETDEV_CHANGENAME:
case NETDEV_FEAT_CHANGE:
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 12d03f390..927ef68de 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -796,7 +796,7 @@ int rxe_requester(struct rxe_qp *qp)
wqe->status = IB_WC_LOC_PROT_ERR;
else
wqe->status = IB_WC_LOC_QP_OP_ERR;
- kfree_skb(skb);
+ rxe_put_skb(skb);
if (ah)
rxe_put(ah);
goto err;
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index d8cbdfa70..ee3630e4b 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -866,7 +866,7 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
err = rxe_prepare(&qp->pri_av, ack, skb);
if (err) {
- kfree_skb(skb);
+ rxe_put_skb(skb);
return NULL;
}
@@ -994,7 +994,7 @@ static enum resp_states read_reply(struct rxe_qp *qp,
err = rxe_mr_copy(mr, res->read.va, payload_addr(&ack_pkt),
payload, RXE_FROM_MR_OBJ);
if (err) {
- kfree_skb(skb);
+ rxe_put_skb(skb);
state = RESPST_ERR_RKEY_VIOLATION;
goto err_out;
}
--
2.43.0
No virus found
Checked by Hillstone Network AntiVirus
^ permalink raw reply related [flat|nested] 5+ messages in thread