netdev - Re: [PATCH net] veth: Avoid drop packets when xdp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 28 Sep 2022 19:57:34 +0800
From:   Heng Qi <hengqi@...ux.alibaba.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Subject: Re: [PATCH net] veth: Avoid drop packets when xdp_redirect performs



在 2022/9/27 下午8:20, Toke Høiland-Jørgensen 写道:
> Heng Qi <hengqi@...ux.alibaba.com> writes:
>
>> In the current processing logic, when xdp_redirect occurs, it transmits
>> the xdp frame based on napi.
>>
>> If napi of the peer veth is not ready, the veth will drop the packets.
>> This doesn't meet our expectations.
> Erm, why don't you just enable NAPI? Loading an XDP program is not
> needed these days, you can just enable GRO on both peers...

In general, we don't expect veth to drop packets when it doesn't mount
the xdp program or otherwise, because this is not as expected.

>> In this context, if napi is not ready, we convert the xdp frame to a skb,
>> and then use veth_xmit() to deliver it to the peer veth.
>>
>> Like the following case:
>> Even if veth1's napi cannot be used, the packet redirected from the NIC
>> will be transmitted to veth1 successfully:
>>
>> NIC   ->   veth0----veth1
>>   |                   |
>> (XDP)             (no XDP)
>>
>> Signed-off-by: Heng Qi <hengqi@...ux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
>> ---
>>   drivers/net/veth.c | 36 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> index 466da01..e1f5561 100644
>> --- a/drivers/net/veth.c
>> +++ b/drivers/net/veth.c
>> @@ -469,8 +469,42 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
>>   	/* The napi pointer is set if NAPI is enabled, which ensures that
>>   	 * xdp_ring is initialized on receive side and the peer device is up.
>>   	 */
>> -	if (!rcu_access_pointer(rq->napi))
>> +	if (!rcu_access_pointer(rq->napi)) {
>> +		for (i = 0; i < n; i++) {
>> +			struct xdp_frame *xdpf = frames[i];
>> +			struct netdev_queue *txq = NULL;
>> +			struct sk_buff *skb;
>> +			int queue_mapping;
>> +			u16 mac_len;
>> +
>> +			skb = xdp_build_skb_from_frame(xdpf, dev);
>> +			if (unlikely(!skb)) {
>> +				ret = nxmit;
>> +				goto out;
>> +			}
>> +
>> +			/* We need to restore ETH header, because it is pulled
>> +			 * in eth_type_trans.
>> +			 */
>> +			mac_len = skb->data - skb_mac_header(skb);
>> +			skb_push(skb, mac_len);
>> +
>> +			nxmit++;
>> +
>> +			queue_mapping = skb_get_queue_mapping(skb);
>> +			txq = netdev_get_tx_queue(dev, netdev_cap_txqueue(dev, queue_mapping));
>> +			__netif_tx_lock(txq, smp_processor_id());
>> +			if (unlikely(veth_xmit(skb, dev) != NETDEV_TX_OK)) {
>> +				__netif_tx_unlock(txq);
>> +				ret = nxmit;
>> +				goto out;
>> +			}
>> +			__netif_tx_unlock(txq);
> Locking and unlocking the txq repeatedly for each packet? Yikes! Did you
> measure the performance overhead of this?

Yes, there are indeed some optimizations that can be done here,
like putting the lock outside the loop.
But in __dev_queue_xmit(), where each packet sent is also protected by a lock.

Thanks.

>
> -Toke