* [PATCH] RDMA/rxe: Hold netdev reference for transmit skbs
@ 2026-07-13 10:03 weimin xiong
2026-07-14 0:02 ` Zhu Yanjun
2026-07-14 1:55 ` [PATCH v2] " weimin xiong
0 siblings, 2 replies; 10+ messages in thread
From: weimin xiong @ 2026-07-13 10:03 UTC (permalink / raw)
To: linux-rdma; +Cc: jgg, 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.
Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
Cc: linux-rdma@vger.kernel.org
Cc: Jason Gunthorpe <jgg@nvidia.com>
---
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -95,6 +95,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,
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -355,6 +355,16 @@ 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;
+ }
+}
+
+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)
@@ -441,7 +451,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;
@@ -486,8 +496,8 @@ 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();
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -793,7 +793,7 @@ next_wqe:
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;
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -817,7 +817,7 @@ static struct sk_buff *copy_data(struct rxe_qp *qp,
err = rxe_prepare(&qp->pri_av, ack, skb);
if (err) {
- kfree_skb(skb);
+ rxe_put_skb(skb);
return NULL;
}
@@ -945,7 +945,7 @@ static enum resp_states read(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] 10+ messages in thread* Re: [PATCH] RDMA/rxe: Hold netdev reference for transmit skbs 2026-07-13 10:03 [PATCH] RDMA/rxe: Hold netdev reference for transmit skbs weimin xiong @ 2026-07-14 0:02 ` Zhu Yanjun 2026-07-14 1:55 ` [PATCH v2] " weimin xiong 1 sibling, 0 replies; 10+ messages in thread From: Zhu Yanjun @ 2026-07-14 0:02 UTC (permalink / raw) To: weimin xiong, linux-rdma, yanjun.zhu@linux.dev; +Cc: jgg, xiongweimin 在 2026/7/13 3:03, 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. > > Signed-off-by: xiongweimin <xiongweimin@kylinos.cn> This commit can not be applied in linux kernel upstream cleanly. The logic of holding a netdev reference to guarantee memory safety (preventing Use-After-Free) is correct. But, this introduces the classic risk of blocking netdev unregistration if an skb is leaked or held indefinitely. To mitigate this and ensure clean netdev unregistration: Ensure the driver properly handles `NETDEV_UNREGISTER` notifications by immediately flushing all QPs/TX queues to release pending skbs under unregistration events. So please modify this commit to apply this commit on linux upstream. Zhu Yanjun > Cc: linux-rdma@vger.kernel.org > Cc: Jason Gunthorpe <jgg@nvidia.com> > --- > > --- a/drivers/infiniband/sw/rxe/rxe_loc.h > +++ b/drivers/infiniband/sw/rxe/rxe_loc.h > @@ -95,6 +95,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, > --- a/drivers/infiniband/sw/rxe/rxe_net.c > +++ b/drivers/infiniband/sw/rxe/rxe_net.c > @@ -355,6 +355,16 @@ 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; > + } > +} > + > +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) > @@ -441,7 +451,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; > @@ -486,8 +496,8 @@ 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(); > > --- a/drivers/infiniband/sw/rxe/rxe_req.c > +++ b/drivers/infiniband/sw/rxe/rxe_req.c > @@ -793,7 +793,7 @@ next_wqe: > 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; > --- a/drivers/infiniband/sw/rxe/rxe_resp.c > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c > @@ -817,7 +817,7 @@ static struct sk_buff *copy_data(struct rxe_qp *qp, > > err = rxe_prepare(&qp->pri_av, ack, skb); > if (err) { > - kfree_skb(skb); > + rxe_put_skb(skb); > return NULL; > } > > @@ -945,7 +945,7 @@ static enum resp_states read(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] 10+ messages in thread
* [PATCH v2] RDMA/rxe: Hold netdev reference for transmit skbs 2026-07-13 10:03 [PATCH] RDMA/rxe: Hold netdev reference for transmit skbs weimin xiong 2026-07-14 0:02 ` Zhu Yanjun @ 2026-07-14 1:55 ` weimin xiong 2026-07-15 2:47 ` Zhu Yanjun 2026-07-15 6:41 ` [PATCH v3] " weimin xiong 1 sibling, 2 replies; 10+ messages in thread From: weimin xiong @ 2026-07-14 1:55 UTC (permalink / raw) To: linux-rdma; +Cc: jgg, 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. 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; + } +} + +/* + * 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; } -- 2.43.0 No virus found Checked by Hillstone Network AntiVirus ^ permalink raw reply related [flat|nested] 10+ messages in thread
* 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; 10+ 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] 10+ 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 2026-07-16 2:27 ` yanjun.zhu 1 sibling, 1 reply; 10+ 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] 10+ messages in thread
* Re: [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs 2026-07-15 6:41 ` [PATCH v3] " weimin xiong @ 2026-07-16 2:27 ` yanjun.zhu 2026-07-16 2:56 ` Yanjun.Zhu 2026-07-16 3:04 ` Xiong Weimin 0 siblings, 2 replies; 10+ messages in thread From: yanjun.zhu @ 2026-07-16 2:27 UTC (permalink / raw) To: weimin xiong, linux-rdma, Zhu Yanjun Cc: jgg, zyjzyj2000, xiongweimin, Leon Romanovsky On 7/14/26 11:41 PM, weimin xiong wrote: > 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) I am fine with this commit. Just a reminder: please send a new version of a patch in a new email thread. It is better not to send multiple versions of the same patch in the same thread. Other than that, thanks a lot! Let us wait for the feedback from Leon and Jason. Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> Zhu Yanjun > > 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; > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs 2026-07-16 2:27 ` yanjun.zhu @ 2026-07-16 2:56 ` Yanjun.Zhu 2026-07-16 3:11 ` Xiong Weimin 2026-07-16 3:04 ` Xiong Weimin 1 sibling, 1 reply; 10+ messages in thread From: Yanjun.Zhu @ 2026-07-16 2:56 UTC (permalink / raw) To: weimin xiong, linux-rdma, Zhu Yanjun Cc: jgg, zyjzyj2000, xiongweimin, Leon Romanovsky On 7/15/26 7:27 PM, yanjun.zhu wrote: > On 7/14/26 11:41 PM, weimin xiong wrote: >> 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) > > I am fine with this commit. Just a reminder: please send a new version > of a patch in a new email thread. It is better not to send multiple > versions of the same patch in the same thread. > > Other than that, thanks a lot! > Let us wait for the feedback from Leon and Jason. > > Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> > > Zhu Yanjun > >> >> 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; To be honest, I'm confused about this `ndev` pointer. `ndev` is a pointer to a `struct net_device` on Host A (the source host). From what I understand, the current code appears to transfer this `ndev` pointer to Host B (the destination host). On Host A, `dev_hold(ndev)` is called, while on Host B, `dev_put(ndev)` is called. I don't understand how this can work. Host A and Host B have completely separate virtual address spaces, so a pointer value from Host A should be meaningless on Host B. Could you explain how this pointer remains valid across two different hosts? Zhu Yanjun >> +} >> + >> +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; >> } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re:Re: [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs 2026-07-16 2:56 ` Yanjun.Zhu @ 2026-07-16 3:11 ` Xiong Weimin 2026-07-16 3:17 ` Yanjun.Zhu 0 siblings, 1 reply; 10+ messages in thread From: Xiong Weimin @ 2026-07-16 3:11 UTC (permalink / raw) To: Yanjun.Zhu; +Cc: linux-rdma, jgg, zyjzyj2000, xiongweimin, Leon Romanovsky Subject: Re: [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs Hi Yanjun, Thanks for taking the time to review. Let me clarify the question about the ndev pointer. The ndev pointer (struct net_device *) is a kernel virtual address that lives entirely in the local host's kernel address space. It is never serialized or transmitted over the wire. The RDMA/RoCE packet on the wire only contains protocol headers (ETH, IP, UDP, BTH) and payload data — no kernel pointers. The flow is: 1. rxe_init_packet() allocates an skb and calls dev_hold(ndev) to take a reference on the local net_device. This prevents the net_device from being unregistered (freeing the struct net_device memory) while the skb still references it. 2. The skb is passed to the kernel's IP stack via ip_local_out(). The IP stack may rewrite skb->dev during processing (e.g., VLAN, bonding, tunnel encapsulation) — this is why we stash the held ndev in skb_shinfo()->destructor_arg. 3. When the TX completes, rxe_skb_tx_dtor() calls dev_put() on the stashed ndev from destructor_arg (not the live skb->dev), releasing the reference. 4. Host B has its own completely separate struct net_device instances in its own kernel address space. It never sees or receives Host A's ndev pointer value. This is purely a reference counting fix for the local host's netdev lifetime — it ensures the net_device structure doesn't disappear while an in-flight TX skb still holds a pointer to it. Hope this clarifies. Please let me know if you have further questions. Best regards, xiongweimin At 2026-07-16 10:56:20, "Yanjun.Zhu" <yanjun.zhu@linux.dev> wrote: > >On 7/15/26 7:27 PM, yanjun.zhu wrote: >> On 7/14/26 11:41 PM, weimin xiong wrote: >>> 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) >> >> I am fine with this commit. Just a reminder: please send a new version >> of a patch in a new email thread. It is better not to send multiple >> versions of the same patch in the same thread. >> >> Other than that, thanks a lot! >> Let us wait for the feedback from Leon and Jason. >> >> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> >> >> Zhu Yanjun >> >>> >>> 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; > >To be honest, I'm confused about this `ndev` pointer. > >`ndev` is a pointer to a `struct net_device` on Host A (the source >host). From what I understand, the current code appears to transfer this >`ndev` pointer to Host B (the destination host). > >On Host A, `dev_hold(ndev)` is called, while on Host B, `dev_put(ndev)` >is called. > >I don't understand how this can work. Host A and Host B have completely >separate virtual address spaces, so a pointer value from Host A should >be meaningless on Host B. Could you explain how this pointer remains >valid across two different hosts? > >Zhu Yanjun > >>> +} >>> + >>> +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; >>> } >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs 2026-07-16 3:11 ` Xiong Weimin @ 2026-07-16 3:17 ` Yanjun.Zhu 0 siblings, 0 replies; 10+ messages in thread From: Yanjun.Zhu @ 2026-07-16 3:17 UTC (permalink / raw) To: Xiong Weimin; +Cc: linux-rdma, jgg, zyjzyj2000, xiongweimin, Leon Romanovsky On 7/15/26 8:11 PM, Xiong Weimin wrote: > > > Subject: Re: [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs > Hi Yanjun, > Thanks for taking the time to review. Let me clarify the question about the ndev pointer. > The ndev pointer (struct net_device *) is a kernel virtual address that lives entirely in the local host's kernel address space. It is never serialized or transmitted over the wire. The RDMA/RoCE packet on the wire only contains protocol headers (ETH, IP, UDP, BTH) and payload data — no kernel pointers. > The flow is: > 1. rxe_init_packet() allocates an skb and calls dev_hold(ndev) to take a reference on the local net_device. This prevents the net_device from being unregistered (freeing the struct net_device memory) while the skb still references it. > 2. The skb is passed to the kernel's IP stack via ip_local_out(). The IP stack may rewrite skb->dev during processing (e.g., VLAN, bonding, tunnel encapsulation) — this is why we stash the held ndev in skb_shinfo()->destructor_arg. > 3. When the TX completes, rxe_skb_tx_dtor() calls dev_put() on the stashed ndev from destructor_arg (not the live skb->dev), releasing the reference. > 4. Host B has its own completely separate struct net_device instances in its own kernel address space. It never sees or receives Host A's ndev pointer value. > This is purely a reference counting fix for the local host's netdev lifetime — it ensures the net_device structure doesn't disappear while an in-flight TX skb still holds a pointer to it. > Hope this clarifies. Please let me know if you have further questions. Thanks a lot. Zhu Yanjun > > Best regards, > xiongweimin > > > At 2026-07-16 10:56:20, "Yanjun.Zhu" <yanjun.zhu@linux.dev> wrote: >> On 7/15/26 7:27 PM, yanjun.zhu wrote: >>> On 7/14/26 11:41 PM, weimin xiong wrote: >>>> 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) >>> I am fine with this commit. Just a reminder: please send a new version >>> of a patch in a new email thread. It is better not to send multiple >>> versions of the same patch in the same thread. >>> >>> Other than that, thanks a lot! >>> Let us wait for the feedback from Leon and Jason. >>> >>> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> >>> >>> Zhu Yanjun >>> >>>> 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; >> To be honest, I'm confused about this `ndev` pointer. >> >> `ndev` is a pointer to a `struct net_device` on Host A (the source >> host). From what I understand, the current code appears to transfer this >> `ndev` pointer to Host B (the destination host). >> >> On Host A, `dev_hold(ndev)` is called, while on Host B, `dev_put(ndev)` >> is called. >> >> I don't understand how this can work. Host A and Host B have completely >> separate virtual address spaces, so a pointer value from Host A should >> be meaningless on Host B. Could you explain how this pointer remains >> valid across two different hosts? >> >> Zhu Yanjun >> >>>> +} >>>> + >>>> +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; >>>> } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re:Re: [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs 2026-07-16 2:27 ` yanjun.zhu 2026-07-16 2:56 ` Yanjun.Zhu @ 2026-07-16 3:04 ` Xiong Weimin 1 sibling, 0 replies; 10+ messages in thread From: Xiong Weimin @ 2026-07-16 3:04 UTC (permalink / raw) To: yanjun.zhu; +Cc: linux-rdma, jgg, zyjzyj2000, xiongweimin, Leon Romanovsky Subject: Re: [PATCH v3] RDMA/rxe: Hold netdev reference for transmit skbs Hi Yanjun, Thanks for the Reviewd-by and the reminder about sending new versions in a separate thread! I'll keep that in mind for future submissions. Best regards, xiongweimin At 2026-07-16 10:27:24, "yanjun.zhu" <yanjun.zhu@linux.dev> wrote: >On 7/14/26 11:41 PM, weimin xiong wrote: >> 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) > >I am fine with this commit. Just a reminder: please send a new version >of a patch in a new email thread. It is better not to send multiple >versions of the same patch in the same thread. > >Other than that, thanks a lot! >Let us wait for the feedback from Leon and Jason. > >Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> > >Zhu Yanjun > >> >> 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; >> } ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-07-16 3:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-13 10:03 [PATCH] RDMA/rxe: Hold netdev reference for transmit skbs weimin xiong 2026-07-14 0:02 ` Zhu Yanjun 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 2026-07-16 2:27 ` yanjun.zhu 2026-07-16 2:56 ` Yanjun.Zhu 2026-07-16 3:11 ` Xiong Weimin 2026-07-16 3:17 ` Yanjun.Zhu 2026-07-16 3:04 ` Xiong Weimin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox