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 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:   Fri, 30 Sep 2022 19:03:31 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Hangbin Liu <liuhangbin@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>
Cc:     netdev@...r.kernel.org, Cong Wang <xiyou.wangcong@...il.com>,
        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>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH (repost) net-next] sched: add extack for tfilter_notify

On Thu, 29 Sep 2022 11:35:05 +0800 Hangbin Liu wrote:
> In commit 81c7288b170a ("sched: cls: enable verbose logging") Marcelo
> made cls could log verbose info for offloading failures, which helps
> improving Open vSwitch debuggability when using flower offloading.
> 
> It would also be helpful if "tc monitor" could log this message, as it
> doesn't require vswitchd log level adjusment. Let's add the extack message
> in tfilter_notify so the monitor program could receive the failures.
> e.g.

The title read as "just another extack addition" but this is much 
more than that :S 

Jamal, you may want to take a look.

>   # tc monitor
>   added chain dev enp3s0f1np1 parent ffff: chain 0
>   added filter dev enp3s0f1np1 ingress protocol all pref 49152 flower chain 0 handle 0x1
>     ct_state +trk+new
>     not_in_hw
>           action order 1: gact action drop
>            random type none pass val 0
>            index 1 ref 1 bind 1
> 
>   Warning: mlx5_core: matching on ct_state +new isn't supported.
> 
> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
> 
> Rebase the patch to latest net-next as the previous could not
> apply to net-next.

> +	nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm),
> +			(extack && extack->_msg) ? flags | NLM_F_MULTI : flags);

> +
> +	if (extack && extack->_msg) {
> +		nlh = nlmsg_put(skb, portid, seq, NLMSG_DONE, 0, flags | NLM_F_ACK_TLVS);
> +		if (!nlh)
> +			goto out_nlmsg_trim;
> +
> +		if (nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
> +			goto nla_put_failure;
> +
> +		nlmsg_end(skb, nlh);
> +	}
> +

So you're adding a fake* _F_MULTI on the notification just so you
can queue a NLMSG_DONE after and not break the "NLMSG_DONE terminates 
a _F_MUTLI" sequence rule?

* fake as in there's only one message, there's no multi-ness here.

I don't think _F_MULTI should be treated lightly and I don't think
NLMSG_DONE as part of notification sequences is a good idea either :(

(1) does the tracepoint not give you want you need?
    (netlink:netlink_extack), failing that -
(2) why not wrap the extack msg in an attribute