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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20171206095532.6acc99ce@shemminger-XPS-13-9360>
Date:   Wed, 6 Dec 2017 09:55:32 -0800
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Michal Privoznik <mprivozn@...hat.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
        jiri@...lanox.com
Subject: Re: [PATCH iproute2] tc: police: fix control action parsing

On Wed, 29 Nov 2017 10:06:26 +0100
Michal Privoznik <mprivozn@...hat.com> wrote:

> 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

Michal, if you and Jiri can work out cleaner parse action semantic that would
be best. If not, then this patch would do.  Let me know next week what you figure out.