From patchwork Thu Nov 15 19:52:00 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lennert Buytenhek X-Patchwork-Id: 199393 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id AC49B2C04C6 for ; Fri, 16 Nov 2012 06:52:08 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1768600Ab2KOTwG (ORCPT ); Thu, 15 Nov 2012 14:52:06 -0500 Received: from fw.wantstofly.org ([80.101.37.227]:61706 "EHLO mail.wantstofly.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1767627Ab2KOTwF (ORCPT ); Thu, 15 Nov 2012 14:52:05 -0500 Received: by mail.wantstofly.org (Postfix, from userid 500) id 22FC7214F0; Thu, 15 Nov 2012 20:52:00 +0100 (CET) Date: Thu, 15 Nov 2012 20:52:00 +0100 From: Lennert Buytenhek To: Stephen Hemminger , netdev@vger.kernel.org Cc: Steven Kath , Anatoly Kaplan , Arthur Xiong , Chris Healy Subject: stp issue and "bridge: send proper message_age in config BPDU" Message-ID: <20121115195200.GD730@wantstofly.org> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi! 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 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): thanks, Lennert --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- 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);