netdev - Re: [PATCH iproute2-next] tc: mqprio: reject queues count/offset pair count higher than num

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: <08dff834-05e7-6079-4895-907a35a2a1a6@gmail.com>
Date:   Mon, 18 May 2020 09:12:44 -0600
From:   David Ahern <dsahern@...il.com>
To:     Stephen Hemminger <stephen@...workplumber.org>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc:     netdev@...r.kernel.org, kiran.patil@...el.com
Subject: Re: [PATCH iproute2-next] tc: mqprio: reject queues count/offset pair
 count higher than num_tc

On 5/18/20 9:08 AM, Stephen Hemminger wrote:
> On Wed, 13 May 2020 21:47:17 +0200
> Maciej Fijalkowski <maciej.fijalkowski@...el.com> wrote:
> 
>> Provide a sanity check that will make sure whether queues count/offset
>> pair count will not exceed the actual number of TCs being created.
>>
>> Example command that is invalid because there are 4 count/offset pairs
>> whereas num_tc is only 2.
>>
>>  # tc qdisc add dev enp96s0f0 root mqprio num_tc 2 map 0 0 0 0 1 1 1 1
>> queues 4@0 4@4 4@8 4@12 hw 1 mode channel
>>
>> Store the parsed count/offset pair count onto a dedicated variable that
>> will be compared against opt.num_tc after all of the command line
>> arguments were parsed. Bail out if this count is higher than opt.num_tc
>> and let user know about it.
>>
>> Drivers were swallowing such commands as they were iterating over
>> count/offset pairs where num_tc was used as a delimiter, so this is not
>> a big deal, but better catch such misconfiguration at the command line
>> argument parsing level.
>>
>> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
>> ---
>>  tc/q_mqprio.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/tc/q_mqprio.c b/tc/q_mqprio.c
>> index 0eb41308..f26ba8d7 100644
>> --- a/tc/q_mqprio.c
>> +++ b/tc/q_mqprio.c
>> @@ -48,6 +48,7 @@ static int mqprio_parse_opt(struct qdisc_util *qu, int argc,
>>  	__u64 max_rate64[TC_QOPT_MAX_QUEUE] = {0};
>>  	__u16 shaper = TC_MQPRIO_SHAPER_DCB;
>>  	__u16 mode = TC_MQPRIO_MODE_DCB;
>> +	int cnt_off_pairs = 0;
> 
> Since opt.num_tc is u8, shouldn't this be u8 as well?
> Note: maximum number of queue is TC_QOPT_MAX_QUEUE (16).
> 

I was looking that as having cnt_off_pairs as an int makes for simpler
code - not having to detect rollover. But if there is a limit, I guess
that takes precedence.

And if there is a limit, perhaps that should be checked on num_tc parsing.