netdev - Re: [PATCH net-next v2 0/4] Implement classifier-action terse dump mode

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: <vbfo8qkb8ip.fsf@mellanox.com>
Date:   Tue, 19 May 2020 12:04:30 +0300
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Edward Cree <ecree@...arflare.com>
Cc:     Vlad Buslov <vladbu@...lanox.com>, netdev@...r.kernel.org,
        davem@...emloft.net, jhs@...atatu.com, xiyou.wangcong@...il.com,
        jiri@...nulli.us, dcaratti@...hat.com, marcelo.leitner@...il.com,
        kuba@...nel.org
Subject: Re: [PATCH net-next v2 0/4] Implement classifier-action terse dump mode


On Mon 18 May 2020 at 18:37, Edward Cree <ecree@...arflare.com> wrote:
> On 15/05/2020 12:40, Vlad Buslov wrote:
>> In order to
>> significantly improve filter dump rate this patch sets implement new
>> mode of TC filter dump operation named "terse dump" mode. In this mode
>> only parameters necessary to identify the filter (handle, action cookie,
>> etc.) and data that can change during filter lifecycle (filter flags,
>> action stats, etc.) are preserved in dump output while everything else
>> is omitted.
> I realise I'm a bit late, but isn't this the kind of policy that shouldn't
>  be hard-coded in the kernel?  I.e. if next year it turns out that some
>  user needs one parameter that's been omitted here, but not the whole dump,
>  are they going to want to add another mode to the uapi?

Why not just extend terse dump? I won't break user land unless you are
removing something from it.

> Should this not instead have been done as a set of flags to specify which
>  pieces of information the caller wanted in the dump, rather than a mode
>  flag selecting a pre-defined set?
>
> -ed

I considered that approach initially but decided against it for
following reasons:

- Generic data is covered by current terse dump implementation.
  Everything else will be act or cls specific which would result long
  list of flag values like: TCA_DUMP_FLOWER_KEY_ETH_DST,
  TCA_DUMP_FLOWER_KEY_ETH_DST, TCA_DUMP_FLOWER_KEY_VLAN_ID, ...,
  TCA_DUMP_TUNNEL_KEY_ENC_KEY_ID, TCA_DUMP_TUNNEL_KEY_ENC_TOS. All of
  these would require a lot of dedicated logic in act and cls dump
  callbacks. Also, it would be quite a challenge to test all possible
  combinations.

- It is hard to come up with proper validation for such implementation.
  In case of terse dump I just return an error if classifier doesn't
  implement the callback (and since current implementation only outputs
  generic action info, it doesn't even require support from
  action-specific dump callbacks). But, for example, how do we validate
  a case where user sets some flower and tunnel_key act dump flags from
  previous paragraph, but Qdisc contains some other classifier? Or
  flower classifier points to other types of actions? Or when flower
  classifier has and tunnel_key actions but also mirred? Should the
  implementation return an error on encountering any classifier or
  action that doesn't have any flags set for its type or just print all
  data like regular dump? What if user asks to dump some specific option
  that wasn't set for particular filter of action instance?

Overall, the more I think about such implementation the more it looks
like a mess to me.