netdev - Re: [PATCH net-next 4/5] veth: introduce more xdp counters

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 for Android: free password hash cracker in your pocket

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

Message-ID: <04ca75e8-1291-4f25-3ad4-18ca5d6c6ddb@gmail.com>
Date:   Sat, 21 Mar 2020 22:38:46 +0900
From:   Toshiaki Makita <toshiaki.makita1@...il.com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, brouer@...hat.com,
        dsahern@...il.com, lorenzo.bianconi@...hat.com, toke@...hat.com
Subject: Re: [PATCH net-next 4/5] veth: introduce more xdp counters

On 2020/03/20 22:37, Lorenzo Bianconi wrote:
>> On 2020/03/20 1:41, Lorenzo Bianconi wrote:
>>> Introduce xdp_xmit counter in order to distinguish between XDP_TX and
>>> ndo_xdp_xmit stats. Introduce the following ethtool counters:
>>> - rx_xdp_tx
>>> - rx_xdp_tx_errors
>>> - tx_xdp_xmit
>>> - tx_xdp_xmit_errors
>>> - rx_xdp_redirect
>>
>> Thank you for working on this!
>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
>>> ---
>> ...
>>> @@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
>>>    	}
>>>    	rcv_priv = netdev_priv(rcv);
>>> -	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
>>> +	qidx = veth_select_rxq(rcv);
>>> +	rq = &rcv_priv->rq[qidx];
>>>    	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
>>>    	 * side. This means an XDP program is loaded on the peer and the peer
>>>    	 * device is up.
>>> @@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
>>>    	if (flags & XDP_XMIT_FLUSH)
>>>    		__veth_xdp_flush(rq);
>>> +	rq = &priv->rq[qidx];
>>
>> I think there is no guarantee that this rq exists. Qidx is less than
>> rcv->real_num_rx_queues, but not necessarily less than
>> dev->real_num_rx_queues.
>>
>>> +	u64_stats_update_begin(&rq->stats.syncp);
>>
>> So this can cuase NULL pointer dereference.
> 
> oh right, thanks for spotting this.
> I think we can recompute qidx for tx netdevice in this case, doing something
> like:
> 
> qidx = veth_select_rxq(dev);
> rq = &priv->rq[qidx];
> 
> what do you think?

This would not cause NULL pointer deref, but I wonder what counters 
you've added mean.

- rx_xdp_redirect, rx_xdp_drops, rx_xdp_tx

These counters names will be rx_queue_[i]_rx_xdp_[redirect|drops|tx].
"rx_" in their names looks redundant.
Also it looks like there is not "rx[i]_xdp_tx" counter but there is 
"rx[i]_xdp_tx_xmit" in mlx5 from this page.
https://community.mellanox.com/s/article/understanding-mlx5-ethtool-counters

- tx_xdp_xmit, tx_xdp_xmit_errors

These counters names will be rx_queue_[i]_tx_xdp_[xmit|xmit_errors].
Are these rx counters or tx counters?
If tx, currently there is no storage to store these tx counters so 
adding these would not be simple.
If rx, I guess you should use peer rx queue counters instead of dev rx 
queue counters.

Toshiaki Makita