Skip to content

Commit

Permalink
bonding: protect port for bond_3ad_handle_link_change()
Browse files Browse the repository at this point in the history
The bond_3ad_handle_link_change is called with RTNL only,
and the function will modify the port's information with
no further locking, it will not mutex against bond state
machine and incoming LACPDU which do not hold RTNL, So I
add __get_state_machine_lock to protect the port.

But it is not a critical bug, it exist since day one, and till
now it has never been hit and reported, because changes to
speed is very rare, and will not occur critical problem.

The comments in the function is very old, cleanup it and
add a new pr_debug to debug the port message.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
dingtianhong authored and David S. Miller committed Dec 18, 2013
1 parent bca44a7 commit 108db73
Showing 1 changed file with 17 additions and 6 deletions.
23 changes: 17 additions & 6 deletions drivers/net/bonding/bond_3ad.c
Original file line number Diff line number Diff line change
Expand Up @@ -2268,15 +2268,21 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)

port = &(SLAVE_AD_INFO(slave).port);

// if slave is null, the whole port is not initialized
/* if slave is null, the whole port is not initialized */
if (!port->slave) {
pr_warning("Warning: %s: link status changed for uninitialized port on %s\n",
slave->bond->dev->name, slave->dev->name);
return;
}

// on link down we are zeroing duplex and speed since some of the adaptors(ce1000.lan) report full duplex/speed instead of N/A(duplex) / 0(speed)
// on link up we are forcing recheck on the duplex and speed since some of he adaptors(ce1000.lan) report
__get_state_machine_lock(port);
/* on link down we are zeroing duplex and speed since
* some of the adaptors(ce1000.lan) report full duplex/speed
* instead of N/A(duplex) / 0(speed).
*
* on link up we are forcing recheck on the duplex and speed since
* some of he adaptors(ce1000.lan) report.
*/
if (link == BOND_LINK_UP) {
port->is_enabled = true;
port->actor_admin_port_key &= ~AD_DUPLEX_KEY_BITS;
Expand All @@ -2292,10 +2298,15 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
port->actor_oper_port_key = (port->actor_admin_port_key &=
~AD_SPEED_KEY_BITS);
}
//BOND_PRINT_DBG(("Port %d changed link status to %s", port->actor_port_number, ((link == BOND_LINK_UP)?"UP":"DOWN")));
// there is no need to reselect a new aggregator, just signal the
// state machines to reinitialize
pr_debug("Port %d changed link status to %s",
port->actor_port_number,
(link == BOND_LINK_UP) ? "UP" : "DOWN");
/* there is no need to reselect a new aggregator, just signal the
* state machines to reinitialize
*/
port->sm_vars |= AD_PORT_BEGIN;

__release_state_machine_lock(port);
}

/*
Expand Down

0 comments on commit 108db73

Please sign in to comment.