netdev - Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

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

Date:   Mon, 19 Oct 2020 18:18:20 +0300
From:   Vlad Buslov <vlad@...lov.dev>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Vlad Buslov <vladbu@...dia.com>, dsahern@...il.com,
        stephen@...workplumber.org, netdev@...r.kernel.org,
        davem@...emloft.net, xiyou.wangcong@...il.com, jiri@...nulli.us,
        ivecera@...hat.com, Vlad Buslov <vladbu@...lanox.com>
Subject: Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump


On Mon 19 Oct 2020 at 16:48, Jamal Hadi Salim <jhs@...atatu.com> wrote:
> On 2020-10-18 8:16 a.m., Vlad Buslov wrote:
>> On Sat 17 Oct 2020 at 14:20, Jamal Hadi Salim <jhs@...atatu.com> wrote:
>>> On 2020-10-16 12:42 p.m., Vlad Buslov wrote:
>>>
>
>>> Either one sounds appealing - the refactoring feels simpler
>>> as opposed to a->terse_print().
>>
>> With such refactoring we action type will be printed before some basic
>> validation which can lead to outputting the type together with error
>> message. Consider tunnel key action output callback as example:
>>
>> static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
>> {
>> 	struct rtattr *tb[TCA_TUNNEL_KEY_MAX + 1];
>> 	struct tc_tunnel_key *parm;
>>
>> 	if (!arg)
>> 		return 0;
>>
>> 	parse_rtattr_nested(tb, TCA_TUNNEL_KEY_MAX, arg);
>>
>> 	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
>> 		fprintf(stderr, "Missing tunnel_key parameters\n");
>> 		return -1;
>> 	}
>> 	parm = RTA_DATA(tb[TCA_TUNNEL_KEY_PARMS]);
>>
>> 	print_string(PRINT_ANY, "kind", "%s ", "tunnel_key");
>>
>> If print "kind" call is moved before checking the arg it will always be
>> printed, even when immediately followed by "Missing tunnel_key
>> parameters\n" string. Is this a concern?
>>
>
> That could be a good thing, no? you get to see the action name with the
> error. Its really not a big deal if you decide to do a->terse_print()
> instead.

Maybe. Just saying that this change would also change user-visible
iproute2 behavior.

>
>>>
>>> BTW: the action index, unless i missed something, is not transported
>>> from the kernel for terse option. It is an important parameter
>>> when actions are shared by filters (since they will have the same
>>> index).
>>> Am i missing something?
>>
>> Yes, tc_action_ops->dump(), which outputs action index among other data,
>> is not called at all by terse dump.
>
> I am suggesting it is an important detail that is currently missing.
> Alternatively since you have the cookies in there - it is feasible that
> someone who creates the action could "encode" the index in the cookie.
> But that makes it a "proprietary" choice of whoever is creating
> the filter/action.

It is not a trivial change. To get this data we need to call
tc_action_ops->dump() which puts bunch of other unrelated info in
TCA_OPTIONS nested attr. This hurts both dump size and runtime
performance. Even if we add another argument to dump "terse dump, print
only index", index is still part of larger options structure which
includes at least following fields:

#define tc_gen \
	__u32                 index; \
	__u32                 capab; \
	int                   action; \
	int                   refcnt; \
	int                   bindcnt

This wouldn't be much of a terse dump anymore. What prevents user that
needs all action info from calling regular dump? It is not like terse
dump substitutes it or somehow makes it harder to use.

>
> cheers,
> jamal