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:   Mon, 25 Nov 2019 10:48:05 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        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>
Subject: Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a
 formattable buffer

Hi Jakub,

On Sat, 2019-11-23 at 16:56 -0800, Jakub Kicinski wrote:
> 
> > -/* 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.

Regarding your other email also - this here was one stated purpose -
what I had also (maybe even more) in mind back then was that we'd be
able to elide the strings entirely for really small embedded builds. If
it's not used interactively, there isn't that much value in the strings
after all.

> > - * 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

That does seem quite excessive, indeed.

I think _if_ we want this at all then I'd say that we should move this
out-of-line, to have a helper function that will kasprintf() if
necessary, and use a bit somewhere to indicate "has been allocated" so
it can be freed later?

However, this will need some kind of "release extack" API for those
places that declare their own struct on the stack, and would need to be
reentrant (in the sense that old error messages may be overwritten, and
must be freed at that point)...

Maybe an alternative would be to have a "can sprintf" flag, and then
provide a buffer in another pointer? The caller can then decide whether
this should be permitted, e.g. netlink_rcv_skb() could provide it, but
other places maybe don't need to?

Here in the patchset though, I basically found three cases using this
capability:

+	NL_SET_ERR_MSG_MOD(extack, MLXFW_ERR_PRFX "%s",
+			   mlxfw_fsm_state_err_str[fsm_state_err]);

This one seems somewhat unnecessary - it just takes a fixed string and
adds a prefix, that may be easier this way but it wouldn't be *that*
hard to "explode" that into a bunch of NL_SET_ERR_MSG_MOD() statements I
guess.

+		NL_SET_ERR_MSG_MOD(extack, "FSM state query failed, err (%d)",
+				   err);

This (and other similar instances) is pretty useless, the error number
is almost certainly returned to userspace anyway?

+		NL_SET_ERR_MSG_MOD(extack,
+				   "FSM component query failed, comp_name(%s) err (%d)",
+				   comp_name, err);

This (and similar) one seems like the only one that's reasonable. Note
that "comp_name" is actually just a number though.

johannes