netdev - Re: [PATCH iproute2] tc: police: fix control action parsing

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: <16bd923d-f725-e455-f8c3-43708b233564@redhat.com>
Date:   Wed, 29 Nov 2017 10:06:26 +0100
From:   Michal Privoznik <mprivozn@...hat.com>
To:     Jiri Pirko <jiri@...nulli.us>,
        Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org, jiri@...lanox.com
Subject: Re: [PATCH iproute2] tc: police: fix control action parsing

On 11/28/2017 02:02 PM, Jiri Pirko wrote:
> Mon, Nov 27, 2017 at 09:32:59PM CET, stephen@...workplumber.org wrote:
>> On Mon, 27 Nov 2017 19:00:14 +0100
>> Michal Privoznik <mprivozn@...hat.com> wrote:
>>
>>> parse_action_control helper does advancing of the arg inside. So don't
>>> do it outside.
>>>
>>> Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
>>> Signed-off-by: Michal Privoznik <mprivozn@...hat.com>
>>
>> The helpers are not helping here.
>> Adding another layer of indirection on moving argc/argv then causing caller
>> to have to keep track is bad design.
>>
>> Also since pars_action_control_slash is only used by police, why was it
>> moved into tc_util in the first place? I would prefer just to rip out that
>> bit and put it back in policer.
> 
> I tried to make all the x-specific parsing to go away and make all done
> in core. That should have been done from the very beginning, we would
> lot of mess.
> 

Okay, would it be a better solution if __parse_action_control() wouldn't
call NEXT_ARG_FWD() at the end? Then this patch wouldn't be needed and
every place that calls parse_action_control() would not need to special
case it.

Just a bit of background:
Libvirt has a capability of setting QoS on bridges/TAPs it manages. And
as part of that it issues the following command to set traffic limiting:

  tc filter add dev virbr0 parent ffff: protocol all u32 match u32 0 0 \
  police rate 50kbps burst 50kb mtu 64kb drop flowid :1

More info can be found in this bug:

  https://bugzilla.redhat.com/show_bug.cgi?id=1514963

Fortunately, no a lot of distros ship 4.13+ so libvirt ain't broken yet.
But soon.

Michal