netdev - Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

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 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:   Sun, 20 Nov 2022 11:21:08 +0100
From:   netdev@...io-technology.com
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB
 implementation

On 2022-11-15 23:23, Vladimir Oltean wrote:
>> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c 
>> b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> index 8a874b6fc8e1..0a57f4e7dd46 100644
>> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> @@ -12,6 +12,7 @@
>> 
>>  #include "chip.h"
>>  #include "global1.h"
>> +#include "switchdev.h"
>> 
>>  /* Offset 0x01: ATU FID Register */
>> 
>> @@ -426,6 +427,8 @@ static irqreturn_t 
>> mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>>  	if (err)
>>  		goto out;
>> 
>> +	mv88e6xxx_reg_unlock(chip);
>> +
> 
> I concur with Ido's suggestion to split up changes which are only
> tangentially related as preparatory patches, with the motivation which
> you explained over email as the commit message. Also, the current "out"
> label needs to become something like "out_unlock", and a new "out"
> created, for the error path jumps below, that don't have the register
> lock held.
> 
>>  	spid = entry.state;
>> 
>>  	if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) {
>> @@ -446,6 +449,12 @@ static irqreturn_t 
>> mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>>  				    "ATU miss violation for %pM portvec %x spid %d\n",
>>  				    entry.mac, entry.portvec, spid);
>>  		chip->ports[spid].atu_miss_violation++;
>> +
>> +		if (fid && chip->ports[spid].mab)
>> +			err = mv88e6xxx_handle_violation(chip, spid, &entry,
>> +							 fid, MV88E6XXX_G1_ATU_OP_MISS_VIOLATION);
> 
> The check for non-zero FID looks strange until one considers that FID 0
> is MV88E6XXX_FID_STANDALONE. But then again, since only standalone 
> ports
> use FID 0 and standalone ports cannot have the MAB/locked feature 
> enabled,
> I consider the check to be redundant. We should know for sure that the
> FID is non-zero.
> 

I have something like this, using 'mvls vtu' from 
https://github.com/wkz/mdio-tools:
  VID   FID  SID  P  Q  F  0  1  2  3  4  5  6  7  8  9  a
    0     0    0  y  -  -  =  =  =  =  =  =  =  =  =  =  =
    1     2    0  -  -  -  u  u  u  u  u  u  u  u  u  u  =
4095     1    0  -  -  -  =  =  =  =  =  =  =  =  =  =  =

as a vtu table. I don't remember exactly the consequences, but I am 
quite sure that fid=0 gave
incorrect handling, but there might be something that I have missed as 
to other setups.