netdev - Re: [PATCH net-next v2 00/13] Change DSA's FDB API and perform switchdev cleanup

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:   Fri, 4 Aug 2017 01:39:02 +0300
From:   Arkadi Sharshevsky <arkadis@...lanox.com>
To:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        netdev@...r.kernel.org, jiri@...nulli.us
Cc:     davem@...emloft.net, ivecera@...hat.com, f.fainelli@...il.com,
        andrew@...n.ch, Woojung.Huh@...rochip.com,
        stephen@...workplumber.org, mlxsw@...lanox.com
Subject: Re: [PATCH net-next v2 00/13] Change DSA's FDB API and perform
 switchdev cleanup


[...]

>> Now we have the "offload" read only flag, which is good to inform about
>> a successfully programmed hardware, but adds another level of complexity
>> to understand the interaction with the hardware.
>>
>> I think iproute2 is getting more and more confusing. From what I
>> understood, respecting the "self" flag as described is not possible
>> anymore due to some retro-compatibility reasons.
>>
>> Also Linux must use the hardware as an accelerator (so "self" or
>> "offload" must be the default), and always fall back to software
>> otherwise, hence "master" do not make sense here.
>>
>> What do you think about this synopsis for bridge fdb add?
>>
>>     # bridge fdb add LLADDR dev DEV [ offload { on | off } ]
>>
>> Where offload defaults to "on". This option should also be ported to
>> other offloaded features like MDB and VLAN. Even though this is a bit
>> out of scope of this patchset, do you think this is feasible?
>>
> 
> I agree completely that currently its confusing. The documentation
> should be updated for sure. I think that 'self' was primarily introduced
> (Commit 77162022a) for NIC embedded switches which are used for sriov, in
> that case the self is related to the internal eswitch, which completely
> diverge from the software one (clearly not swithcdev).
> 
> IMHO For switchdev devices 'self' should not be an option at all, or any
> other arg regarding hardware. Furthermore, the 'offload' flag should be
> only relevant during the dump as an indication to the user.
> 
> Unfortunately, the  lack of ability of syncing the sw with hw in DSA's
> case introduces a problem for indicating that the entries are only
> in hw, I mean marking it only as offloaded is not enough.

Hi,

It seems impossible currently to move the self to be the default, and
this introduces regression which you don't approve, so it seems few
options left:

a) Leave two ways to add fdb, through the bridge (by using the master
   flag) which is introduced in this patchset, and by using the self
   which is the legacy way. In this way no regression will be introduced,
   yet, it feels confusing a bit. The benefit is that we (DSA/mlxsw)
   will be synced.
b) Leave only the self (which means removing patch no 4,5).

In both cases the switchdev implementation of .ndo_fdb_add() will be
moved inside DSA in a similar way to the dump because its only used by
you.

Option b) actually turns this patchset into cosmetic one which does
only cleanup.

Thanks,
Arkadi