| Message ID | 20191122224126.24847-2-saeedm@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | mlxfw: Improve error reporting | expand |
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@mellanox.com> > Reviewed-by: Jiri Pirko <jiri@mellanox.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;
On 11/23/19 5:56 PM, Jakub Kicinski wrote: >> 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 This makes extack use a lot of stack. There are a number of places that are already emitting warnings about high stack usage.
On 11/23/19 6:10 PM, David Ahern wrote: > On 11/23/19 5:56 PM, Jakub Kicinski wrote: >>> 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 > > This makes extack use a lot of stack. There are a number of places that > are already emitting warnings about high stack usage. > It also adds unnecessary overhead since initialization 0's out the entire buffer (most sites declare with {}; a few use memset).
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
On Mon, 2019-11-25 at 10:48 +0100, Johannes Berg wrote: > 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? > I thought about all of these alternatives before i wrote this patch, the idea here is to keep it simple and not introduce any state bits or multiple buffers to choose from, also it is hard to guarantee that we are going to hit the line of code that is going to free the allocated extack message. only the driver knows if it want a formattable message and it should allocate it, and only the netlink core should free it, I personally don't like such asymmetries in the kernel code, without a proper contract or infrastructure that will guarantee correctness and no memleaks. > > 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. > Yes, most of the use cases don't really require a formattable message. I only did it after some internal code reviews that pointed out that doing this will make my code more easy on the eye, since i could wrap everything in one function that would dump dmesg and extack messages in one shot. I will drop this patch for now. Thanks, Saeed.
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) + +#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;