netdev - Re: Port STP state after removing port from bridge

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] [day] [month] [year] [list]

Message-ID: <CAE4R7bAxar0+fOMjN5=hPUUM+JPbZtpeo1-9O_RUddJy4SHGbA@mail.gmail.com>
Date:	Sat, 21 Feb 2015 18:49:15 -0800
From:	Scott Feldman <sfeldma@...il.com>
To:	Stephen Hemminger <stephen@...workplumber.org>
Cc:	Florian Fainelli <f.fainelli@...il.com>,
	Jiri Pirko <jiri@...nulli.us>, netdev <netdev@...r.kernel.org>
Subject: Re: Port STP state after removing port from bridge

On Sat, Feb 21, 2015 at 2:58 PM, Stephen Hemminger
<stephen@...workplumber.org> wrote:
> On Fri, 20 Feb 2015 09:04:18 -0800
> Florian Fainelli <f.fainelli@...il.com> wrote:
>
>> On 20/02/15 07:03, Scott Feldman wrote:
>> > On Fri, Feb 20, 2015 at 2:00 AM, Jiri Pirko <jiri@...nulli.us> wrote:
>> >> Fri, Feb 20, 2015 at 05:45:01AM CET, sfeldma@...il.com wrote:
>> >>> On Wed, Feb 18, 2015 at 11:39 PM, Florian Fainelli <f.fainelli@...il.com>
>> >>> wrote:
>> >>>
>> >>>> Hi,
>> >>>>
>> >>>> It just occured to me that the following sequence:
>> >>>>
>> >>>> brctl addbr br0
>> >>>> brctl addif br0 port0
>> >>>> ... STP happens
>> >>>> brctl delif br0 port0
>> >>>>
>> >>>> will leave port0 in STP disabled state, because the bridge code will
>> >>>> set the STP state to DISABLED, and only a down/up sequence can bring
>> >>>> it back to FORWARDING.
>> >>>>
>> >>>> Is this something that we should somehow fix? As an user it seems a
>> >>>> little convoluted having to do a down/up sequence to restore things. I
>> >>>> believe however that it is valid for the bridge layer to mark a port
>> >>>> as DISABLED when removing it. This is typically not noticed or even
>> >>>> remotely a problem with software bridges because we cannot enforce an
>> >>>> actual STP state at the HW level.
>> >>>>
>> >>>> Let me know your thoughts.
>> >>>>
>> >>>>
>> >>> The fix in rocker would be:
>> >>>
>> >>> diff --git a/drivers/net/ethernet/rocker/rocker.c
>> >>> b/drivers/net/ethernet/rocker/rocker.c
>> >>> index 34389b6a..e2004fb 100644
>> >>> --- a/drivers/net/ethernet/rocker/rocker.c
>> >>> +++ b/drivers/net/ethernet/rocker/rocker.c
>> >>> @@ -4456,8 +4456,10 @@ static int rocker_port_bridge_leave(struct
>> >>> rocker_port *rocker_port)
>> >>>                rocker_port_internal_vlan_id_get(rocker_port,
>> >>>                                                 rocker_port->dev->ifindex);
>> >>>        err = rocker_port_vlan(rocker_port, 0, 0);
>> >>> +       if (err)
>> >>> +               return err;
>> >>>
>> >>> -       return err;
>> >>> +       return rocker_port_stp_update(rocker_port, BR_STATE_FORWARDING);
>> >>> }
>> >>>
>> >>>
>> >>> This will return the port back to it's initial state of
>> >>> BR_STATE_FORWARDING, after it's removed from the bridge.
>> >>>
>> >>> I'll include this patch in the rocker pile to be pushed later.
>> >>>
>> >>> -scott
>> >>
>> >>
>> >> I'm not sure, but wouldn't it be nicer it the bridge code would set
>> >> state to disabled before the port is removed from the bridge?
>> >
>> > When the port is removed from a bridge, for example with brctl delif,
>> > the bridge driver puts port in BR_STATE_DISABLED and then sends
>> > netdevice event NETDEV_CHANGEUPPER.  In response to
>> > NETDEV_CHANGEUPPER, the rocker driver is returning port back to
>> > BR_STATE_FORWARDING (the initial state for an un-bridged port).  So
>> > this preserves bridge behavior for non-switchdev uses.  Does this
>> > answer the question, or did I miss understand your question?
>>
>> I think what we want is a solution at the bridge level, we have rocker
>> now updating the STP state to BR_STATE_FORWARDING when a given
>> rocker_port leaves a bridge, and I also had a similar change in DSA.
>>
>> Something like this maybe (untested):
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index b087d278c679..d693a2a10b3c 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -242,6 +242,8 @@ static void del_nbp(struct net_bridge_port *p)
>>
>>         spin_lock_bh(&br->lock);
>>         br_stp_disable_port(p);
>> +       if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD)
>> +               br_set_state(p, BR_STATE_FORWARDING);
>>         spin_unlock_bh(&br->lock);
>
> When port is deleted from bridge, it is no longer part of the
> bridge, so as far as it is concerned there should be no STP state!

Your point is valid.  It seems we're back to the port driver doing
what it needs to do so the port can pass traffic once it leaves the
bridge.  It looks like this needs to be port-driver specific.

-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