linux-kernel - Re: [PATCH] Revert "net: sched: drop all special handling of tx_queue

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: <20160217135847.GB23205@orbyte.nwl.cc>
Date:	Wed, 17 Feb 2016 14:58:47 +0100
From:	Phil Sutter <phil@....cc>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	"David S . Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>,
	netdev@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] Revert "net: sched: drop all special handling of
 tx_queue_len == 0"

On Wed, Feb 17, 2016 at 01:57:42PM +0000, Mathieu Desnoyers wrote:
> ----- On Feb 17, 2016, at 7:47 AM, Phil Sutter phil@....cc wrote:
> 
> > Hi,
> > 
> > On Tue, Feb 16, 2016 at 07:56:23PM -0500, Mathieu Desnoyers wrote:
> >> This reverts commit 348e3435cbefa815bd56a5205c1412b5afe7b92e.
> >> It breaks HTB classful qdiscs on the loopback interface.
> >> 
> >> It has been broken since kernel v4.2. The offending commit has
> >> been identified by bissection of the issue with the following
> >> test-case. It appears that the loopback interface does indeed
> >> still have tx_queue_len == 0.
> >> 
> >> Reverting the commit on a v4.4.1 kernel fixes the issue.
> > 
> > Indeed, this is ugly. Affected are all drivers not calling ether_setup()
> > in their setup callback, and therefore not initializing tx_queue_len.
> > 
> > As the commit to be reverted shows, there is no common fallback value
> > for tx_queue_len - most qdisc implementations used 1, but HTB fell back
> > to 2 and PLUG to 100. But as the removed comment in sch_plug.c stated:
> > 
> >| /* We will set a default limit of 100 pkts (~150kB)
> >|  * in case tx_queue_len is not available. The
> >|  * default value is completely arbitrary.
> >|  */
> > 
> > It seems not to be overly important to fallback to the exact value each
> > qdisc had before. Therefore I guess the following change should
> > appropriately fix the issue at hand:
> > 
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -7440,8 +7440,10 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv,
> > const char *name,
> >        dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
> >        setup(dev);
> > 
> > -       if (!dev->tx_queue_len)
> > +       if (!dev->tx_queue_len) {
> >                dev->priv_flags |= IFF_NO_QUEUE;
> > +               dev->tx_queue_len = 1;
> > +       }
> > 
> >        dev->num_tx_queues = txqs;
> >        dev->real_num_tx_queues = txqs;
> > 
> > Unless there is concern, I will formally submit this later.
> 
> Works for me!
> 
> Tested-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>

Cool, thanks for testing!

Cheers, Phil