possible bridge regression in "bridge: implement [add/del]_slave ops"?

possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Alexander Stein]

Hello,

I tried using the rstpd daemon from 
http://git.kernel.org/?p=linux/kernel/git/shemminger/rstp.git;a=summary which 
worked fine on my desktop.
I then tried doing the same on my embedded atom baord and failed.
It turned out the used kernel 2.6.38-gentoo-r6 had not the problem the kernel 
v2.6.39 used on atomboard had. After trying a 2.6.39 based kernel on my 
desktop the same problem occured.
After bisecting I ended up at commit 
afc6151a78a43bdca5f64a8bd3e3c13837580c54
"bridge: implement [add/del]_slave ops"

My /sbin/bridge-stp is the one from commit 
b27ab0efa4ecf7a839f750ec1e9b9092c577ebbb in the rstpd repository.

My steps to reproduce:
* start rstpd (I used rstpd -d -v2 from a different terminal to see the error 
messages)
* echo $(pgrep rstpd) > /var/run/rstpd.pid
* brctl addbr br1
* echo 1 > /sys/class/net/br1/bridge/stp_state
* brctl addif br1 eth1
* brctl addif br1 eth2
* ifconfig br1 up 192.168.0.20
* ifconfig eth1 up
* ifconfig eth2 up

Then I get the following output from rstpd:
7: br1 
2011-06-30 09:08:01 create_if: Add bridge br1
2011-06-30 09:08:01 CTL_enable_bridge_rstp: bridge 7, enable 1
3: eth1 master br1 
2011-06-30 09:08:01 stp_enabled: STP on br1 state 2
2011-06-30 09:08:01 set_br_up: br1 was up stp was down
2011-06-30 09:08:01 set_br_up: Set bridge br1 up stp up
2011-06-30 09:08:01 create_if: Add iface eth1 to bridge br1
2: eth2 master br1 
2011-06-30 09:08:01 create_if: Add iface eth2 to bridge br1
7: br1 
2011-06-30 09:08:01 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up. 
2: eth2 master br1 
2011-06-30 09:08:03 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up. 
2011-06-30 09:08:03 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up. 
2011-06-30 09:08:05 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up. 
2011-06-30 09:08:05 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up.

The last error messages are printed for each BPDU received at the bridge.

Reverting the named commit on v2.6.39.2 restores the old behavior and I get a 
working RSTP again.

BTW: I noticed that in 2.6.39.2 independently from this patch revert this 
bridge didn't show up RUNNING ifconfg. Is this intended? Another bridge I 
have, which doesn't use (R)STP, is shown as RUNNING like before.

Regards,
Alexander

[Bridge] possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Alexander Stein]

Hello,

I tried using the rstpd daemon from 
http://git.kernel.org/?p=linux/kernel/git/shemminger/rstp.git;a=summary which 
worked fine on my desktop.
I then tried doing the same on my embedded atom baord and failed.
It turned out the used kernel 2.6.38-gentoo-r6 had not the problem the kernel 
v2.6.39 used on atomboard had. After trying a 2.6.39 based kernel on my 
desktop the same problem occured.
After bisecting I ended up at commit 
afc6151a78a43bdca5f64a8bd3e3c13837580c54
"bridge: implement [add/del]_slave ops"

My /sbin/bridge-stp is the one from commit 
b27ab0efa4ecf7a839f750ec1e9b9092c577ebbb in the rstpd repository.

My steps to reproduce:
* start rstpd (I used rstpd -d -v2 from a different terminal to see the error 
messages)
* echo $(pgrep rstpd) > /var/run/rstpd.pid
* brctl addbr br1
* echo 1 > /sys/class/net/br1/bridge/stp_state
* brctl addif br1 eth1
* brctl addif br1 eth2
* ifconfig br1 up 192.168.0.20
* ifconfig eth1 up
* ifconfig eth2 up

