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]

Message-ID: <20170804052301.GA1906@nanopsycho.orion>
Date:   Fri, 4 Aug 2017 07:23:01 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Arkadi Sharshevsky <arkadis@...lanox.com>
Cc:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        netdev@...r.kernel.org, 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

Fri, Aug 04, 2017 at 12:39:02AM CEST, arkadis@...lanox.com wrote:
>
>[...]
>
>>> 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).

I belive that option a) is the correct way to go. Introduction of self
inclusion was a mistake from the very beginning. I think that we should
just move one and correct this mistake.

Vivien, any arguments against a)?

Thanks!


>
>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
>
>
>
>