linux-kernel - Re: [PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

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: <be302ef8-7f12-aaf1-5f68-c4de89c7d83d@gmail.com>
Date:   Mon, 23 Oct 2017 17:54:13 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...oirfairelinux.com,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next 0/2] net: dsa: don't unmask port bitmaps

On 10/23/2017 02:26 PM, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@...n.ch> writes:
> 
>> On Mon, Oct 23, 2017 at 02:17:29PM -0400, Vivien Didelot wrote:
>>> DSA has several bitmaps to store the type of ports: cpu_port_mask,
>>> dsa_port_mask and enabled_port_mask. But the code is inconsistently
>>> unmasking them.
>>>
>>> The legacy code tries to unmask cpu_port_mask and dsa_port_mask but
>>> skips enabled_port_mask.
>>>
>>> The new bindings unmasks cpu_port_mask and enabled_port_mask but skips
>>> dsa_port_mask.
>>>
>>> In fact there is no need to unmask them because we are in the error
>>> path, and they won't be used after. Instead of fixing the unmasking,
>>> simply remove them.
>>
>> I'm not looked at the code, travelling and don't have time.
> 
> heu, ok.
> 
>> What happens if the failure is -PROBE_DEFERRED, and it tried again
>> later. Will these masks be set back to 0? Or will they retain the old
>> values? I think there is supposed to be symmetry here, so that we undo
>> what we did, and so the next time we try again, we start from a good
>> state.

In case of probe deferral, you get the full probe function to exit with
an error, and that usually involves freeing the recently allocated
dsa_switch instance, and then allocating a new one when probe is
re-entered, so that should not be a problem.

> 
> The type of a port is a static information parsed either from device
> tree or from platform data. Thus there is no symmetry needed here.
> 
> The fact that the unmasking of these bitmaps is currently erroneous also
> shows that it is unnecessary.

As much as I would like to see this being symmetrical here, as Vivien
points out, this does not appear to be the case because of missing code,
and it does not seem to solve a particular problem in being symmetrical.
-- 
Florian