netdev - Re: [PATCH net v2 3/3] net: nixge: fix tx queue handling

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]

Message-ID: <Y3S7LpbVaTRdKmLl@electric-eye.fr.zoreil.com>
Date:   Wed, 16 Nov 2022 11:27:58 +0100
From:   Francois Romieu <romieu@...zoreil.com>
To:     Zhang Changzhong <zhangchangzhong@...wei.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, mdf@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2 3/3] net: nixge: fix tx queue handling

Zhang Changzhong <zhangchangzhong@...wei.com> :
> On 2022/11/16 7:04, Francois Romieu wrote:
> > Zhang Changzhong <zhangchangzhong@...wei.com> :
[...]
> >> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> >> index 91b7ebc..3776a03 100644
> >> --- a/drivers/net/ethernet/ni/nixge.c
> >> +++ b/drivers/net/ethernet/ni/nixge.c
> > [...]
> >>  static netdev_tx_t nixge_start_xmit(struct sk_buff *skb,
> >> @@ -518,10 +523,15 @@ static netdev_tx_t nixge_start_xmit(struct sk_buff *skb,
> >>  	cur_p = &priv->tx_bd_v[priv->tx_bd_tail];
> >>  	tx_skb = &priv->tx_skb[priv->tx_bd_tail];
> >>  
> >> -	if (nixge_check_tx_bd_space(priv, num_frag + 1)) {
> >> -		if (!netif_queue_stopped(ndev))
> >> -			netif_stop_queue(ndev);
> >> -		return NETDEV_TX_OK;
> >> +	if (unlikely(nixge_check_tx_bd_space(priv, num_frag + 1))) {
> >> +		/* Should not happen as last start_xmit call should have
> >> +		 * checked for sufficient space and queue should only be
> >> +		 * woken when sufficient space is available.
> >> +		 */
> > 
> > Almost. IRQ triggering after nixge_start_xmit::netif_stop_queue and
> > before nixge_start_xmit::smp_mb may wrongly wake queue.
> > 
> 
> I don't know what you mean by "wronly wake queue". The queue is woken
> only when there is sufficient for next packet.

Between nixge_start_xmit::netif_stop_queue and nixge_start_xmit::smp_mb,
"next" packet is current packet in hard_start_xmit. However said current
packet may not be accounted for in the IRQ context transmit completion
handler.

[nixge_start_xmit]

        ++priv->tx_bd_tail;
        priv->tx_bd_tail %= TX_BD_NUM;

	/* Stop queue if next transmit may not have space */
	if (nixge_check_tx_bd_space(priv, MAX_SKB_FRAGS + 1)) {
                netif_stop_queue(ndev);

Which value does [nixge_start_xmit_done] read as priv->tx_bd_tail at
this point ? The value set a few lines above or some older value ?

		/* Matches barrier in nixge_start_xmit_done */
		smp_mb();

		/* Space might have just been freed - check again */
		if (!nixge_check_tx_bd_space(priv, MAX_SKB_FRAGS + 1))
			netif_wake_queue(ndev);
	}

-- 
Ueimor