linux-stable.git/net/bridge, branch v3.16.35

bridge: fix lockdep addr_list_lock false positive splat

2016-02-02T23:47:24+00:00

commit c6894dec8ea9ae05747124dce98b3b5c2e69b168 upstream.

After promisc mode management was introduced a bridge device could do
dev_set_promiscuity from its ndo_change_rx_flags() callback which in
turn can be called after the bridge's addr_list_lock has been taken
(e.g. by dev_uc_add). This causes a false positive lockdep splat because
the port interfaces' addr_list_lock is taken when br_manage_promisc()
runs after the bridge's addr list lock was already taken.
To remove the false positive introduce a custom bridge addr_list_lock
class and set it on bridge init.
A simple way to reproduce this is with the following:
$ brctl addbr br0
$ ip l add l br0 br0.100 type vlan id 100
$ ip l set br0 up
$ ip l set br0.100 up
$ echo 1 > /sys/class/net/br0/bridge/vlan_filtering
$ brctl addif br0 eth0
Splat:
[   43.684325] =============================================
[   43.684485] [ INFO: possible recursive locking detected ]
[   43.684636] 4.4.0-rc8+ #54 Not tainted
[   43.684755] ---------------------------------------------
[   43.684906] brctl/1187 is trying to acquire lock:
[   43.685047]  (_xmit_ETHER){+.....}, at: [] dev_set_rx_mode+0x1e/0x40
[   43.685460]  but task is already holding lock:
[   43.685618]  (_xmit_ETHER){+.....}, at: [] dev_uc_add+0x27/0x80
[   43.686015]  other info that might help us debug this:
[   43.686316]  Possible unsafe locking scenario:

[   43.686743]        CPU0
[   43.686967]        ----
[   43.687197]   lock(_xmit_ETHER);
[   43.687544]   lock(_xmit_ETHER);
[   43.687886] *** DEADLOCK ***

[   43.688438]  May be due to missing lock nesting notation

[   43.688882] 2 locks held by brctl/1187:
[   43.689134]  #0:  (rtnl_mutex){+.+.+.}, at: [] rtnl_lock+0x17/0x20
[   43.689852]  #1:  (_xmit_ETHER){+.....}, at: [] dev_uc_add+0x27/0x80
[   43.690575] stack backtrace:
[   43.690970] CPU: 0 PID: 1187 Comm: brctl Not tainted 4.4.0-rc8+ #54
[   43.691270] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
[   43.691770]  ffffffff826a25c0 ffff8800369fb8e0 ffffffff81360ceb ffffffff826a25c0
[   43.692425]  ffff8800369fb9b8 ffffffff810d0466 ffff8800369fb968 ffffffff81537139
[   43.693071]  ffff88003a08c880 0000000000000000 00000000ffffffff 0000000002080020
[   43.693709] Call Trace:
[   43.693931]  [] dump_stack+0x4b/0x70
[   43.694199]  [] __lock_acquire+0x1e46/0x1e90
[   43.694483]  [] ? netlink_broadcast_filtered+0x139/0x3e0
[   43.694789]  [] ? nlmsg_notify+0x5a/0xc0
[   43.695064]  [] lock_acquire+0xe5/0x1f0
[   43.695340]  [] ? dev_set_rx_mode+0x1e/0x40
[   43.695623]  [] _raw_spin_lock_bh+0x45/0x80
[   43.695901]  [] ? dev_set_rx_mode+0x1e/0x40
[   43.696180]  [] dev_set_rx_mode+0x1e/0x40
[   43.696460]  [] dev_set_promiscuity+0x3c/0x50
[   43.696750]  [] br_port_set_promisc+0x25/0x50 [bridge]
[   43.697052]  [] br_manage_promisc+0x8a/0xe0 [bridge]
[   43.697348]  [] br_dev_change_rx_flags+0x1e/0x20 [bridge]
[   43.697655]  [] __dev_set_promiscuity+0x132/0x1f0
[   43.697943]  [] __dev_set_rx_mode+0x82/0x90
[   43.698223]  [] dev_uc_add+0x5e/0x80
[   43.698498]  [] vlan_device_event+0x542/0x650 [8021q]
[   43.698798]  [] notifier_call_chain+0x5d/0x80
[   43.699083]  [] raw_notifier_call_chain+0x16/0x20
[   43.699374]  [] call_netdevice_notifiers_info+0x6e/0x80
[   43.699678]  [] call_netdevice_notifiers+0x16/0x20
[   43.699973]  [] br_add_if+0x47e/0x4c0 [bridge]
[   43.700259]  [] add_del_if+0x6e/0x80 [bridge]
[   43.700548]  [] br_dev_ioctl+0xaf/0xc0 [bridge]
[   43.700836]  [] dev_ifsioc+0x30c/0x3c0
[   43.701106]  [] dev_ioctl+0xf9/0x6f0
[   43.701379]  [] ? mntput_no_expire+0x5/0x450
[   43.701665]  [] ? mntput_no_expire+0xae/0x450
[   43.701947]  [] sock_do_ioctl+0x42/0x50
[   43.702219]  [] sock_ioctl+0x1e5/0x290
[   43.702500]  [] do_vfs_ioctl+0x2cb/0x5c0
[   43.702771]  [] SyS_ioctl+0x79/0x90
[   43.703033]  [] entry_SYSCALL_64_fastpath+0x16/0x7a

