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: <20221110092709.06859da9@kernel.org>
Date:   Thu, 10 Nov 2022 09:27:09 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Hangbin Liu <liuhangbin@...il.com>,
        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>,
        Paolo Abeni <pabeni@...hat.com>,
        David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

On Thu, 10 Nov 2022 09:27:40 -0500 Jamal Hadi Salim wrote:
> > "Global", but they necessitate complicating the entire protocol
> > to use directly.
> >
> > Unless we want to create a separate netlink multicast channel for
> > just ext acks of a family. That's fine by me, I guess. I'm mostly
> > objecting to pretending notifications are multi-msg just to reuse
> > NLMSG_DONE, and forcing all notification listeners to deal with it.
> 
> TBH, I am struggling as well. NLMSG_DONE is really for multi-message
> (with kernel state) like dumps. Could we just extend nlmsg_notify()
> callers to take extack and pass it through and then have  nlmsg_notify()
> do the NLM_F_ACK_TLVS dance without MULTI flag? It would have to
> be backward compat and require user space changes which Hangbin's
> patch avoids but will be more general.

I think we'd need some sort of "internal / netlink level attributes"
to do that. We have only one attribute "space" inside any message,
defined by the family itself. So attribute type 1 for a TCA notification
is TCA_KIND, not NLMSGERR_ATTR_MSG.

We'd need changes to struct nlmsghdr to allow the nlmsghdr to have its
own set of attributes. Would be cool, but major surgery at this point.
I guess we could assume the families don't use high attr types, and say
that attr types > 0x400 are reserved for netlink. Put NLMSG_ATTRs there.
Seems risky, tho.

> > The more time we spend discussing this the more I'm inclined to say
> > "this is a typical tracing use case, just use the tracepoint" :(  
> 
> I understand your frustration but from an operational pov it is
> better to deal with one tool than two (Marcelo's point).

IDK, we can have a kernel hook into the trace point and generate 
the output over netlink, like we do with drop monitor and skb_free().
But I really doubt that its worth it. Also you can put a USDT into OvS
if you don't want to restart it. There are many options, not everything
is a nail :S

> The way i look at these uapi discussions is it is ok to discuss the
> color of the bike shed(within reason) because any decisions made here
> will have a long term effect.

To stretch the analogy - in my mind we have way too many one-off, 
odd looking bike sheds and not enough bikes (users) with netlink.

So anything that reads to me like "ooh, look at this neat trick 
I can do with netlink that I can totally hand parse in iproute2" 
rises the hair on my back :(