lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230126223213.riq6i2gdztwuinwi@skbuf>
Date:   Fri, 27 Jan 2023 00:32:13 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Leon Romanovsky <leonro@...dia.com>,
        Andrew Lunn <andrew@...n.ch>,
        bridge@...ts.linux-foundation.org,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, Nikolay Aleksandrov <razor@...ckwall.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Roopa Prabhu <roopa@...dia.com>
Subject: Re: [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@...dia.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@...dia.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).

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