Commit ce1e3302 authored by Jakub Kicinski's avatar Jakub Kicinski
Browse files

Merge branch 'bridge-fix-sleep-in-atomic-context' 

Ido Schimmel says:

====================
bridge: Fix sleep in atomic context

Under certain circumstances the bridge driver can call
dev_set_promiscuity() while holding the bridge spin lock. This is a
problem as dev_set_promiscuity() might sleep.

Patches #1-#2 fix the problem in the netlink and sysfs configuration
paths by only taking the lock where it is actually needed, thereby
avoiding calling dev_set_promiscuity() from an atomic context.

Patch #3 adds test cases for both configuration paths in rtnetlink.sh
which already includes test cases for similar issues.

Note that dev_set_promiscuity() can sleep either when it takes the net
device mutex or when calling netif_rx_mode_sync(). I encountered the
problem with the latter, but blamed the former since it came earlier.
====================

Link: https://patch.msgid.link/20260526064818.272516-1-idosch@nvidia.com


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parents 8ba68464 147f3b1f

net/bridge/br_netlink.c

+7 −10
Original line number Diff line number Diff line
@@ -1000,19 +1000,25 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[], 
	br_port_flags_change(p, changed_mask);



	if (tb[IFLA_BRPORT_COST]) {

		spin_lock_bh(&p->br->lock);

		err = br_stp_set_path_cost(p, nla_get_u32(tb[IFLA_BRPORT_COST]));

		spin_unlock_bh(&p->br->lock);

		if (err)

			return err;

	}



	if (tb[IFLA_BRPORT_PRIORITY]) {

		spin_lock_bh(&p->br->lock);

		err = br_stp_set_port_priority(p, nla_get_u16(tb[IFLA_BRPORT_PRIORITY]));

		spin_unlock_bh(&p->br->lock);

		if (err)

			return err;

	}



	if (tb[IFLA_BRPORT_STATE]) {

		spin_lock_bh(&p->br->lock);

		err = br_set_port_state(p, nla_get_u8(tb[IFLA_BRPORT_STATE]));

		spin_unlock_bh(&p->br->lock);

		if (err)

			return err;

	}
