Linux Ethernet Bridging - Jan 2023 - [Bridge] [PATCH net-next] netlink: provide an ability to set default extack message

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 at 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 at unreal
> > Signed-off-by: Leon Romanovsky <leonro at 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 at kernel.org/
And from there, out of sight, out of mind.

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 at 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 at unreal
> > > Signed-off-by: Leon Romanovsky <leonro at 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