| Message ID | 26c2cf2e699de83905e2c21491b71af0e34d00d8.1665567166.git.ecree.xilinx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netlink: formatted extacks | expand |
On Thu, 13 Oct 2022 10:23:00 +0100 edward.cree@amd.com wrote: > + if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN, \ > + (fmt), ##args) >= NETLINK_MAX_FMTMSG_LEN) \ > + net_warn_ratelimited("truncated extack: " fmt "\n", \ > + ##args); \ > + \ Some "take it or leave it" comments: - Jiri's idea of always printing may be worth exploring - my preference would also be to produce a warning on overflow, rather than the exact print, because I always worry about people starting to depend on the print. And WARN_ON() is really heavy and may trigger remediations even tho truncated extack is just a minor nuisance. I'd do: pr(extack formatting overflow $__FILE__:$__func__:$__LINE__ $needed_len) (I think splicing the "trunced extack:" with fmt will result in the format string getting stored in .ro twice?)
On Thu, 2022-10-13 at 08:29 -0700, Jakub Kicinski wrote: > > I'd do: > > pr(extack formatting overflow $__FILE__:$__func__:$__LINE__ $needed_len) > > (I think splicing the "trunced extack:" with fmt will result > in the format string getting stored in .ro twice?) > If you worry about the strings (and sizes) then you probably shouldn't advocate always having __FILE__ and __func__ ;-) FWIW, my argument earlier was that if we have the truncated string a) it lets you recover better in a live system b) the message ought to be enough to figure out where the issue is, and if the message isn't unique you probably have the problem twice too But yeah, I'm with "take it or leave it", it all doesn't matter that much. johannes
On Thu, 13 Oct 2022 18:16:47 +0200 Johannes Berg wrote: > If you worry about the strings (and sizes) then you probably shouldn't > advocate always having __FILE__ and __func__ ;-) To be clear I meant to pass those as arguments to a common format string. Are you saying just using them will bloat the binary? Oh well, I guess :(
On 13/10/2022 16:29, Jakub Kicinski wrote: > (I think splicing the "trunced extack:" with fmt will result > in the format string getting stored in .ro twice?) Yes, it will. I guess we could splice "%s" with fmt in _both_ calls (snprintf and net_warn_ratelimited), pass "" to one and "truncated extack: " to the other. Then there's only a single string to put in .ro. Is that worth the complication?
On Mon, 17 Oct 2022 13:04:35 +0100 Edward Cree wrote: > On 13/10/2022 16:29, Jakub Kicinski wrote: > > (I think splicing the "trunced extack:" with fmt will result > > in the format string getting stored in .ro twice?) > > Yes, it will. I guess we could splice "%s" with fmt in _both_ > calls (snprintf and net_warn_ratelimited), pass "" to one and > "truncated extack: " to the other. Then there's only a single > string to put in .ro. Is that worth the complication? I vote 'yes', with a simple comment next to it, it should be a fairly obvious trick to a reader of this code.
diff --git a/include/linux/netlink.h b/include/linux/netlink.h index d51e041d2242..4cbe87739c4d 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -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,27 @@ 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); \ + \ + if (!__extack) \ + break; \ + if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN, \ + (fmt), ##args) >= NETLINK_MAX_FMTMSG_LEN) \ + net_warn_ratelimited("truncated extack: " fmt "\n", \ + ##args); \ + \ + do_trace_netlink_extack(__extack->_msg_buf); \ + \ + __extack->_msg = __extack->_msg_buf; \ +} while (0) + #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...) \ + 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); \