netdev - Re: [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops

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]

Date:   Thu, 31 Oct 2019 21:28:01 +0200
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Joe Perches <joe@...ches.com>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, roopa@...ulusnetworks.com,
        bridge@...ts.linux-foundation.org
Subject: Re: [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops

On 31/10/2019 20:37, Joe Perches wrote:
> On Tue, 2019-10-29 at 13:45 +0200, Nikolay Aleksandrov wrote:
>> Hi,
>> We'd like to have a well-defined behaviour when changing fdb flags. The
>> problem is that we've added new fields which are changed from all
>> contexts without any locking. We are aware of the bit test/change races
>> and these are fine (we can remove them later), but it is considered
>> undefined behaviour to change bitfields from multiple threads and also
>> on some architectures that can result in unexpected results,
>> specifically when all fields between the changed ones are also
>> bitfields. The conversion to bitops shows the intent clearly and
>> makes them use functions with well-defined behaviour in such cases.
>> There is no overhead for the fast-path, the bit changing functions are
>> used only in special cases when learning and in the slow path.
>> In addition this conversion allows us to simplify fdb flag handling and
>> avoid bugs for future bits (e.g. a forgetting to clear the new bit when
>> allocating a new fdb). All bridge selftests passed, also tried all of the
>> converted bits manually in a VM.
>>
>> Thanks,
>>  Nik
>>
>> Nikolay Aleksandrov (7):
>>   net: bridge: fdb: convert is_local to bitops
>>   net: bridge: fdb: convert is_static to bitops
>>   net: bridge: fdb: convert is_sticky to bitops
>>   net: bridge: fdb: convert added_by_user to bitops
>>   net: bridge: fdb: convert added_by_external_learn to use bitops
>>   net: bridge: fdb: convert offloaded to use bitops
>>   net: bridge: fdb: set flags directly in fdb_create
> 
> Wouldn't it be simpler to change all these bitfields to bool?
> 
> The next member is ____cachline_aligned_in_smp so it's not
> like the struct size matters or likely even changes.
> 

I guess it's just preference now, I'd prefer having 1 field which is well
packed and can contain more bits (and more are to come) instead of bunch
of bool or u8 fields which is a waste of space. We can set them together, it's more
compact and also the atomic bitops make it clear that these can change
without any locking. We're about to add new bits to these and it's nice
to have a clear understanding and well-defined API for them. Specifically
the test_and_set/clear_bit() can simplify code considerably.

> ---
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index ce2ab1..46d2f10 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -183,12 +183,12 @@ struct net_bridge_fdb_entry {
>  
>  	struct net_bridge_fdb_key	key;
>  	struct hlist_node		fdb_node;
> -	unsigned char			is_local:1,
> -					is_static:1,
> -					is_sticky:1,
> -					added_by_user:1,
> -					added_by_external_learn:1,
> -					offloaded:1;
> +	bool				is_local;
> +	bool				is_static;
> +	bool				is_sticky;
> +	bool				added_by_user;
> +	bool				added_by_external_learn;
> +	bool				offloaded;
>  
>  	/* write-heavy members should not affect lookups */
>  	unsigned long			updated ____cacheline_aligned_in_smp;
> 
>