netlink: formatted extacks

include/linux/netlink.h struct netlink_ext_ack { · line 112

Quoted 6 lines

include/linux/netlink.h +5 -0

+#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
+	struct netlink_ext_ack *__extack = (extack);		\
+								\
+	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
+		  (fmt), ##args);				\

Maybe that should print some kind of warning if the string was longer than the buffer? OTOH, I guess the user would notice anyway, and until you run the code nobody can possibly notice ... too bad then?

Maybe we could at least _statically_ make sure that the *format* string (fmt) is shorter than say 60 chars or something to give some wiggle room for the print expansion?

	/* allow 20 chars for format expansion */
	BUILD_BUG_ON(strlen(fmt) > NETLINK_MAX_FMTMSG_LEN - 20);

might even work? Just as a sanity check.

include/linux/netlink.h struct netlink_ext_ack { · line 116

Quoted 4 lines

include/linux/netlink.h +4 -0

+	do_trace_netlink_extack(__extack->_msg_buf);		\
+								\
+	if (__extack)						\
+		__extack->_msg = __extack->_msg_buf;		\

That "if (__extack)" check seems a bit strange, you've long crashed with a NPD if it was really NULL?

Quoted 18 lines · Johannes Berg

include/linux/netlink.h struct netlink_ext_ack { · line 112

Quoted 6 lines

include/linux/netlink.h +5 -0

+#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
+	struct netlink_ext_ack *__extack = (extack);		\
+								\
+	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
+		  (fmt), ##args);				\

Maybe that should print some kind of warning if the string was longer than the buffer? OTOH, I guess the user would notice anyway, and until you run the code nobody can possibly notice ... too bad then?

Maybe we could at least _statically_ make sure that the *format* string (fmt) is shorter than say 60 chars or something to give some wiggle room for the print expansion?

	/* allow 20 chars for format expansion */
	BUILD_BUG_ON(strlen(fmt) > NETLINK_MAX_FMTMSG_LEN - 20);

Quoted 1 line

might even work? Just as a sanity check.

Hmm, I don't think we want to prohibit the case of (say) a 78-char format
string with one %d that's always small-valued in practice.
In fact if you have lots of % in the format string the output could be
significantly *shorter* than fmt.
So while I do like the idea of a sanity check, I don't see how to do it
without imposing unnecessary limitations.

Unless maybe we printed a warning together with the full string, so the user could recover it?

Quoted 19 lines · Edward Cree

Quoted 18 lines · Johannes Berg

include/linux/netlink.h struct netlink_ext_ack { · line 112

Quoted 6 lines

include/linux/netlink.h +5 -0

+#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
+	struct netlink_ext_ack *__extack = (extack);		\
+								\
+	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
+		  (fmt), ##args);				\

Maybe that should print some kind of warning if the string was longer than the buffer? OTOH, I guess the user would notice anyway, and until you run the code nobody can possibly notice ... too bad then?

Maybe we could at least _statically_ make sure that the *format* string (fmt) is shorter than say 60 chars or something to give some wiggle room for the print expansion?

	/* allow 20 chars for format expansion */
	BUILD_BUG_ON(strlen(fmt) > NETLINK_MAX_FMTMSG_LEN - 20);

Quoted 9 lines

Quoted 1 line

might even work? Just as a sanity check.

Hmm, I don't think we want to prohibit the case of (say) a 78-char format
string with one %d that's always small-valued in practice.
In fact if you have lots of % in the format string the output could be
significantly *shorter* than fmt.
So while I do like the idea of a sanity check, I don't see how to do it
without imposing unnecessary limitations.

Yeah, I agree. We could runtime warn but that's also pretty useless.

I guess we just have to be careful - but I know from experience that won't work ;-)

(and some things like %pM or even %p*H can expand a lot anyway)

Unless maybe we printed a warning together with the full string, so the user could recover it? WARN_ON() isn't useful though, the string should be enough to understand where it came from.

Anyway just thinking out loud :)

johannes

I think that the macro caller need to take the buffer size into account passing the formatted msg. So if the generated message would not fit into the buffer, it's a caller bug. WARN_ON() is suitable for such things, as it most probaly will hit the developer testing newly added exack message.

Quoted 2 lines

That "if (__extack)" check seems a bit strange, you've long crashed with a NPD if it was really NULL?

Good point, I blindly copied NL_SET_ERR_MSG without thinking. The check should enclose the whole body, will fix in v2.

From: Edward Cree <[email protected]>

Include an 80-byte buffer in struct netlink_ext_ack that can be used for scnprintf()ed messages. This does mean that the resulting string can't be enumerated, translated etc. in the way NL_SET_ERR_MSG() was designed to allow.

Signed-off-by Edward Cree <[email protected]>

---
include/linux/netlink.h | 21 +++++++++++++++++++--

1 file changed, 19 insertions(+), 2 deletions(-)

include/linux/netlink.h +10 -2

index d51e041d2242..bfab9dbd64fa 100644
@@ -64,6 +64,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 NETLINK_MAX_FMTMSG_LEN	80
 
 /**
 * struct netlink_ext_ack - netlink extended ACK report struct

@@ -75,6 +76,8 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
 * @miss_nest: nest missing an attribute (%NULL if missing top level attr)
 * @cookie: cookie data to return to userspace (for success)
 * @cookie_len: actual cookie data length
+ * @_msg_buf: output buffer for formatted message strings - don't access
+ *	directly, use %NL_SET_ERR_MSG_FMT
 */
 struct netlink_ext_ack {
 	const char *_msg;

@@ -84,13 +87,13 @@ struct netlink_ext_ack {
 	u16 miss_type;
 	u8 cookie[NETLINK_MAX_COOKIE_LEN];
 	u8 cookie_len;
+	char _msg_buf[NETLINK_MAX_FMTMSG_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.)
+ * If string formatting is needed use NL_SET_ERR_MSG_FMT.
 */
 #define NL_SET_ERR_MSG(extack, msg) do {		\
 	static const char __msg[] = msg;		\

@@ -102,9 +105,23 @@ struct netlink_ext_ack {
 		__extack->_msg = __msg;			\
 } while (0)
 
+#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
+	struct netlink_ext_ack *__extack = (extack);		\
+								\
+	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
+		  (fmt), ##args);				\

include/linux/netlink.h +2 -0

+	NL_SET_ERR_MSG_FMT((extack), KBUILD_MODNAME ": " fmt, ##args)
+
 #define NL_SET_BAD_ATTR_POLICY(extack, attr, pol) do {	\
 	if ((extack)) {					\
 		(extack)->bad_attr = (attr);		\

Quoted 4 lines · Fri, Oct 07, 2022 at 03:25:12PM CEST, [email protected] wrote:

#define NL_SET_ERR_MSG_MOD(extack, msg) \
NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)

+#define NL_SET_ERR_MSG_FMT_MOD(extack, fmt, args...) \

I wonder, wouldn't it be better to just have NL_SET_ERR_MSG_MOD which accepts format string and that's it. I understand there is an extra overhead for the messages that don't use formatting, but do we care? This is no fastpath and usually happens only seldom. The API towards the driver would be more simple.

I like this a lot!

Quoted 5 lines

I wonder, wouldn't it be better to just have NL_SET_ERR_MSG_MOD which accepts format string and that's it. I understand there is an extra overhead for the messages that don't use formatting, but do we care? This is no fastpath and usually happens only seldom. The API towards the driver would be more simple.

Could do, but this way a fixed string isn't limited to
NETLINK_MAX_FMTMSG_LEN like it would be if we tried to stuff it in _msg_buf. Unless you're suggesting some kind of macro magic that detects whether args is empty and chooses which implementation to use, but that seems like excessive hidden cleverness — better to have driver authors aware of the limitations of each choice.