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] [day] [month] [year] [list]

Message-ID: <c5d16533-8a53-a418-08df-499fa82bb6f9@cumulusnetworks.com>
Date:   Fri, 23 Nov 2018 03:55:24 +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 18:13, Nikolay Aleksandrov wrote:
<snip>
>>> +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.
> 

Actually this doesn't improve user experience, after testing a little the problem is
that we can't return to the user the exact option that failed in any understandable manner.
The options are:
 - exact error message: (no bit/option value) so pretty much just say "Failed bool option"
 - pr_err: this has 2 issues, first it can't be retrieved by the caller (will be in the logs)
           and more importantly the best we can do is print the option bit that we don't support
           which really doesn't help the user at all

So the example is having newer iproute2 on older kernel and trying to set non-existing option
mixed with existing ones - the user has no way of determining which one failed even if we print
the bit, so this is really frustrating. The current way of ignoring the missing options seems
a bit more user-friendly and also with the change that we return the supported bit options
in optmask (thanks for the suggestion), the user-space tool (iproute2) can determine if an
option is not supported and at least give an indication when dumping it.
All of this does align with how netlink currently handles options and people are used to it.