Then I get the following output from rstpd:
7: br1 
2011-06-30 09:08:01 create_if: Add bridge br1
2011-06-30 09:08:01 CTL_enable_bridge_rstp: bridge 7, enable 1
3: eth1 master br1 
2011-06-30 09:08:01 stp_enabled: STP on br1 state 2
2011-06-30 09:08:01 set_br_up: br1 was up stp was down
2011-06-30 09:08:01 set_br_up: Set bridge br1 up stp up
2011-06-30 09:08:01 create_if: Add iface eth1 to bridge br1
2: eth2 master br1 
2011-06-30 09:08:01 create_if: Add iface eth2 to bridge br1
7: br1 
2011-06-30 09:08:01 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up. 
2: eth2 master br1 
2011-06-30 09:08:03 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up. 
2011-06-30 09:08:03 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up. 
2011-06-30 09:08:05 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up. 
2011-06-30 09:08:05 Error in bridge_bpdu_rcv at bridge_track.c:585 verifying 
ifc->up.

The last error messages are printed for each BPDU received at the bridge.

Reverting the named commit on v2.6.39.2 restores the old behavior and I get a 
working RSTP again.

BTW: I noticed that in 2.6.39.2 independently from this patch revert this 
bridge didn't show up RUNNING ifconfg. Is this intended? Another bridge I 
have, which doesn't use (R)STP, is shown as RUNNING like before.

Regards,
Alexander

Re: possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Alexander Stein]

On Thursday 30 June 2011 10:33:23 Alexander Stein wrote:
> BTW: I noticed that in 2.6.39.2 independently from this patch revert this
> bridge didn't show up RUNNING ifconfg. Is this intended? Another bridge I
> have, which doesn't use (R)STP, is shown as RUNNING like before.

This change was caused by commit 1faa4356a3bd89ea11fb92752d897cff3a20ec0e 
"bridge: control carrier based on ports online". It prevents the bridge from 
actually receiving/sending packets. Reverting restores the old behavior.

Re: possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Stephen Hemminger]

On Thu, 30 Jun 2011 15:27:19 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> On Thursday 30 June 2011 10:33:23 Alexander Stein wrote:
> > BTW: I noticed that in 2.6.39.2 independently from this patch revert this
> > bridge didn't show up RUNNING ifconfg. Is this intended? Another bridge I
> > have, which doesn't use (R)STP, is shown as RUNNING like before.
> 
> This change was caused by commit 1faa4356a3bd89ea11fb92752d897cff3a20ec0e 
> "bridge: control carrier based on ports online". It prevents the bridge from 
> actually receiving/sending packets. Reverting restores the old behavior.

It is really a bug in RSTP, I will fix it there.

Re: [Bridge] possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Stephen Hemminger]

On Thu, 30 Jun 2011 15:27:19 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> On Thursday 30 June 2011 10:33:23 Alexander Stein wrote:
> > BTW: I noticed that in 2.6.39.2 independently from this patch revert this
> > bridge didn't show up RUNNING ifconfg. Is this intended? Another bridge I
> > have, which doesn't use (R)STP, is shown as RUNNING like before.
> 
> This change was caused by commit 1faa4356a3bd89ea11fb92752d897cff3a20ec0e 
> "bridge: control carrier based on ports online". It prevents the bridge from 
> actually receiving/sending packets. Reverting restores the old behavior.

It is really a bug in RSTP, I will fix it there.

Re: possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Stephen Hemminger]

On Thu, 30 Jun 2011 10:33:23 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> * echo $(pgrep rstpd) > /var/run/rstpd.pid
> * brctl addbr br1
> * echo 1 > /sys/class/net/br1/bridge/stp_state

This bogus. You are running both kernel and spanning
tree daemon at the same time!

Doing the echo of 1 to stp_state forces kernel spanning
tree. You want 2 which is what is supposed to be use for user
mode spanning tree.

Note: dropping LKML off the thread to save time/space/noise.

Re: [Bridge] possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Stephen Hemminger]

On Thu, 30 Jun 2011 10:33:23 +0200
Alexander Stein <alexander.stein@systec-electronic.com> wrote:

> * echo $(pgrep rstpd) > /var/run/rstpd.pid
> * brctl addbr br1
> * echo 1 > /sys/class/net/br1/bridge/stp_state

This bogus. You are running both kernel and spanning
tree daemon at the same time!

Doing the echo of 1 to stp_state forces kernel spanning
tree. You want 2 which is what is supposed to be use for user
mode spanning tree.

Note: dropping LKML off the thread to save time/space/noise.

Re: possible bridge regression in "bridge: implement [add/del]_slave ops"?

[David Lamparter]

