On Thu, Jan 26, 2023 at 09:15:03PM +0200, Leon Romanovsky wrote: > From: Leon Romanovsky <[email protected]> > > 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 <[email protected]> > --- 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).
