netdev - Re: [PATCH net-next 1/2] net: bridge: add support for user-controlled bool options

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: <47484385-f944-e8bf-c358-e3436dfd776d@cumulusnetworks.com>
Date:   Thu, 22 Nov 2018 18:13:09 +0200
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, roopa@...ulusnetworks.com,
        davem@...emloft.net, bridge@...ts.linux-foundation.org
Subject: Re: [PATCH net-next 1/2] net: bridge: add support for user-controlled
 bool options

On 22/11/2018 17:49, Andrew Lunn wrote:
>> +/* br_boolopt_toggle - change user-controlled boolean option
>> + *
>> + * @br: bridge device
>> + * @opt: id of the option to change
>> + * @on: new option value
>> + *
>> + * Changes the value of the respective boolean option to @on taking care of
>> + * any internal option value mapping and configuration.
>> + */
>> +int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id opt, bool on)
>> +{
>> +	int err = -ENOENT;
>> +
>> +	switch (opt) {
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return err;
>> +}
>> +
> 
> 
>> +int br_boolopt_multi_toggle(struct net_bridge *br,
>> +			    struct br_boolopt_multi *bm)
>> +{
>> +	unsigned long bitmap = bm->optmask;
>> +	int err = 0;
>> +	int opt_id;
>> +
>> +	for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) {
>> +		bool on = !!(bm->optval & BIT(opt_id));
>> +
>> +		err = br_boolopt_toggle(br, opt_id, on);
>> +		if (err) {
>> +			br_debug(br, "boolopt multi-toggle error: option: %d current: %d new: %d error: %d\n",
>> +				 opt_id, br_boolopt_get(br, opt_id), on, err);
>> +			break;
>> +		}
> 
> An old kernel with a new iproute2 might partially succeed in toggling
> some low bits, but then silently ignore a high bit that is not
> supported by the kernel. As a user, i want to know the kernel does not
> support an option i'm trying to toggle.
> 

That is already how netlink option setting works, if the option isn't supported
it is silently ignored. I tried to stay as close to the current behaviour as possible.
It also applies to partial option changes, some options will be changed until an error
is encountered.

> Can you walk all the bits in the u32 from the MSB to the LSB? That
> should avoid this problem.
> 

I did wonder about this and left it as netlink option behaviour but
I can leave the walk as is and just check if the highest order set bit >= BR_BOOLOPT_MAX
and err out with ENOTSUPP for example. Will reconsider for v2.

> 	Andrew
>