On Thu, Jun 30, 2011 at 10:08:19AM -0700, Stephen Hemminger wrote:
> On Thu, 30 Jun 2011 10:33:23 +0200
> Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> 
> > * echo $(pgrep rstpd) > /var/run/rstpd.pid
> > * brctl addbr br1
> > * echo 1 > /sys/class/net/br1/bridge/stp_state
> 
> This bogus. You are running both kernel and spanning
> tree daemon at the same time!
> 
> Doing the echo of 1 to stp_state forces kernel spanning
> tree. You want 2 which is what is supposed to be use for user
> mode spanning tree.

I just tested this on my box, you can't echo 2 into that sysfs file (it
reads back as 1).

That you can change this variable at all when an userspace stp
implementation is running is a bug anyway, IMHO. rstpd should do all the
required settings and the kernel should prevent them from being changed
while rstpd has the bridge under its control.


-David

Re: [Bridge] possible bridge regression in "bridge: implement [add/del]_slave ops"?

[David Lamparter]

On Thu, Jun 30, 2011 at 10:08:19AM -0700, Stephen Hemminger wrote:
> On Thu, 30 Jun 2011 10:33:23 +0200
> Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> 
> > * echo $(pgrep rstpd) > /var/run/rstpd.pid
> > * brctl addbr br1
> > * echo 1 > /sys/class/net/br1/bridge/stp_state
> 
> This bogus. You are running both kernel and spanning
> tree daemon at the same time!
> 
> Doing the echo of 1 to stp_state forces kernel spanning
> tree. You want 2 which is what is supposed to be use for user
> mode spanning tree.

I just tested this on my box, you can't echo 2 into that sysfs file (it
reads back as 1).

That you can change this variable at all when an userspace stp
implementation is running is a bug anyway, IMHO. rstpd should do all the
required settings and the kernel should prevent them from being changed
while rstpd has the bridge under its control.


-David

Re: possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Alexander Stein]

On Thursday 30 June 2011 19:08:19 Stephen Hemminger wrote:
> On Thu, 30 Jun 2011 10:33:23 +0200
> 
> Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> > * echo $(pgrep rstpd) > /var/run/rstpd.pid
> > * brctl addbr br1
> > * echo 1 > /sys/class/net/br1/bridge/stp_state
> 
> This bogus. You are running both kernel and spanning
> tree daemon at the same time!
> 
> Doing the echo of 1 to stp_state forces kernel spanning
> tree. You want 2 which is what is supposed to be use for user
> mode spanning tree.

Well, at first sight you're right. But looking at the code at 
br_stp_set_enabled() in br_stp_if.c it doesn't matter which value you echo 
into stp_state, it just have to be non-zero
The detection of usermode oder kernelmode STP is done using the usermode 
helper (see br_stp_start). I tried echoing 1, 2 or even 5 into stp_state, with 
a working /sbin/bridge-stp I get 2 from stp_state each time.

Maybe it is valuable to change this odd behavior.

Regards,
Alexander

Re: [Bridge] possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Alexander Stein]

On Thursday 30 June 2011 19:08:19 Stephen Hemminger wrote:
> On Thu, 30 Jun 2011 10:33:23 +0200
> 
> Alexander Stein <alexander.stein@systec-electronic.com> wrote:
> > * echo $(pgrep rstpd) > /var/run/rstpd.pid
> > * brctl addbr br1
> > * echo 1 > /sys/class/net/br1/bridge/stp_state
> 
> This bogus. You are running both kernel and spanning
> tree daemon at the same time!
> 
> Doing the echo of 1 to stp_state forces kernel spanning
> tree. You want 2 which is what is supposed to be use for user
> mode spanning tree.

Well, at first sight you're right. But looking at the code at 
br_stp_set_enabled() in br_stp_if.c it doesn't matter which value you echo 
into stp_state, it just have to be non-zero
The detection of usermode oder kernelmode STP is done using the usermode 
helper (see br_stp_start). I tried echoing 1, 2 or even 5 into stp_state, with 
a working /sbin/bridge-stp I get 2 from stp_state each time.

Maybe it is valuable to change this odd behavior.

Regards,
Alexander

Re: possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Stephen Hemminger]

I just put a fix for the detection of bridge pseudo-device being up
into the current rstp code available at:
  git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/rstp.git

Re: [Bridge] possible bridge regression in "bridge: implement [add/del]_slave ops"?

[Stephen Hemminger]

I just put a fix for the detection of bridge pseudo-device being up
into the current rstp code available at:
  git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/rstp.git