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: <CAM_iQpXqLdAJOcwyQ=DZs5zi=zEtr97_LT9uhPtTTPke=8Vvdw@mail.gmail.com>
Date:   Tue, 19 May 2020 11:58:33 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     Edward Cree <ecree@...arflare.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Davide Caratti <dcaratti@...hat.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next v2 0/4] Implement classifier-action terse dump mode

On Tue, May 19, 2020 at 2:04 AM Vlad Buslov <vladbu@...lanox.com> wrote:
> 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.

Well, if you consider netlink dump as a database query, what Edward
proposed is merely "select COLUMN1 COLUMN2 from cls_db" rather
than "select * from cls_db".

No one said it is easy to implement, it is just more elegant than you
select a hardcoded set of columns for the user.

Think about it, what if another user wants a less terse dump but still
not a full dump? Would you implement ops->terse_dump2()? Or
what if people still think your terse dump is not as terse as she wants?
ops->mini_dump()? How many ops's we would end having?


>
> - 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

Each action should be able to dump selectively too. If you think it
as a database, it is just a different table with different schemas.


>   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?

Undefined or error.

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

This is what I think about your current implementation. You know once
we add this we can't remove it any longer, right? This is why we should
make it right and better in the first place, not after carrying it for even one
release.

Thanks.