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

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

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