| Message ID | 2919eb55e2e9b92265a3ba600afc8137a901ae5f.1674760340.git.leon@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] netlink: provide an ability to set default extack message | expand |
On Thu, Jan 26, 2023 at 09:15:03PM +0200, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@nvidia.com> > > In netdev common pattern, xxtack pointer is forwarded to the drivers ~~~~~~ extack > to be filled with error message. However, the caller can easily > overwrite the filled message. > > Instead of adding multiple "if (!extack->_msg)" checks before any > NL_SET_ERR_MSG() call, which appears after call to the driver, let's > add this check to common code. > > [1] https://lore.kernel.org/all/Y9Irgrgf3uxOjwUm@unreal > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > --- I would somewhat prefer not doing this, and instead introducing a new NL_SET_ERR_MSG_WEAK() of sorts. The reason has to do with the fact that an extack is sometimes also used to convey warnings rather than hard errors, for example right here in net/dsa/slave.c: if (err == -EOPNOTSUPP) { if (extack && !extack->_msg) NL_SET_ERR_MSG_MOD(extack, "Offloading not supported"); NL_SET_ERR_MSG_MOD(extack, "Offloading not supported"); err = 0; } Imagine (not the case here) that below such a "warning extack" lies something like this: if (arg > range) { NL_SET_ERR_MSG_MOD(extack, "Argument outside expected range"); return -ERANGE; } What you'll get is: Error: Offloading not supported (error code -ERANGE). whereas before, we relied on any NL_SET_ERR_MSG_MOD() call to overwrite the "warning" extack, and that to only be shown on error code 0. Also, if we make this change this way, there's no going back (just like there's no going back from kfree(NULL), rtnl_lock() and others).
On Fri, 27 Jan 2023 00:32:13 +0200 Vladimir Oltean wrote: > On Thu, Jan 26, 2023 at 09:15:03PM +0200, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > In netdev common pattern, xxtack pointer is forwarded to the drivers > ~~~~~~ > extack > > > to be filled with error message. However, the caller can easily > > overwrite the filled message. > > > > Instead of adding multiple "if (!extack->_msg)" checks before any > > NL_SET_ERR_MSG() call, which appears after call to the driver, let's > > add this check to common code. > > > > [1] https://lore.kernel.org/all/Y9Irgrgf3uxOjwUm@unreal > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > --- > > I would somewhat prefer not doing this, and instead introducing a new > NL_SET_ERR_MSG_WEAK() of sorts. That'd be my preference too, FWIW. It's only the offload cases which need this sort of fallback. BTW Vladimir, I remember us discussing this. I was searching the archive as you sent this, but can't find the thread. Mostly curious whether I flip flipped on this or I'm not completely useless :) > The reason has to do with the fact that an extack is sometimes also > used to convey warnings rather than hard errors, for example right here > in net/dsa/slave.c: > > if (err == -EOPNOTSUPP) { > if (extack && !extack->_msg) > NL_SET_ERR_MSG_MOD(extack, > "Offloading not supported"); > NL_SET_ERR_MSG_MOD(extack, > "Offloading not supported"); > err = 0; > }
On Thu, Jan 26, 2023 at 02:37:23PM -0800, Jakub Kicinski wrote: > > I would somewhat prefer not doing this, and instead introducing a new > > NL_SET_ERR_MSG_WEAK() of sorts. > > That'd be my preference too, FWIW. It's only the offload cases which > need this sort of fallback. > > BTW Vladimir, I remember us discussing this. I was searching the > archive as you sent this, but can't find the thread. Mostly curious > whether I flip flipped on this or I'm not completely useless :) What we discussed was on a patch of mine fixing "if (!extack->_msg)" to "if (extack && !extack->_msg)". I never proposed a new macro wrapper (you did), but I didn't do it at the time because it was a patch for "net", and I forgot to put a reminder for the next net->net-next merge. https://lore.kernel.org/netdev/20220822182523.6821e176@kernel.org/ And from there, out of sight, out of mind.
On Fri, 27 Jan 2023 00:44:57 +0200 Vladimir Oltean wrote: > On Thu, Jan 26, 2023 at 02:37:23PM -0800, Jakub Kicinski wrote: > > > I would somewhat prefer not doing this, and instead introducing a new > > > NL_SET_ERR_MSG_WEAK() of sorts. > > > > That'd be my preference too, FWIW. It's only the offload cases which > > need this sort of fallback. > > > > BTW Vladimir, I remember us discussing this. I was searching the > > archive as you sent this, but can't find the thread. Mostly curious > > whether I flip flipped on this or I'm not completely useless :) > > What we discussed was on a patch of mine fixing "if (!extack->_msg)" to > "if (extack && !extack->_msg)". I never proposed a new macro wrapper > (you did), but I didn't do it at the time because it was a patch for > "net", and I forgot to put a reminder for the next net->net-next merge. > https://lore.kernel.org/netdev/20220822182523.6821e176@kernel.org/ > And from there, out of sight, out of mind. That explains it, I was running blame the message lines, not the if (). Thanks for digging it up!
On Fri, Jan 27, 2023 at 12:32:13AM +0200, Vladimir Oltean wrote: > On Thu, Jan 26, 2023 at 09:15:03PM +0200, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > In netdev common pattern, xxtack pointer is forwarded to the drivers > ~~~~~~ > extack > > > to be filled with error message. However, the caller can easily > > overwrite the filled message. > > > > Instead of adding multiple "if (!extack->_msg)" checks before any > > NL_SET_ERR_MSG() call, which appears after call to the driver, let's > > add this check to common code. > > > > [1] https://lore.kernel.org/all/Y9Irgrgf3uxOjwUm@unreal > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > --- > > I would somewhat prefer not doing this, and instead introducing a new > NL_SET_ERR_MSG_WEAK() of sorts. It means changing ALL error unwind places where extack was forwarded before to subfunctions. Places like this: ret = func(..., extack) if (ret) { NL_SET_ERR_MSG_MOD... return ret; } will need to be changed to something like this: ret = func(..., extack) if (ret) { NL_SET_ERR_MSG_WEAK... return ret; } > > The reason has to do with the fact that an extack is sometimes also > used to convey warnings rather than hard errors, for example right here > in net/dsa/slave.c: > > if (err == -EOPNOTSUPP) { > if (extack && !extack->_msg) > NL_SET_ERR_MSG_MOD(extack, > "Offloading not supported"); > NL_SET_ERR_MSG_MOD(extack, > "Offloading not supported"); > err = 0; > } > > Imagine (not the case here) that below such a "warning extack" lies > something like this: > > if (arg > range) { > NL_SET_ERR_MSG_MOD(extack, "Argument outside expected range"); > return -ERANGE; > } > > What you'll get is: > > Error: Offloading not supported (error code -ERANGE). > > whereas before, we relied on any NL_SET_ERR_MSG_MOD() call to overwrite > the "warning" extack, and that to only be shown on error code 0. Can we please discuss current code and not over-engineered case which doesn't exist in the reality? Even for your case, I would like to see NL_SET_ERR_MSG_FORCE() to explicitly say that message will be overwritten. Thanks
On Thu, Jan 26, 2023 at 02:37:23PM -0800, Jakub Kicinski wrote: > On Fri, 27 Jan 2023 00:32:13 +0200 Vladimir Oltean wrote: > > On Thu, Jan 26, 2023 at 09:15:03PM +0200, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > In netdev common pattern, xxtack pointer is forwarded to the drivers > > ~~~~~~ > > extack > > > > > to be filled with error message. However, the caller can easily > > > overwrite the filled message. > > > > > > Instead of adding multiple "if (!extack->_msg)" checks before any > > > NL_SET_ERR_MSG() call, which appears after call to the driver, let's > > > add this check to common code. > > > > > > [1] https://lore.kernel.org/all/Y9Irgrgf3uxOjwUm@unreal > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > > --- > > > > I would somewhat prefer not doing this, and instead introducing a new > > NL_SET_ERR_MSG_WEAK() of sorts. > > That'd be my preference too, FWIW. It's only the offload cases which > need this sort of fallback. Of course not, almost any error unwind path which sets extack will need it. See devlink as an example, but I'm confident that same issue exists in other places too. You are suggesting API which is very easy to do wrong. So I prefer to stay with my proposal if it is possible. Thanks
On Fri, 27 Jan 2023 07:26:13 +0200 Leon Romanovsky wrote: > > That'd be my preference too, FWIW. It's only the offload cases which > > need this sort of fallback. > > Of course not, almost any error unwind path which sets extack will need it. I guess we can come up with scenarios where the new behavior would be useful. But the fact is - your patch changes 4 places... > See devlink as an example I don't know what part of devlink you mean at a quick scroll.
On Fri, Jan 27, 2023 at 07:22:26AM +0200, Leon Romanovsky wrote: > It means changing ALL error unwind places where extack was forwarded > before to subfunctions. > > Places like this: > ret = func(..., extack) > if (ret) { > NL_SET_ERR_MSG_MOD... > return ret; > } > > will need to be changed to something like this: > ret = func(..., extack) > if (ret) { > NL_SET_ERR_MSG_WEAK... > return ret; > } Yeah, but my point is that you inspect the code that you plan to convert, rather than converting it in bulk and inspecting later... > Can we please discuss current code and not over-engineered case which > doesn't exist in the reality? > > Even for your case, I would like to see NL_SET_ERR_MSG_FORCE() to > explicitly say that message will be overwritten. __nla_validate_parse() if (unlikely(rem > 0)) { pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n", rem, current->comm); NL_SET_ERR_MSG(extack, "bytes leftover after parsing attributes"); if (validate & NL_VALIDATE_TRAILING) return -EINVAL; } return 0; called by nla_validate_deprecated() with validate == NL_VALIDATE_LIBERAL followed by other extack setting in tunnel_key_copy_opts(), which will not overwrite the initial warning message.
On Thu, Jan 26, 2023 at 11:26:15PM -0800, Jakub Kicinski wrote: > On Fri, 27 Jan 2023 07:26:13 +0200 Leon Romanovsky wrote: > > > That'd be my preference too, FWIW. It's only the offload cases which > > > need this sort of fallback. > > > > Of course not, almost any error unwind path which sets extack will need it. > > I guess we can come up with scenarios where the new behavior would > be useful. But the fact is - your patch changes 4 places... ok, I'll rename. > > > See devlink as an example > > I don't know what part of devlink you mean at a quick scroll. I overlooked "return err" in the middle. You are right. Thanks
diff --git a/include/linux/netlink.h b/include/linux/netlink.h index 38f6334f408c..87d2900cb448 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -101,7 +101,7 @@ struct netlink_ext_ack { \ do_trace_netlink_extack(__msg); \ \ - if (__extack) \ + if (__extack && !__extack->_msg) \ __extack->_msg = __msg; \ } while (0) @@ -111,7 +111,7 @@ struct netlink_ext_ack { #define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do { \ struct netlink_ext_ack *__extack = (extack); \ \ - if (!__extack) \ + if (!__extack || __extack->_msg) \ break; \ if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN, \ "%s" fmt "%s", "", ##args, "") >= \ diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index 7eb6fd5bb917..9f7ff63ef853 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -104,9 +104,8 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, return 0; if (err) { - if (extack && !extack->_msg) - NL_SET_ERR_MSG_MOD(extack, - "bridge flag offload is not supported"); + NL_SET_ERR_MSG_MOD(extack, + "bridge flag offload is not supported"); return -EOPNOTSUPP; } @@ -115,9 +114,8 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, err = switchdev_port_attr_set(p->dev, &attr, extack); if (err) { - if (extack && !extack->_msg) - NL_SET_ERR_MSG_MOD(extack, - "error setting offload flag on port"); + NL_SET_ERR_MSG_MOD(extack, + "error setting offload flag on port"); return err; } diff --git a/net/dsa/master.c b/net/dsa/master.c index 26d90140d271..bcf39c524664 100644 --- a/net/dsa/master.c +++ b/net/dsa/master.c @@ -464,9 +464,7 @@ int dsa_master_lag_setup(struct net_device *lag_dev, struct dsa_port *cpu_dp, err = dsa_port_lag_join(cpu_dp, lag_dev, uinfo, extack); if (err) { - if (extack && !extack->_msg) - NL_SET_ERR_MSG_MOD(extack, - "CPU port failed to join LAG"); + NL_SET_ERR_MSG_MOD(extack, "CPU port failed to join LAG"); goto out_master_teardown; } diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 6014ac3aad34..c5527aa2c403 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2692,9 +2692,8 @@ static int dsa_slave_changeupper(struct net_device *dev, if (!err) dsa_bridge_mtu_normalization(dp); if (err == -EOPNOTSUPP) { - if (extack && !extack->_msg) - NL_SET_ERR_MSG_MOD(extack, - "Offloading not supported"); + NL_SET_ERR_MSG_MOD(extack, + "Offloading not supported"); err = 0; } err = notifier_from_errno(err);