Re: [PATCH v3 net-next] bonding: no longer rely on RTNL in bond_fill_info()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
bonding: no longer rely on RTNL in bond_fill_info()

This commit removes the RTNL lock dependency in bond_fill_info(), replacing
it with RCU protection and READ_ONCE()/WRITE_ONCE() annotations for reading
bond configuration fields.

> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 90365d3f7ebff..aa1725e6a3776 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c

[ ... ]

> @@ -674,53 +674,58 @@ static size_t bond_get_size(const struct net_device *bond_dev)

[ ... ]

>  static int bond_fill_info(struct sk_buff *skb,
>  			  const struct net_device *bond_dev)
>  {
> -	struct bonding *bond = netdev_priv(bond_dev);
> -	unsigned int packets_per_slave;
> -	int ifindex, i, targets_added;
> +	const struct bonding *bond = netdev_priv(bond_dev);
> +	int i, targets_added, miimon, mode;
> +	const struct slave *primary;
>  	struct nlattr *targets;
> -	struct slave *primary;
>  
> -	if (nla_put_u8(skb, IFLA_BOND_MODE, BOND_MODE(bond)))
> +	rcu_read_lock();
> +	mode = READ_ONCE(bond->params.mode);

[ ... ]

> -	if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> +	if (mode == BOND_MODE_8023AD) {
>  		struct ad_info info;
>  
>  		if (capable(CAP_NET_ADMIN)) {
>  			if (nla_put_u16(skb, IFLA_BOND_AD_ACTOR_SYS_PRIO,
> -					bond->params.ad_actor_sys_prio))
> +					READ_ONCE(bond->params.ad_actor_sys_prio)))
>  				goto nla_put_failure;
>  
>  			if (nla_put_u16(skb, IFLA_BOND_AD_USER_PORT_KEY,
> -					bond->params.ad_user_port_key))
> +					READ_ONCE(bond->params.ad_user_port_key)))
>  				goto nla_put_failure;
>  
> +			/* Small race here, this is a minor trade off. */
>  			if (nla_put(skb, IFLA_BOND_AD_ACTOR_SYSTEM,
>  				    ETH_ALEN, &bond->params.ad_actor_system))
>  				goto nla_put_failure;
>  		}
> -		if (!bond_3ad_get_active_agg_info(bond, &info)) {
> +		if (!__bond_3ad_get_active_agg_info(bond, &info)) {
        ^^^^

[Severity: Critical]
Is it possible for a concurrent mode change to cause a NULL pointer
dereference here?

Since bond_fill_info() no longer holds the RTNL lock, another thread could
concurrently change the bonding mode from BOND_MODE_8023AD to
BOND_MODE_ACTIVEBACKUP and immediately enslave a new device.

During bond_alloc_slave(), if the new mode is ACTIVEBACKUP, slave->ad_info
is not allocated and remains NULL. The slave is then added to the
RCU-protected bond->slave_list.

Because bond_fill_info() cached the mode earlier as BOND_MODE_8023AD:

    mode = READ_ONCE(bond->params.mode);

It will still enter this block and call __bond_3ad_get_active_agg_info().
That function iterates over the slave list and attempts to dereference the
newly added slave's ad_info without checking if it is NULL:

drivers/net/bonding/bond_3ad.c:__bond_3ad_get_active_agg_info() {
        ...
        bond_for_each_slave_rcu(bond, slave, iter) {
                port = &(SLAVE_AD_INFO(slave)->port);
                tmp = rcu_dereference(port->aggregator);
        ...
}

Could this be addressed by adding a NULL check for SLAVE_AD_INFO(slave) in
__bond_3ad_get_active_agg_info(), or by re-evaluating the mode?

>  			struct nlattr *nest;
>  
>  			nest = nla_nest_start_noflag(skb, IFLA_BOND_AD_INFO);
-- 
pw-bot: cr