netdev - Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer

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]

Date:   Sat, 23 Nov 2019 16:56:55 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Saeed Mahameed <saeedm@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        David Ahern <dsahern@...il.com>,
        Johannes Berg <johannes@...solutions.net>
Subject: Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a
 formattable buffer

On Fri, 22 Nov 2019 22:41:47 +0000, Saeed Mahameed wrote:
> Before this patch the extack msg had to be a const char and didn't leave
> much room for developers to report formattable and flexible messages.
> 
> All current usages are oneliner messages, hence, a buffer of size 128B
> should be sufficient to replace the const char pointer.
> 
> This will allow future usages to provide formattable messages and more
> flexible error reporting, without any impact on current users.
> 
> Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> Reviewed-by: Jiri Pirko <jiri@...lanox.com>

Let's CC DSA and Johannes on this one

> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 205fa7b1f07a..de9004da0288 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -62,6 +62,7 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>  
>  /* this can be increased when necessary - don't expose to userland */
>  #define NETLINK_MAX_COOKIE_LEN	20
> +#define NL_EXTACK_MAX_MSG_SZ	128
>  
>  /**
>   * struct netlink_ext_ack - netlink extended ACK report struct
> @@ -72,40 +73,36 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>   * @cookie_len: actual cookie data length
>   */
>  struct netlink_ext_ack {
> -	const char *_msg;
> +	char _msg[NL_EXTACK_MAX_MSG_SZ];
>  	const struct nlattr *bad_attr;
>  	u8 cookie[NETLINK_MAX_COOKIE_LEN];
>  	u8 cookie_len;
>  };
>  
> -/* 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.)
> - */
> -#define NL_SET_ERR_MSG(extack, msg) do {		\
> -	static const char __msg[] = msg;		\
> +#define NL_MSG_FMT(extack, fmt, ...) \
> +	WARN_ON(snprintf(extack->_msg, NL_EXTACK_MAX_MSG_SZ, fmt, ## __VA_ARGS__) \
> +		>= NL_EXTACK_MAX_MSG_SZ)

I'd personally appreciate a word of analysis and reassurance in the
commit message that this snprintf + WARN_ON inlined in every location
where extack is used won't bloat the kernel :S

One could easily imagine a solution which would continue to carry the
static strings via a pointer without the unnecessary roundtrip thru
snprintf().

> +#define NL_SET_ERR_MSG(extack, fmt, ...) do {		\
>  	struct netlink_ext_ack *__extack = (extack);	\
>  							\
>  	if (__extack)					\
> -		__extack->_msg = __msg;			\
> +		NL_MSG_FMT(__extack, fmt, ## __VA_ARGS__); \
>  } while (0)
>  
> -#define NL_SET_ERR_MSG_MOD(extack, msg)			\
> -	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> +#define NL_SET_ERR_MSG_MOD(extack, fmt, ...)			\
> +	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " fmt, ## __VA_ARGS__)
>  
>  #define NL_SET_BAD_ATTR(extack, attr) do {		\
>  	if ((extack))					\
>  		(extack)->bad_attr = (attr);		\
>  } while (0)
>  
> -#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do {	\
> -	static const char __msg[] = msg;		\
> +#define NL_SET_ERR_MSG_ATTR(extack, attr, fmt, ...) do {	\
>  	struct netlink_ext_ack *__extack = (extack);	\
>  							\
>  	if (__extack) {					\
> -		__extack->_msg = __msg;			\
> +		NL_MSG_FMT(__extack, fmt, ## __VA_ARGS__); \
>  		__extack->bad_attr = (attr);		\
>  	}						\
>  } while (0)
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index cace9b307781..2ce1d6b68ce8 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -208,7 +208,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
>  	case NLA_REJECT:
>  		if (extack && pt->validation_data) {
>  			NL_SET_BAD_ATTR(extack, nla);
> -			extack->_msg = pt->validation_data;
> +			NL_MSG_FMT(extack, pt->validation_data);
>  			return -EINVAL;
>  		}
>  		err = -EINVAL;