netdev - RE: [PATCH net] net: enetc: correct the indexes of highest and 2nd highest TCs

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] [day] [month] [year] [list]

Message-ID:
 <DB6PR04MB31419A2E70C34BC974EA7F7D8853A@DB6PR04MB3141.eurprd04.prod.outlook.com>
Date: Wed, 7 Jun 2023 02:02:41 +0000
From: Wei Fang <wei.fang@....com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Claudiu Manoil <claudiu.manoil@....com>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net] net: enetc: correct the indexes of highest and 2nd
 highest TCs

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@....com>
> Sent: 2023年6月6日 17:17
> To: Wei Fang <wei.fang@....com>
> Cc: Claudiu Manoil <claudiu.manoil@....com>; davem@...emloft.net;
> edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH net] net: enetc: correct the indexes of highest and 2nd
> highest TCs
> 
> On Tue, Jun 06, 2023 at 04:46:18PM +0800, wei.fang@....com wrote:
> > From: Wei Fang <wei.fang@....com>
> >
> > For ENETC hardware, the TCs are numbered from 0 to N-1, where N
> > is the number of TCs. Numerically higher TC has higher priority.
> > It's obvious that the highest priority TC index should be N-1 and
> > the 2nd highest priority TC index should be N-2.
> > However, the previous logic uses netdev_get_prio_tc_map() to get
> > the indexes of highest priority and 2nd highest priority TCs, it
> > does not make sense and is incorrect. It may get wrong indexes of
> > the two TCs and make the CBS unconfigurable. e.g.
> 
> Well, you need to consider that prior to commit 1a353111b6d4 ("net:
> enetc: act upon the requested mqprio queue configuration"), the driver
> would always set up an identity mapping between priorities, traffic
> classes, rings and netdev queues.
> 
I also considered the situation prior to the commit 1a353111b6d4. The
problem also existed.
e.g.
$ tc qdisc add dev eno0 root handle 1: mqprio num_tc 8 \
	map 0 1 2 3 6 7 4 5 queues 1@0 1@1 \
			  ^ ^ ^ ^
	1@2 1@3 1@4 1@5 1@6 1@7 hw 1
The driver would deem the indexes of the two highest TCs are 5 and 4,
rather than 7 and 6.

> So, yes, giving a "tc" argument to netdev_get_prio_tc_map() is
> semantically incorrect, but it only started being a problem when the
> identity mapping started being configurable.
> 
In my opinion, "unconfigurable" is also a problem.

> > $ tc qdisc add dev eno0 parent root handle 100: mqprio num_tc 6 \
> > 	map 0 0 1 1 2 3 4 5 queues 1@0 1@1 1@2 1@3 2@4 2@6 hw 1
> > $ tc qdisc replace dev eno0 parent 100:6 cbs idleslope 100000 \
> > 	sendslope -900000 hicredit 12 locredit -113 offload 1
> > $ Error: Specified device failed to setup cbs hardware offload.
> >   ^^^^^
> 
> ok.
> 
> >
> > Fixes: c431047c4efe ("enetc: add support Credit Based Shaper(CBS) for
> hardware offload")
> 
> In principle, there shouldn't be an issue with backporting the fix that
> far (v5.5), even if it is unnecessary beyond commit 1a353111b6d4 (v6.3).
> If you want to respin the patch to clarify the situation, fine. If not,
> also fine.
> 
> > Signed-off-by: Wei Fang <wei.fang@....com>
> > ---
> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@....com>