netdev - Re: [PATCH (repost) net-next] sched: add extack for tfilter

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: <Y1kEtovIpgclICO3@Laptop-X1>
Date:   Wed, 26 Oct 2022 17:58:14 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

On Sun, Oct 02, 2022 at 11:27:08AM -0400, Jamal Hadi Salim wrote:
> > But, precisely. In the example Hangbin gave, it is showing why the
> > entry is not_in_hw. That's still data that belongs to the event that
> > happened and that can't be queried afterwards even if the user/app
> > monitoring it want to. Had it failed entirely, I agree, as the control
> > path never changed.
> >
> > tc monitor is easier to use than perf probes in some systems. It's not
> > uncommon to have tc installed but not perf. It's also easier to ask a
> > customer to run it than explain how to enable the tracepoint and print
> > ftrace buffer via /sys files, and the output is more meaningful for us
> > as well: we know exactly which filter triggered the message. The only
> > other place that we can correlate the filter and the warning, is on
> > vswitchd log. Which is not easy to read either.
> 
> To Jakub's point: I think one of those NLMSGERR TLVs is the right place
> and while traces look attractive I see the value of having a unified
> collection point via the tc monitor.

Hi Jamal,

Sorry for the late response. I just came back form vacation. For this issue,
I saw netlink_dump_done() also put NLMSGERR_ATTR_MSG in NLMSG_DONE.
So why can't we do the same here?

In https://www.kernel.org/doc/html//next/userspace-api/netlink/intro.html,
The "optionally extended ACK" in NLMSG_DONE is OK.

> Since you cant really batch events - it seems the NLMSG_DONE/MULTI
> hack is done just to please iproute2::tc?

Yes.

> IMO:
> I think if you need to do this, then you have to teach iproute2
> new ways of interpreting the message (which is nice because you
> dont have to worry about backward compat). Some of that code
> should be centralized and reused by netlink generically
> instead of just cls_api, example the whole NLM_F_ACK_TLVS dance.

Would you please help explain more about this?

> 
> Also - i guess it will depend on the underlying driver?
> This seems very related to a specific driver:
> "Warning: mlx5_core: matching on ct_state +new isn't supported."
> Debuggability is always great but so is backwards compat.
> What happens when you run old userspace tc? There are tons
> of punting systems that process these events out there and
> depend on the current event messages as is.

I think old tc should just ignore this NLMSGERR_ATTR_MSG?

Thanks
Hangbin