netdev - Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

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]

Date:	Sat, 15 Mar 2014 02:39:40 +0100
From:	"Luis R. Rodriguez" <mcgrof@...e.com>
To:	Cong Wang <cwang@...pensource.com>
Cc:	"Luis R. Rodriguez" <mcgrof@...not-panic.com>,
	netdev <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	kvm@...r.kernel.org, xen-devel@...ts.xenproject.org,
	Stephen Hemminger <stephen@...workplumber.org>,
	bridge@...ts.linux-foundation.org
Subject: Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port
	changes

On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
> <mcgrof@...not-panic.com> wrote:
> >                         spin_lock_bh(&p->br->lock);
> >                         err = br_setport(p, tb);
> > +                       changed = br_stp_recalculate_bridge_id(p->br);
> 
> Looks like you only want to check if the mac addr gets changed here,

Nope, the reason why we want a full thorough check is that br_setport()
may change currently any of these:
                                                                                
  * IFLA_BRPORT_MODE
  * IFLA_BRPORT_GUARD
  * IFLA_BRPORT_FAST_LEAVE
  * IFLA_BRPORT_PROTECT
  * IFLA_BRPORT_LEARNING,
  * IFLA_BRPORT_UNICAST_FLOOD
  * IFLA_BRPORT_COST
  * IFLA_BRPORT_PRIORITY
  * IFLA_BRPORT_STATE

That's good reason to trigger a good inspection. Having the MAC address
change would be simply collateral and its just something we need to do
some additional work for outside of the locking context.

> but br_stp_recalculate_bridge_id() does more than just checking it,
> are you sure the side-effects are all what you want here?

Yeap.

> >                         spin_unlock_bh(&p->br->lock);
> > +                       if (changed)
> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> > +                                                        p->br->dev);
> > +                       netdev_update_features(p->br->dev);
> 
> I think this is supposed to be in netdev event handler of br->dev
> instead of here.

Do you mean netdev_update_features() ? I mimic'd what was being done on
br_del_if() given that root blocking is doing something similar. If
we need to change something for the above then I suppose it means we need
to change br_del_if() too. Let me know if you see any reason for something
else.

  Luis

Content of type "application/pgp-signature" skipped