netdev - Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than one message

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: <ef243654-c428-b645-7264-6134909744cf@gmail.com>
Date:   Sun, 18 Mar 2018 10:11:20 -0600
From:   David Ahern <dsahern@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        netdev@...r.kernel.org
Cc:     Alexander Aring <aring@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kubakici@...pl>
Subject: Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than
 one message

On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote:
> Currently extack can carry only a single message, which is usually the
> error message.
> 
> This imposes a limitation on a more verbose error reporting. For
> example, it's not able to carry warning messages together with the error
> message, or 2 warning messages.


The only means for userspace to separate an error message from info or
warnings is the error in nlmsgerr. If it is non-0, any extack message is
considered an error else it is a warning.


> 
> One use case is when dealing with tc offloading. If it failed to
> offload, and also failed to install on software, it will report only
> regarding the error about the software datapath, but the error from the
> hardware path would also have been welcomed.
> 
> This patch extends extack so it now can carry up to 8 messages and these
> messages may be prefixed similarly to printk/pr_warning, so thus they
> can be tagged either was warning or error.
> 
> Fixed number of messages because supporting a dynamic limit seem to be
> an overkill for the moment. Remember that this is not meant to be a
> trace tool, but an error reporting one.
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> ---
>  include/linux/netlink.h  | 50 +++++++++++++++++++++++++++++-------------------
>  net/netlink/af_netlink.c | 12 +++++++-----
>  2 files changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index f3075d6c7e8229c999ab650537f1e3b11e1f457b..d9780836cf263d4c436d732e9b7a8cde0739ac23 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -71,43 +71,53 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>   * @cookie: cookie data to return to userspace (for success)
>   * @cookie_len: actual cookie data length
>   */
> +#define NETLINK_MAX_EXTACK_MSGS 8

8 is way too many. If some change fails because of an error, why would a
single error message not be enough? If it is a not an error, why is more
than 1 warning message not enough? (I forget the details of the tc
'skip_sw' use case)


>  struct netlink_ext_ack {
> -	const char *_msg;
> +	const char *_msg[NETLINK_MAX_EXTACK_MSGS];
>  	const struct nlattr *bad_attr;
>  	u8 cookie[NETLINK_MAX_COOKIE_LEN];
>  	u8 cookie_len;
> +	u8 _msg_count;
>  };
>  
> -/* Always use this macro, this allows later putting the
> - * message into a separate section or such for things
> - * like translation or listing all possible messages.
> - * Currently string formatting is not supported (due
> - * to the lack of an output buffer.)
> +/* Always use these macros, this allows later putting
> + * the message into a separate section or such for
> + * things like translation or listing all possible
> + * messages.  Currently string formatting is not
> + * supported (due to the lack of an output buffer.)
>   */
> -#define NL_SET_ERR_MSG(extack, msg) do {		\
> -	static const char __msg[] = msg;		\
> -	struct netlink_ext_ack *__extack = (extack);	\
> -							\
> -	if (__extack)					\
> -		__extack->_msg = __msg;			\
> +#define NL_SET_MSG(extack, msg) do {					\
> +	static const char __msg[] = msg;				\
> +	struct netlink_ext_ack *__extack = (extack);			\
> +									\
> +	if (__extack &&							\
> +	    !WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS))	\
> +		__extack->_msg[__extack->_msg_count++] = __msg;		\
>  } while (0)
>  
> +#define NL_SET_ERR_MSG(extack, msg)	NL_SET_MSG(extack, msg)
> +#define NL_SET_WARN_MSG(extack, msg)	NL_SET_MSG(extack, KERN_WARNING msg)
> +
>  #define NL_SET_ERR_MSG_MOD(extack, msg)			\
>  	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> +#define NL_SET_WARN_MSG_MOD(extack, msg)		\
> +	NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg)
> +

Adding separate macros for error versus warning is confusing since from
an extack perspective a message is a message and there is no uapi to
separate them.