netdev - Re: [PATCH V2 6/8] net: mediatek: fix TX locking

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: <5706B927.8090309@cogentembedded.com>
Date:	Thu, 7 Apr 2016 22:46:47 +0300
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	John Crispin <blogic@...nwrt.org>,
	"David S. Miller" <davem@...emloft.net>
Cc:	Felix Fietkau <nbd@...nwrt.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Sean Wang (王志亘) <sean.wang@...iatek.com>,
	netdev@...r.kernel.org, linux-mediatek@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 6/8] net: mediatek: fix TX locking

Hello.

On 04/07/2016 10:26 PM, John Crispin wrote:

> Inside the TX path there is a lock inside the tx_map function. This is
> however too late. The patch moves the lock to the start of the xmit
> function right before the free count check of the DMA ring happens.
> If we do not do this, the code becomes racy leading to TX stalls and
> dropped packets. This happens as there are 2 netdevs running on the
> same physical DMA ring.
>
> Signed-off-by: John Crispin <blogic@...nwrt.org>
> ---
>   drivers/net/ethernet/mediatek/mtk_eth_soc.c |   20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 60b66ab..8434355 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
[...]
> @@ -712,14 +702,22 @@ static int mtk_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	struct mtk_eth *eth = mac->hw;
>   	struct mtk_tx_ring *ring = &eth->tx_ring;
>   	struct net_device_stats *stats = &dev->stats;
> +	unsigned long flags;
>   	bool gso = false;
>   	int tx_num;
>
> +	/* normally we can rely on the stack not calling this more than once,
> +	 * however we have 2 queues running ont he same ring so we need to lock

    s/ont he/ on the/, perhaps a good chance to fix the comment?

> +	 * the ring access
> +	 */
> +	spin_lock_irqsave(&eth->page_lock, flags);
> +
>   	tx_num = mtk_cal_txd_req(skb);
>   	if (unlikely(atomic_read(&ring->free_count) <= tx_num)) {
>   		mtk_stop_queue(eth);
>   		netif_err(eth, tx_queued, dev,
>   			  "Tx Ring full when queue awake!\n");
> +		spin_unlock_irqrestore(&eth->page_lock, flags);
>   		return NETDEV_TX_BUSY;
>   	}
>
[...]

MBR, Sergei