netdev - Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX

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]

Date:   Wed, 19 Apr 2017 12:07:48 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        eric.dumazet@...il.com, xiyou.wangcong@...il.com
Subject: Re: [PATCH net-next v4 1/2] net sched actions: dump more than
 TCA_ACT_MAX_PRIO actions per batch

On 17-04-19 11:54 AM, Jiri Pirko wrote:
> Wed, Apr 19, 2017 at 05:37:15PM CEST, jhs@...atatu.com wrote:
>> On 17-04-19 09:13 AM, Jiri Pirko wrote:
>>> Wed, Apr 19, 2017 at 03:03:59PM CEST, jhs@...atatu.com wrote:
>>>> On 17-04-19 08:36 AM, Jiri Pirko wrote:
>>>>> Wed, Apr 19, 2017 at 01:57:29PM CEST, jhs@...atatu.com wrote:
>>>>>> From: Jamal Hadi Salim <jhs@...atatu.com>
>>>>
>>>>>> include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
>>>>>> net/sched/act_api.c            | 43 ++++++++++++++++++++++++++++++++----------
>>>>>> 3 files changed, 53 insertions(+), 12 deletions(-)
>>
>>>>>> +#define TCAA_MAX (__TCAA_MAX - 1)
>>>>>> #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>>>>>> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>>>>>> -#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
>>>>>> -#define TCAA_MAX 1
>>>>>> +#define TCA_ACT_TAB TCAA_ACT_TAB
>>>>>
>>>>> This is mess. What does "TCAA" stand for?
>>>>
>>>> TC Actions Attributes.  What would you call it? I could have
>>>> called it TCA_ROOT etc. But maybe a comment to just call it
>>>> TC Actions Attributes would be enough?
>>>
>>> TCA_DUMP_X
>>>
>>> it is only for dumping. Naming it "attribute" seems weird. Same as if
>>> you have: int variable_something;
>>>
>>
>> Jiri, this is not just for dumping. We are describing high level
>> attributes for tc actions.
>
> This is already present:
> enum {
>         TCA_ACT_UNSPEC,
>         TCA_ACT_KIND,
>         TCA_ACT_OPTIONS,
>         TCA_ACT_INDEX,
>         TCA_ACT_STATS,
>         TCA_ACT_PAD,
>         TCA_ACT_COOKIE,
>         __TCA_ACT_MAX
> };
>
> This is nested inside the dump message (TCAA_MAX, TCA_ACT_TAB)
>
> Looks like you are mixing these 2.
>

No - That space is per action. The space i am defining is
above that in the the hierarchy. There used to be only
one attribute there (TCA_ACT_TAB) and now we are making
it more generic.

>
>

>> It is a _lot_ of code to change! Note:
>> This is all the UAPI visible code (the same coding style for 20 years).
>> I am worried about this part.
>
> We'll see. Lets do it in a sensitive way, in steps. But for new things,
> I think it is good not to stick with old and outlived habits.
>
>

I know you have the muscle to get it done - so fine, i will start
with this one change.

>
> Netlink is TLV, should be used as TLV. I don't see how you can run out
> any space. You tend to use Netlink in some weird hybrid mode, with only
> argument being space. I think that couple of bytes wasted is not
> a problem at all...
>

You are not making sense to me still.
What you describe as "a couple of bytes" adds up when you have
a large number of objects. I am trying to cut down on data back
and forth from user/kernel and a bitmap is a well understood entity.

Even if i did use a TLV - when i am representing flags which require one
bit each - it makes sense to use a bitmask encapsulated in a TLV. Not
to waste a TLV per bit.

cheers,
jamal