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

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

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