netdev - Re: [PATCH net] net: renesas: rswitch: Fix return value in error path of xmit

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: <20230601091012.5633540f@kernel.org>
Date: Thu, 1 Jun 2023 09:10:12 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Simon Horman <simon.horman@...igine.com>, Yoshihiro Shimoda 
 <yoshihiro.shimoda.uh@...esas.com>, s.shtylyov@....ru, davem@...emloft.net,
 edumazet@...gle.com, netdev@...r.kernel.org,
 linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH net] net: renesas: rswitch: Fix return value in error
 path of xmit

On Thu, 01 Jun 2023 10:41:34 +0200 Paolo Abeni wrote:
> > I agree that this is the correct return value for this case.
> > But I do wonder if, as per the documentation of ndo_start_xmit,
> > something should be done to avoid getting into such a situation.
> > 
> >  * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb,
> >  *                               struct net_device *dev);
> >  *      Called when a packet needs to be transmitted.
> >  *      Returns NETDEV_TX_OK.  Can return NETDEV_TX_BUSY, but you should stop
> >  *      the queue before that can happen; it's for obsolete devices and weird
> >  *      corner cases, but the stack really does a non-trivial amount
> >  *      of useless work if you return NETDEV_TX_BUSY.
> >  *      Required; cannot be NULL.  
> 
> I agree with Simon, it looks like the driver usage of
> netif_stop_subqueue()/netif_wake_subqueue() is a dubious.
> 
> I think you will be better of using
> netif_subqueue_maybe_stop()/netif_subqueue_completed_wake() alike what
> rtl8169 is doing. e.g. netif_subqueue_maybe_stop() should be invoked
> after the tx buffer enqueue, and netif_subqueue_completed_wake() should
> be invoked after successful tx ring cleanup.

That's a separate issue, tho, right? The cleanup is lockless and our
magic lockless macro scheme does not protect from spurious wakeups.
So they still need to check if the queue is full at the top of xmit.
And they still need to return the correct error in that case..