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

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.

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 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 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 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.
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