netdev - Re: [PATCH iproute2 -next] ingress, clsact: don't add TCA

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: <573B1852.4000703@iogearbox.net>
Date:	Tue, 17 May 2016 15:10:42 +0200
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Jamal Hadi Salim <jhs@...atatu.com>, stephen@...workplumber.org
CC:	tgraf@...g.ch, netdev@...r.kernel.org
Subject: Re: [PATCH iproute2 -next] ingress, clsact: don't add TCA_OPTIONS
 to nl msg

On 05/17/2016 01:54 PM, Jamal Hadi Salim wrote:
> On 16-05-15 12:36 PM, Daniel Borkmann wrote:
>> In ingress and clsact qdisc TCA_OPTIONS are ignored, since it's
>> parameterless. In tc, we add an empty addattr_l(... TCA_OPTIONS,
>> NULL, 0) to the netlink message nevertheless. This has the
>> side effect that when someone tries a 'tc qdisc replace' and
>> already an existing such qdisc is present, tc fails with
>> EINVAL here.
>>
>> Reason is that in the kernel, this invokes qdisc_change() when
>> such requested qdisc is already present. When TCA_OPTIONS are
>> passed to modify parameters, it looks whether qdisc implements
>> .change() callback, and if not present (like in both cases here)
>> it returns with error. Rather than adding an empty stub to the
>> kernel that ignores TCA_OPTIONS again, just don't add TCA_OPTIONS
>> to the netlink message in the first place.
>>
>> Before:
>>
>>    # tc qdisc replace dev foo clsact    # first try
>>    # tc qdisc replace dev foo clsact    # second one
>>    RTNETLINK answers: Invalid argument
>>
>> After:
>>
>>    # tc qdisc replace dev foo clsact
>>    # tc qdisc replace dev foo clsact
>>    # tc qdisc replace dev foo clsact
>>
>> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
>
> Acked-by: Jamal Hadi Salim <jhs@...atatu.com>
>
> I see need for correctness but was curious of use case
> that made you even look at this ;->

Sure, wanted to use this in a script that is called when some event
arrives and the error 'Invalid argument' looked quite weird to me
(... would have understood -ENOTSUPP ;)), which then led me looking
into whether clsact code is correct or not, but it eventually turned
out that the api code throws that error when TCA_OPTIONS are present,
but no .change() callback, so motivation was to make sure clsact is
fine and to improve usability there for the replace command.

Thanks,
Daniel