netdev - Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

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: <CAE4R7bBL+=-akoNLPqPae=zrYe3DH59t=8UK_T7KhcHSNaKUtw@mail.gmail.com>
Date:	Tue, 2 Jun 2015 00:10:12 -0700
From:	Scott Feldman <sfeldma@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	Toshiaki Makita <makita.toshiaki@....ntt.co.jp>,
	Netdev <netdev@...r.kernel.org>,
	Jiří Pírko <jiri@...nulli.us>,
	"simon.horman@...ronome.com" <simon.horman@...ronome.com>
Subject: Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

On Mon, Jun 1, 2015 at 10:24 PM, David Miller <davem@...emloft.net> wrote:
> From: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
> Date: Tue, 02 Jun 2015 13:51:06 +0900
>
>> On 2015/06/02 3:39, sfeldma@...il.com wrote:
>>> From: Scott Feldman <sfeldma@...il.com>
>>>
>>> Remove support for legacy ndo ops
>>> .ndo_vlan_rx_add_vid/.ndo_vlan_rx_kill_vid.  Rocker will use
>>> bridge_setlink/dellink exclusively for VLAN add/del operations.
>>>
>>> The legacy ops are needed if using 8021q driver module to setup VLANs on
>>> the port.  But an alternative exists in using bridge_setlink/delink to
>>> setup VLANs, which doesn't depend on 8021q module.  So rocker will switch
>>> to the newer setlink/dellink ops.  VLANs can added/delete from the port,
>>> regardless if port is bridged or not, using the bridge commands:
>>>
>>>      bridge vlan [add|del] vid VID dev DEV self
>>
>> Hi Scott,
>>
>> This doesn't look transparent with bridge.
>>
>> Before this patch, I was able to add vid in the same way as software bridge:
>>
>>   ip link set DEV master br0
>>   bridge vlan add vid VID dev DEV
>>
>> Now I need to add "self", which is different from software bridge...
>
> I'm already not liking the looks of this....

Actually, we're now consistent with bridge man page which says master
is the default.

Want we want, I believe, is to adjust what the man page says (and the
bridge vlan command itself), by making the default master and self.
The kernel and driver are fine, it's the default in the bridge command
that needs adjusting.  Once we do this, we'll be back to transparent
with software-only bridge.

How did we get here?   So the RTM_SETLINK for PF_BRIDGE calls
rtnl_bridge_setlink().  rtnl_bridge_setlink() calls ndo_bridge_setlink
for the master (the bridge side of the port) and self (the device side
of the port), depending on if MASTER and/or SELF flags are set.  Since
the default from the iproute2 bridge vlan cmd is to only set MASTER,
only the bridge's ndo_bridge_setlink is called.  But if you dig down
into the bridge's ndo_bridge_setlink, you'll see it will call into the
port driver's ndo_vlan_rx_add_vid() to add the vlan to the device side
of the port.  So we have a MASTER cmd that is doing some SELF work.
My guess this was done to avoid having to update all the NIC drivers
from  ndo_vlan_rx_add_vid to ndo_bridge_setlink.  When you remove
ndo_vlan_rx_add_vid() from the port driver, the cmd needs to target
MASTER and SELF for both sides of the port to be called.  But the
current cmd only sets MASTER.  This is why you (currently) need to add
SELF for cmd to target the device side of the port.

On top of all of this, you can use RTM_SETLINK for PF_BRIDGE on
non-bridged ports, in which case only SELF is used to program the VLAN
on the device, using the device's ndo_bridge_setlink.  This is the
confusing part where you can set VLANs on non-bridged ports using the
bridge cmd.

To summarize, pseudo code for rtnl_bridge_setlink() is:

    rtnl_bridge_setlink()
        if MASTER
            call bridge's ndo_bridge_setlink()
                if bridge port implements ndo_vlan_rx_add_vid()
                    call ndo_vlan_rx_add_vid() on port device to set vlan
        if SELF
            call port device's ndo_bridge_setlink()

If DEV is bridged, today we have:

    bridge vlan add vid VID dev DEV                              sets
MASTER (default)
    bridge vlan add vid VID dev DEV master                   sets MASTER
    bridge vlan add vid VID dev DEV self                        sets SELF
    bridge vlan add vid VID dev DEV master self             sets MASTER and SELF

if DEV is not bridged, today we have:

    bridge vlan add vid VID dev DEV                              //
fails  (no master device)
    bridge vlan add vid VID dev DEV self                        sets SELF

What I propose is we change the bridge vlan cmd for the DEV bridged
case as such:

    bridge vlan add vid VID dev DEV                              sets
MASTER and SELF (default)
    bridge vlan add vid VID dev DEV master                   sets MASTER
    bridge vlan add vid VID dev DEV self                        sets SELF
    bridge vlan add vid VID dev DEV master self             sets MASTER and SELF

For existing users of ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid,
nothing really changes.  If they also have an ndo_bridge_setlink,
it'll get called but they're not doing any vlan stuff there today
anyway, so it's ignored.

For rocker, we're switching to doing all vlan stuff in
ndo_bridge_setlink.  Switching to ndo_bridge_setlink for switchdev
gives us support for stacked drivers with the transaction model,
something we don't get with ndo_vlan_rx_add_vid.

If this makes sense, I'll post the follow up bridge vlan cmd change to
default to master and self.

-scott
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html