Re: [PATCH v2] RDMA/rxe: Hold netdev reference for transmit skbs

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 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;
>   	}