CC: Vlad Yasevich 
CC: Stephen Hemminger 
CC: Bridge list 
CC: Andy Gospodarek 
CC: Roopa Prabhu 
Fixes: 2796d0c648c9 ("bridge: Automatically manage port promiscuous mode.")
Reported-by: Andy Gospodarek 
Signed-off-by: Nikolay Aleksandrov 
Signed-off-by: David S. Miller 
[ luis: backported to 3.16: adjusted context ]
Signed-off-by: Luis Henriques

bridge: Only call /sbin/bridge-stp for the initial network namespace

2016-01-28T10:27:12+00:00

commit ff62198553e43cdffa9d539f6165d3e83f8a42bc upstream.

[I stole this patch from Eric Biederman. He wrote:]

> There is no defined mechanism to pass network namespace information
> into /sbin/bridge-stp therefore don't even try to invoke it except
> for bridge devices in the initial network namespace.
>
> It is possible for unprivileged users to cause /sbin/bridge-stp to be
> invoked for any network device name which if /sbin/bridge-stp does not
> guard against unreasonable arguments or being invoked twice on the
> same network device could cause problems.

[Hannes: changed patch using netns_eq]

Cc: Eric W. Biederman 
Signed-off-by: Eric W. Biederman 
Signed-off-by: Hannes Frederic Sowa 
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques

bridge: mdb: fix delmdb state in the notification

2015-08-25T09:13:53+00:00

commit 7ae90a4f96486e3e20274afa1b8329802f5e1981 upstream.

Since mdb states were introduced when deleting an entry the state was
left as it was set in the delete request from the user which leads to
the following output when doing a monitor (for example):
$ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent
$ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 temp
^^^
Note the "temp" state in the delete notification which is wrong since
the entry was permanent, the state in a delete is always reported as
"temp" regardless of the real state of the entry.

After this patch:
$ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent
$ bridge mdb del dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent

There's one important note to make here that the state is actually not
matched when doing a delete, so one can delete a permanent entry by
stating "temp" in the end of the command, I've chosen this fix in order
not to break user-space tools which rely on this (incorrect) behaviour.

So to give an example after this patch and using the wrong state:
$ bridge mdb add dev br0 port eth3 grp 239.0.0.1 permanent
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent
$ bridge mdb del dev br0 port eth3 grp 239.0.0.1 temp
(monitor) dev br0 port eth3 grp 239.0.0.1 permanent

Note the state of the entry that got deleted is correct in the
notification.

Signed-off-by: Nikolay Aleksandrov 
Fixes: ccb1c31a7a87 ("bridge: add flags to distinguish permanent mdb entires")
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques

bridge: mdb: fix double add notification

2015-08-12T08:48:29+00:00

commit 5ebc784625ea68a9570d1f70557e7932988cd1b4 upstream.

Since the mdb add/del code was introduced there have been 2 br_mdb_notify
calls when doing br_mdb_add() resulting in 2 notifications on each add.

Example:
 Command: bridge mdb add dev br0 port eth1 grp 239.0.0.1 permanent
 Before patch:
 root@debian:~# bridge monitor all
 [MDB]dev br0 port eth1 grp 239.0.0.1 permanent
 [MDB]dev br0 port eth1 grp 239.0.0.1 permanent

 After patch:
 root@debian:~# bridge monitor all
 [MDB]dev br0 port eth1 grp 239.0.0.1 permanent

Signed-off-by: Nikolay Aleksandrov 
Fixes: cfd567543590 ("bridge: add support of adding and deleting mdb entries")
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques

bridge: mdb: zero out the local br_ip variable before use

2015-08-10T16:10:18+00:00

commit f1158b74e54f2e2462ba5e2f45a118246d9d5b43 upstream.

Since commit b0e9a30dd669 ("bridge: Add vlan id to multicast groups")
there's a check in br_ip_equal() for a matching vlan id, but the mdb
functions were not modified to use (or at least zero it) so when an
entry was added it would have a garbage vlan id (from the local br_ip
variable in __br_mdb_add/del) and this would prevent it from being
matched and also deleted. So zero out the whole local ip var to protect
ourselves from future changes and also to fix the current bug, since
there's no vlan id support in the mdb uapi - use always vlan id 0.
Example before patch:
root@debian:~# bridge mdb add dev br0 port eth1 grp 239.0.0.1 permanent
root@debian:~# bridge mdb
dev br0 port eth1 grp 239.0.0.1 permanent
root@debian:~# bridge mdb del dev br0 port eth1 grp 239.0.0.1 permanent
RTNETLINK answers: Invalid argument

