netlink: formatted extacks
Subsystems (3)
v2 cover netlink: formatted extacks
From: Edward Cree <[email protected]>
Currently, netlink extacks can only carry fixed string messages, which
is limiting when reporting failures in complex systems. This series adds the ability to return printf-formatted messages, and uses it in the sfc driver's TC offload code.
Formatted extack messages are limited in length to a fixed buffer size,
currently 80 characters. If the message exceeds this, the full message will be logged (ratelimited) to the console and a truncated version returned over netlink.
There is no change to the netlink uAPI; only internal kernel changes
are needed.
Changed in v2:
* fixed null-checking of extack (with break; as suggested by kuba)
* added logging of full string on truncation (Johannes)
Edward Cree (3):
netlink: add support for formatted extack messages
sfc: use formatted extacks instead of efx_tc_err()
sfc: remove 'log-tc-errors' ethtool private flag
drivers/net/ethernet/sfc/ef100_ethtool.c | 2 - drivers/net/ethernet/sfc/ethtool_common.c | 37 ------------------ drivers/net/ethernet/sfc/ethtool_common.h | 2 - drivers/net/ethernet/sfc/mae.c | 5 +-- drivers/net/ethernet/sfc/net_driver.h | 2 - drivers/net/ethernet/sfc/tc.c | 47 ++++++++++------------- drivers/net/ethernet/sfc/tc.h | 18 --------- include/linux/netlink.h | 25 +++++++++++- 8 files changed, 46 insertions(+), 92 deletions(-)
RFC PATCH v2 1/3 netlink: add support for formatted extack messages
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.
--- include/linux/netlink.h | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
include/linux/netlink.h +23 -2
@@ -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); \
Jakub Kicinski
maintainerRe: [RFC PATCH v2 net-next 1/3] netlink: add support for formatted extack messages
include/linux/netlink.h struct netlink_ext_ack { · line 117
Quoted 5 lines · On Thu, 13 Oct 2022 10:23:00 +0100 [email protected] wrote:
include/linux/netlink.h +5
-0+ 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?)
Re: [RFC PATCH v2 net-next 1/3] netlink: add support for formatted extack messages
Quoted 8 lines · Jakub Kicinski
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
Jakub Kicinski
maintainerRe: [RFC PATCH v2 net-next 1/3] netlink: add support for formatted extack messages
Quoted 2 lines · 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 :(
Edward Cree
maintainerRe: [RFC PATCH v2 net-next 1/3] netlink: add support for formatted extack messages
In reply to Jakub KicinskiQuoted 2 lines · Jakub Kicinski
(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?
Jakub Kicinski
maintainerRe: [RFC PATCH v2 net-next 1/3] netlink: add support for formatted extack messages
Quoted 8 lines · On Mon, 17 Oct 2022 13:04:35 +0100 Edward Cree wrote:
Quoted 2 lines · Jakub Kicinski
(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 andnet_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.
RFC PATCH v2 2/3 sfc: use formatted extacks instead of efx_tc_err()
In reply to [email protected]From: Edward Cree <[email protected]>
Since we can now get a formatted message back to the user with
NL_SET_ERR_MSG_FMT_MOD(), there's no need for our special logging.
--- drivers/net/ethernet/sfc/mae.c | 5 ++-- drivers/net/ethernet/sfc/tc.c | 47 +++++++++++++++------------------- drivers/net/ethernet/sfc/tc.h | 18 ------------- 3 files changed, 23 insertions(+), 47 deletions(-)
drivers/net/ethernet/sfc/mae.c +2 -3
@@ -265,9 +265,8 @@ int efx_mae_match_check_caps(struct efx_nic *efx, rc =efx_mae_match_check_cap_typ (supported_fields[MAE_FIELD_INGRESS_PORT ], ingress_port_mask_type);if (rc) { -efx_tc_err (efx,"No support for %s mask in field ingress_port\n" , -mask_type_name (ingress_port_mask_type)); -NL_SET_ERR_MSG_MOD (extack,"Unsupported mask type for ingress_port" ); +NL_SET_ERR_MSG_FMT_MOD (extack,"No support for %s mask in field ingress_port" , +mask_type_name (ingress_port_mask_type));return rc; }return 0 ;
drivers/net/ethernet/sfc/tc.c +21 -26
139 lines not on this page · raw message
drivers/net/ethernet/sfc/tc.h +0 -18
@@ -15,24 +15,6 @@#include <linux/rhashtable.h> #include "net_driver.h" -/* Error reporting: convenience macros. For indicating why a given filter -* insertion is not supported; errors in internal operation or in the -* hardware should be netif_err()s instead. -*/ -/* Used when error message is constant. */ -#define EFX_TC_ERR_MSG (efx, extack, message) do { \ - NL_SET_ERR_MSG_MOD(extack, message); \ - if (efx->log_tc_errs) \ - netif_info(efx, drv, efx->net_dev, "%s\n", message); \ -} while (0) -/* Used when error message is not constant; caller should also supply a -* constant extack message with NL_SET_ERR_MSG_MOD(). -*/ -#define efx_tc_err(efx, fmt, args...) do { \ -if (efx->log_tc_errs) \ - netif_info(efx, drv, efx->net_dev, fmt, ##args);\ -} while (0) -struct efx_tc_action_set {u16 deliver:1 ;u32 dest_mport;
RFC PATCH v2 3/3 sfc: remove 'log-tc-errors' ethtool private flag
In reply to [email protected]From: Edward Cree <[email protected]>
It no longer does anything now that we're using formatted extacks instead. So we can remove the driver's whole get/set priv_flags implementation.
--- drivers/net/ethernet/sfc/ef100_ethtool.c | 2 -- drivers/net/ethernet/sfc/ethtool_common.c | 37 ----------------------- drivers/net/ethernet/sfc/ethtool_common.h | 2 -- drivers/net/ethernet/sfc/net_driver.h | 2 -- 4 files changed, 43 deletions(-)
drivers/net/ethernet/sfc/ef100_ethtool.c +0 -2
@@ -43,8 +43,6 @@ const struct ethtool_ops ef100_ethtool_ops = { .get_pauseparam = efx_ethtool_get_pauseparam, .set_pauseparam = efx_ethtool_set_pauseparam, .get_sset_count = efx_ethtool_get_sset_count, - .get_priv_flags = efx_ethtool_get_priv_flags, - .set_priv_flags = efx_ethtool_set_priv_flags, .self_test = efx_ethtool_self_test, .get_strings = efx_ethtool_get_strings, .get_link_ksettings = efx_ethtool_get_link_ksettings,
drivers/net/ethernet/sfc/ethtool_common.c +0 -37
@@ -101,14 +101,6 @@ static const struct efx_sw_stat_desc efx_sw_stat_desc[] = {#define EFX_ETHTOOL_SW_STAT_COUNT ARRAY_SIZE(efx_sw_stat_desc) -static const char efx_ethtool_priv_flags_strings[][ETH_GSTRING_LEN ] = { -"log-tc-errors" , -}; - -#define EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS BIT(0) - -#define EFX_ETHTOOL_PRIV_FLAGS_COUNT ARRAY_SIZE(efx_ethtool_priv_flags_strings) -void efx_ethtool_get_drvinfo (struct net_device *net_dev,struct ethtool_drvinfo *info) {
@@ -460,8 +452,6 @@ int efx_ethtool_get_sset_count(struct net_device *net_dev, int string_set)efx_ptp_describe_stats (efx,NULL );case ETH_SS_TEST :return efx_ethtool_fill_self_tests (efx,NULL ,NULL ,NULL ); -case ETH_SS_PRIV_FLAGS : -return EFX_ETHTOOL_PRIV_FLAGS_COUNT ;default :return -EINVAL ; }
@@ -488,39 +478,12 @@ void efx_ethtool_get_strings(struct net_device *net_dev,case ETH_SS_TEST :efx_ethtool_fill_self_tests (efx,NULL , strings,NULL );break ; -case ETH_SS_PRIV_FLAGS : -for (i =0 ; i <EFX_ETHTOOL_PRIV_FLAGS_COUNT ; i++) -strscpy (strings + i *ETH_GSTRING_LEN , - efx_ethtool_priv_flags_strings[i], -ETH_GSTRING_LEN ); -break ;default :/* No other string sets */ break ; } } -u32 efx_ethtool_get_priv_flags (struct net_device *net_dev) -{ -struct efx_nic *efx =efx_netdev_priv (net_dev); -u32 ret_flags =0 ; - -if (efx->log_tc_errs) - ret_flags |=EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS ; - -return ret_flags; -} - -int efx_ethtool_set_priv_flags (struct net_device *net_dev,u32 flags) -{ -struct efx_nic *efx =efx_netdev_priv (net_dev); - - efx->log_tc_errs = - !!(flags &EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS ); - -return 0 ; -} -void efx_ethtool_get_stats (struct net_device *net_dev,struct ethtool_stats *stats,u64 *data)
drivers/net/ethernet/sfc/ethtool_common.h +0 -2
@@ -27,8 +27,6 @@ int efx_ethtool_fill_self_tests(struct efx_nic *efx,int efx_ethtool_get_sset_count (struct net_device *net_dev,int string_set);void efx_ethtool_get_strings (struct net_device *net_dev,u32 string_set,u8 *strings); -u32 efx_ethtool_get_priv_flags (struct net_device *net_dev); -int efx_ethtool_set_priv_flags (struct net_device *net_dev,u32 flags);void efx_ethtool_get_stats (struct net_device *net_dev,struct ethtool_stats *stats __attribute__ ((unused)),u64 *data);
drivers/net/ethernet/sfc/net_driver.h +0 -2
@@ -855,7 +855,6 @@ enum efx_xdp_tx_queues_mode {* @timer_max_ns: Interrupt timer maximum value, in nanoseconds * @irq_rx_adaptive: Adaptive IRQ moderation enabled for RX event queues * @irqs_hooked: Channel interrupts are hooked -* @log_tc_errs: Error logging for TC filter insertion is enabled * @irq_rx_mod_step_us: Step size for IRQ moderation for RX event queues * @irq_rx_moderation_us: IRQ moderation time for RX event queues * @msg_enable: Log message enable flags
@@ -1018,7 +1017,6 @@ struct efx_nic {unsigned int timer_max_ns;bool irq_rx_adaptive;bool irqs_hooked; -bool log_tc_errs;unsigned int irq_mod_step_us;unsigned int irq_rx_moderation_us;u32 msg_enable;