netdev - Re: [RFC PATCH net-next 1/6] net: dsa: make dp->bridge

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]

Date:   Thu, 11 Nov 2021 13:18:50 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Vladimir Oltean <vladimir.oltean@....com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        DENG Qingfang <dqfext@...il.com>
Subject: Re: [RFC PATCH net-next 1/6] net: dsa: make dp->bridge_num one-based

On 11/11/21 13:45, Vladimir Oltean wrote:
> On Thu, Nov 11, 2021 at 12:24:47PM +0000, Alvin Šipraga wrote:
>> On 10/26/21 18:26, Vladimir Oltean wrote:
>>> I have seen too many bugs already due to the fact that we must encode an
>>> invalid dp->bridge_num as a negative value, because the natural tendency
>>> is to check that invalid value using (!dp->bridge_num). Latest example
>>> can be seen in commit 1bec0f05062c ("net: dsa: fix bridge_num not
>>> getting cleared after ports leaving the bridge").
>>>
>>> Convert the existing users to assume that dp->bridge_num == 0 is the
>>> encoding for invalid, and valid bridge numbers start from 1.
>>>
>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>>> ---
>>
>> Reviewed-by: Alvin Šipraga <alsi@...g-olufsen.dk>
> 
> Thanks for the review.
> 
>> Small remark inline.
>>
>>> -int dsa_bridge_num_get(const struct net_device *bridge_dev, int max)
>>> +unsigned int dsa_bridge_num_get(const struct net_device *bridge_dev, int max)
>>>    {
>>> -	int bridge_num = dsa_bridge_num_find(bridge_dev);
>>> +	unsigned int bridge_num = dsa_bridge_num_find(bridge_dev);
>>>    
>>> -	if (bridge_num < 0) {
>>> +	if (!bridge_num) {
>>>    		/* First port that offloads TX forwarding for this bridge */
>>
>> Perhaps you want to update this comment in patch 2/6, since bridge_num
>> is no longer just about TX forwarding offload.
>>
>>> -		bridge_num = find_first_zero_bit(&dsa_fwd_offloading_bridges,
>>> -						 DSA_MAX_NUM_OFFLOADING_BRIDGES);
>>> +		bridge_num = find_next_zero_bit(&dsa_fwd_offloading_bridges,
>>> +						DSA_MAX_NUM_OFFLOADING_BRIDGES,
>>> +						1);
> 
> I will update this comment in patch 2 to say "First port that requests
> FDB isolation or TX forwarding offload for this bridge". Sounds ok?
> 

Yes sounds good - that's also what I had in mind.