After patch:
root@debian:~# bridge mdb add dev br0 port eth1 grp 239.0.0.1 permanent
root@debian:~# bridge mdb
dev br0 port eth1 grp 239.0.0.1 permanent
root@debian:~# bridge mdb del dev br0 port eth1 grp 239.0.0.1 permanent
root@debian:~# bridge mdb

Signed-off-by: Nikolay Aleksandrov 
Fixes: b0e9a30dd669 ("bridge: Add vlan id to multicast groups")
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques

bridge: mdb: start delete timer for temp static entries

2015-08-10T16:10:16+00:00

commit f7e2965db17dd3b60f05fad88e7afc79ea75b48f upstream.

Start the delete timer when adding temp static entries so they can expire.

Signed-off-by: Satish Ashok 
Signed-off-by: Nikolay Aleksandrov 
Fixes: ccb1c31a7a87 ("bridge: add flags to distinguish permanent mdb entires")
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques

bridge: multicast: restore router configuration on port link down/up

2015-07-15T09:01:29+00:00

commit 754bc547f0a79f7568b5b81c7fc0a8d044a6571a upstream.

When a port goes through a link down/up the multicast router configuration
is not restored.

Signed-off-by: Satish Ashok 
Signed-off-by: Nikolay Aleksandrov 
Fixes: 0909e11758bd ("bridge: Add multicast_router sysfs entries")
Acked-by: Herbert Xu 
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques

bridge: fix br_stp_set_bridge_priority race conditions

2015-07-06T09:24:35+00:00

commit 2dab80a8b486f02222a69daca6859519e05781d9 upstream.

After the ->set() spinlocks were removed br_stp_set_bridge_priority
was left running without any protection when used via sysfs. It can
race with port add/del and could result in use-after-free cases and
corrupted lists. Tested by running port add/del in a loop with stp
enabled while setting priority in a loop, crashes are easily
reproducible.
The spinlocks around sysfs ->set() were removed in commit:
14f98f258f19 ("bridge: range check STP parameters")
There's also a race condition in the netlink priority support that is
fixed by this change, but it was introduced recently and the fixes tag
covers it, just in case it's needed the commit is:
af615762e972 ("bridge: add ageing_time, stp_state, priority over netlink")

Signed-off-by: Nikolay Aleksandrov 
Fixes: 14f98f258f19 ("bridge: range check STP parameters")
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques

bridge: fix multicast router rlist endless loop

2015-07-06T09:16:09+00:00

commit 1a040eaca1a22f8da8285ceda6b5e4a2cb704867 upstream.

Since the addition of sysfs multicast router support if one set
multicast_router to "2" more than once, then the port would be added to
the hlist every time and could end up linking to itself and thus causing an
endless loop for rlist walkers.
So to reproduce just do:
echo 2 > multicast_router; echo 2 > multicast_router;
in a bridge port and let some igmp traffic flow, for me it hangs up
in br_multicast_flood().
Fix this by adding a check in br_multicast_add_router() if the port is
already linked.
The reason this didn't happen before the addition of multicast_router
sysfs entries is because there's a !hlist_unhashed check that prevents
it.

Signed-off-by: Nikolay Aleksandrov 
Fixes: 0909e11758bd ("bridge: Add multicast_router sysfs entries")
Acked-by: Herbert Xu 
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques

bridge: disable softirqs around br_fdb_update to avoid lockup

2015-06-12T12:40:05+00:00

commit c4c832f89dc468cf11dc0dd17206bace44526651 upstream.

br_fdb_update() can be called in process context in the following way:
br_fdb_add() -> __br_fdb_add() -> br_fdb_update() (if NTF_USE flag is set)
so we need to disable softirqs because there are softirq users of the
hash_lock. One easy way to reproduce this is to modify the bridge utility
to set NTF_USE, enable stp and then set maxageing to a low value so
br_fdb_cleanup() is called frequently and then just add new entries in
a loop. This happens because br_fdb_cleanup() is called from timer/softirq
context. The spin locks in br_fdb_update were _bh before commit f8ae737deea1
("[BRIDGE]: forwarding remove unneeded preempt and bh diasables")
and at the time that commit was correct because br_fdb_update() couldn't be
called from process context, but that changed after commit:
292d1398983f ("bridge: add NTF_USE support")
Using local_bh_disable/enable around br_fdb_update() allows us to keep
using the spin_lock/unlock in br_fdb_update for the fast-path.

Signed-off-by: Nikolay Aleksandrov 
Fixes: 292d1398983f ("bridge: add NTF_USE support")
Signed-off-by: David S. Miller 
Signed-off-by: Luis Henriques