netdev - Re: stp issue and "bridge: send proper message

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]
Date:	Thu, 15 Nov 2012 21:09:30 +0100
From:	Lennert Buytenhek <buytenh@...tstofly.org>
To:	Stephen Hemminger <shemminger@...tta.com>
Cc:	netdev@...r.kernel.org, Steven Kath <steven.kath@...tta.com>,
	Chris Healy <cphealy@...il.com>
Subject: Re: stp issue and "bridge: send proper message_age in config BPDU"

On Thu, Nov 15, 2012 at 11:58:53AM -0800, Stephen Hemminger wrote:

> > FWIW, I've been debugging an STP issue on an old product kernel tree
> > that I couldn't find an upstream fix for, but after having debugged the
> > issue, there does actually appear to be an upstream commit that makes
> > the issue go away, but the commit message on that commit is somewhat
> > unclear about what the issue is that it's fixing and why the given fix
> > fixes it, and given that I spent considerable time debugging it I
> > figured I'd send this out for the sake of the next person googling for
> > this.
> > 
> > The symptoms are pretty much what's described in this bug:
> > 
> > 	https://bugzilla.vyatta.com/show_bug.cgi?id=7164
> > 
> > And the upstream commit is:
> > 
> > 	https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=0c03150e7ea8f7fcd03cfef29385e0010b22ee92
> > 
> > 	commit 0c03150e7ea8f7fcd03cfef29385e0010b22ee92
> > 	Author: stephen hemminger <shemminger@...tta.com>
> > 	Date:   Fri Jul 22 07:47:06 2011 +0000
> > 
> > 	    bridge: send proper message_age in config BPDU
> > 
> > What I was seeing was that as a non-root bridge, Linux STP would often
> > fail to transmit BPDUs out of designated ports upon reception of a BPDU
> > from an upstream port.
> > 
> > br_received_config_bpdu() handles the received BPDU, and calls into
> > br_record_config_information(), which resets the message age timer on
> > this port to jiffies + (p->br->max_age - bpdu->message_age);
> > 
> > When br_received_config_bpdu() then calls br_config_bpdu_generation(),
> > the latter will call into br_transmit_config() for each enabled
> > designated port, which will send out BPDUs with age br->max_age
> > - (root->message_age_timer.expires - jiffies) + MESSAGE_AGE_INCR if
> > we're not the root bridge, which if you plug in the previously
> > computed timeout simplifies to bpdu->message_age + MESSAGE_AGE_INCR,
> > which is exactly what we want it to be and this computation isn't
> > wrong per se.
> > 
> > The problem with the above logic, though, is that it fails to
> > consider that mod_timer() can round up the timeout you give it (i.e.
> > add timer slack), and that reading back root->message_age_timer.expires
> > in br_transmit_config() won't necessarily return the value that was
> > plugged into mod_timer() for this timer in br_record_config_information().
> > 
> > E.g. if mod_timer() decides to add 5 jiffies to the timeout, the message
> > age value that br_transmit_config() will compute will be:
> > 
> > 	br->max_age - (root->message_age_timer.expires - jiffies) +
> > 		MESSAGE_AGE_INCR
> > 
> > 	= br->max_age - (jiffies + (p->br->max_age - bpdu->message_age) + 5
> > 		- jiffies) + MESSAGE_AGE_INCR
> > 
> > 	= br->max_age - (p->br->max_age - bpdu->message_age + 5) +
> > 		MESSAGE_AGE_INCR
> > 
> > 	= bpdu->message_age - 5 + MESSAGE_AGE_INCR
> > 
> > Which will likely make the computed message age value negative.
> > This message age is stored in a signed int, but is then compared
> > against the bridge max age time:
> > 
> > 	if (bpdu.message_age < br->max_age) {
> > 
> > and br->max_age is an unsigned long, causing the comparison to be
> > unsigned and always fail if the computed message age was negative,
> > and no BPDU to be sent (causing our downstream neighbours to time
> > us out after some time and etc).
> > 
> > Commit 0c03150e7ea fixes the issue because it avoids reading back the
> > expiration time (possibly with timer slack included) of a previously
> > set timer.  Disabling timer slack on the message age timer achieves
> > the same thing (and is what I did initially):
> > 
> > --- a/net/bridge/br_stp_timer.c
> > +++ b/net/bridge/br_stp_timer.c
> > @@ -158,6 +158,7 @@ void br_stp_port_timer_init(struct net_bridge_port *p)
> >  {
> >         setup_timer(&p->message_age_timer, br_message_age_timer_expired,
> >                       (unsigned long) p);
> > +       set_timer_slack(&p->message_age_timer, 0);
> >  
> >         setup_timer(&p->forward_delay_timer, br_forward_delay_timer_expired,
> >                       (unsigned long) p);
> 
> Disabling timer slack causes additional power consumption because
> the tick wakeup has to be immediate. I prefer to handle late timer
> in the code. 

ACK, I wasn't advocating that we do this instead.


> P.s: not sure if timer slack existed back when I first saw the problem.

Timer slack was introduced in March 2010, and first appeared in
2.6.34, and this bug was reported against Vyatta 6.2, which has 2.6.35.
(I ran into it on 2.6.35.3.)

In fact, timer slack is the only reason why this issue triggers in
the first place.  Without timer slack, BPDU generation works fine on
either version of the STP code.
--
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