@@ -1114,9 +1120,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags, 
			if (err)

				return err;



			spin_lock_bh(&p->br->lock);

			err = br_setport(p, tb, extack);

			spin_unlock_bh(&p->br->lock);

		} else {

			/* Binary compatibility with old RSTP */

			if (nla_len(protinfo) < sizeof(u8))
@@ -1203,17 +1207,10 @@ static int br_port_slave_changelink(struct net_device *brdev, 
				    struct nlattr *data[],

				    struct netlink_ext_ack *extack)

{

	struct net_bridge *br = netdev_priv(brdev);

	int ret;



	if (!data)

		return 0;



	spin_lock_bh(&br->lock);

	ret = br_setport(br_port_get_rtnl(dev), data, extack);

	spin_unlock_bh(&br->lock);



	return ret;

	return br_setport(br_port_get_rtnl(dev), data, extack);

}



static int br_port_fill_slave_info(struct sk_buff *skb,

net/bridge/br_switchdev.c

+0 −1
Original line number Diff line number Diff line
@@ -99,7 +99,6 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, 
	attr.u.brport_flags.val = flags;

	attr.u.brport_flags.mask = mask;



	/* We run from atomic context here */

	err = call_switchdev_notifiers(SWITCHDEV_PORT_ATTR_SET, p->dev,

				       &info.info, extack);

	err = notifier_to_errno(err);

net/bridge/br_sysfs_if.c

+22 −8
Original line number Diff line number Diff line
@@ -86,16 +86,34 @@ static ssize_t show_path_cost(struct net_bridge_port *p, char *buf) 
	return sysfs_emit(buf, "%d\n", p->path_cost);

}



static BRPORT_ATTR(path_cost, 0644,

		   show_path_cost, br_stp_set_path_cost);

static int store_path_cost(struct net_bridge_port *p, unsigned long v)

{

	int ret;



	spin_lock_bh(&p->br->lock);

	ret = br_stp_set_path_cost(p, v);

	spin_unlock_bh(&p->br->lock);

	return ret;

}



static BRPORT_ATTR(path_cost, 0644, show_path_cost, store_path_cost);



static ssize_t show_priority(struct net_bridge_port *p, char *buf)

{

	return sysfs_emit(buf, "%d\n", p->priority);

}



static BRPORT_ATTR(priority, 0644,

			 show_priority, br_stp_set_port_priority);

static int store_priority(struct net_bridge_port *p, unsigned long v)

{

	int ret;



	spin_lock_bh(&p->br->lock);

	ret = br_stp_set_port_priority(p, v);

	spin_unlock_bh(&p->br->lock);

	return ret;

}



static BRPORT_ATTR(priority, 0644, show_priority, store_priority);



static ssize_t show_designated_root(struct net_bridge_port *p, char *buf)

{
@@ -334,17 +352,13 @@ static ssize_t brport_store(struct kobject *kobj, 
			ret = -ENOMEM;

			goto out_unlock;

		}

		spin_lock_bh(&p->br->lock);

		ret = brport_attr->store_raw(p, buf_copy);

		spin_unlock_bh(&p->br->lock);

		kfree(buf_copy);

	} else if (brport_attr->store) {

		val = simple_strtoul(buf, &endp, 0);

		if (endp == buf)

			goto out_unlock;

		spin_lock_bh(&p->br->lock);

		ret = brport_attr->store(p, val);

		spin_unlock_bh(&p->br->lock);

	}



	if (!ret) {

tools/testing/selftests/net/rtnetlink.sh

+63 −0
Original line number Diff line number Diff line
@@ -24,6 +24,8 @@ ALL_TESTS=" 
	kci_test_macsec

	kci_test_macsec_vlan

	kci_test_team_bridge_macvlan

	kci_test_bridge_promisc_netlink

	kci_test_bridge_promisc_sysfs

	kci_test_ipsec

	kci_test_ipsec_offload

	kci_test_fdb_get
@@ -61,6 +63,14 @@ check_fail() 
	fi

}



sysfs_write()

{

	local val="$1"

	local path="$2"



	echo "$val" > "$path"

}



run_cmd_common()

{

	local cmd="$*"
@@ -680,6 +690,59 @@ kci_test_team_bridge_macvlan() 
	end_test "PASS: team_bridge_macvlan"

}



# Test that changing bridge port flags via the netlink path does not sleep with

# the bridge spin lock held.

kci_test_bridge_promisc_netlink()

{

	local dummy="test_dummy1"

	local bridge="test_br1"

	local team="test_team1"

	local ret=0



	run_cmd ip link add $team up type team

	run_cmd ip link add $bridge up type bridge vlan_filtering 1

	run_cmd ip link add $dummy up type dummy

	run_cmd ip link set $dummy master $bridge

	run_cmd ip link set $team master $bridge



	# This causes the bridge driver to sync all the static FDB entries to

	# the team device (which supports unicast filtering) and remove it from

	# promiscuous mode. The call to dev_set_promiscuity() can sleep due to

	# Rx mode inlining, which is a problem if the bridge spin lock is held.

	run_cmd bridge link set dev $dummy flood off learning off



	run_cmd ip link del $dummy

	run_cmd ip link del $bridge

	run_cmd ip link del $team



	end_test "PASS: bridge_promisc_netlink"

}



# Same as kci_test_bridge_promisc_netlink(), but the flags are changed via the

# sysfs path.

kci_test_bridge_promisc_sysfs()

{

	local dummy="test_dummy1"

	local bridge="test_br1"

	local team="test_team1"

	local ret=0



	run_cmd ip link add $team up type team

	run_cmd ip link add $bridge up type bridge vlan_filtering 1

	run_cmd ip link add $dummy up type dummy

	run_cmd ip link set $dummy master $bridge

	run_cmd ip link set $team master $bridge



	run_cmd sysfs_write 0 /sys/class/net/$dummy/brport/unicast_flood

	run_cmd sysfs_write 0 /sys/class/net/$dummy/brport/learning



	run_cmd ip link del $dummy

	run_cmd ip link del $bridge

	run_cmd ip link del $team



	end_test "PASS: bridge_promisc_sysfs"

}



#-------------------------------------------------------------------

# Example commands

#   ip x s add proto esp src 14.0.0.52 dst 14.0.0.70 \