Re: [PATCH 02/12] wifi: mwifiex: fix MAC address handling

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Sep 06, 2024 at 04:40:36PM +0200, Francesco Dolcini wrote:
> On Mon, Aug 26, 2024 at 01:01:23PM +0200, Sascha Hauer wrote:
> > The mwifiex driver tries to derive the MAC addresses of the virtual
> > interfaces from the permanent address by adding the bss_num of the
> > particular interface used. It does so each time the virtual interface
> > is changed from AP to station or the other way round. This means that
> > the devices MAC address changes during a change_virtual_intf call which
> > is pretty unexpected by userspace.
> 
> Is this the only reason for this patch or there are other reasons?
> I'd like to understand the whole impact, to be sure the backport to
> stable is what we want.
> 
> > Furthermore the driver doesn't use the permanent address to add the
> > bss_num to, but instead the current MAC address increases each time
> > we do a change_virtual_intf.
> > 
> > Fix this by initializing the MAC address once from the permanent MAC
> > address during creation of the virtual interface and never touch it
> > again. This also means that userspace can set a different MAC address
> > which then stays like this forever and is not unexpectedly changed
> > by the driver.
> > 
> > It is not clear how many (if any) MAC addresses after the permanent MAC
> > address are reserved for a device, so set the locally admistered
> > bit for all MAC addresses modified from the permanent address.
> 
> I wonder if we should not just use the same permanent mac address whatever
> the virtual interface is. Do we have something similar in other wireless
> drivers?

Yes, there are at least four driver that generate different MAC
addresses for different vifs:

drivers/net/wireless/ath/ath6kl/cfg80211.c:3816
drivers/net/wireless/ath/wil6210/cfg80211.c:732
drivers/net/wireless/microchip/wilc1000/netdev.c:983
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:606

(line numbers match 6.11-rc6):

For the mac80211 based drivers there are also tricks played to use
unique MAC addresses in ieee80211_assign_perm_addr().

For reference in mwifiex setting different MAC addresses for different
interfaces goes down to:

| commit 864164683678e27c931b5909c72a001b1b943f36
| Author: Xinming Hu <huxm@marvell.com>
| Date:   Tue Feb 13 14:10:15 2018 +0800
| 
|     mwifiex: set different mac address for interfaces with same bss type
| 
|     Multiple interfaces with same bss type could affect each other if
|     they are sharing the same mac address. In this patch, different
|     mac address is assigned to new interface which have same bss type
|     with exist interfaces.
| 
|     Signed-off-by: Xinming Hu <huxm@marvell.com>
|     Signed-off-by: Kalle Valo <kvalo@codeaurora.org>


> 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/net/wireless/marvell/mwifiex/cfg80211.c |  4 +-
> >  drivers/net/wireless/marvell/mwifiex/init.c     |  1 -
> >  drivers/net/wireless/marvell/mwifiex/main.c     | 54 ++++++++++++-------------
> >  drivers/net/wireless/marvell/mwifiex/main.h     |  5 ++-
> >  4 files changed, 30 insertions(+), 34 deletions(-)
> > 
> ...
> 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 96d1f6039fbca..46acddd03ffd1 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -971,34 +971,16 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  }
> >  
> >  int mwifiex_set_mac_address(struct mwifiex_private *priv,
> > -			    struct net_device *dev, bool external,
> > -			    u8 *new_mac)
> > +			    struct net_device *dev, u8 *new_mac)
> >  {
> >  	int ret;
> > -	u64 mac_addr, old_mac_addr;
> > +	u64 old_mac_addr;
> >  
> > -	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
> > +	netdev_info(dev, "%s: old: %pM new: %pM\n", __func__, priv->curr_addr, new_mac);
> >  
> > -	if (external) {
> > -		mac_addr = ether_addr_to_u64(new_mac);
> > -	} else {
> > -		/* Internal mac address change */
> > -		if (priv->bss_type == MWIFIEX_BSS_TYPE_ANY)
> > -			return -EOPNOTSUPP;
> this was the only usage of MWIFIEX_BSS_TYPE_ANY, correct? Did it had any
> reason before?

I haven't found a path to get here with priv->bss_type ==
MWIFIEX_BSS_TYPE_ANY. This function is called from
mwifiex_init_new_priv_params() and mwifiex_add_virtual_intf(). Both
functions set priv->bss_type to something else or bail out with an error
before calling mwifiex_set_mac_address(). It's also called from the
ndo_set_mac_address method, but for a registered net device the bss_type
should also be set to something meaningful.

> 
> > -
> > -		mac_addr = old_mac_addr;
> > -
> > -		if (priv->bss_type == MWIFIEX_BSS_TYPE_P2P) {
> > -			mac_addr |= BIT_ULL(MWIFIEX_MAC_LOCAL_ADMIN_BIT);
> > -			mac_addr += priv->bss_num;
> > -		} else if (priv->adapter->priv[0] != priv) {
> > -			/* Set mac address based on bss_type/bss_num */
> > -			mac_addr ^= BIT_ULL(priv->bss_type + 8);
> > -			mac_addr += priv->bss_num;
> > -		}
> > -	}
> > +	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
> >  
> > -	u64_to_ether_addr(mac_addr, priv->curr_addr);
> > +	ether_addr_copy(priv->curr_addr, new_mac);
> >  
> >  	/* Send request to firmware */
> >  	ret = mwifiex_send_cmd(priv, HostCmd_CMD_802_11_MAC_ADDRESS,
> > @@ -1015,6 +997,26 @@ int mwifiex_set_mac_address(struct mwifiex_private *priv,
> >  	return 0;
> >  }
> >  
> > +int mwifiex_set_default_mac_address(struct mwifiex_private *priv,
> > +				    struct net_device *dev)
> > +{
> > +	int priv_num;
> > +	u8 mac[ETH_ALEN];
> > +
> > +	ether_addr_copy(mac, priv->adapter->perm_addr);
> > +
> > +	for (priv_num = 0; priv_num < priv->adapter->priv_num; priv_num++)
> > +		if (priv == priv->adapter->priv[priv_num])
> > +			break;
> > +
> > +	if (priv_num) {
> > +		eth_addr_add(mac, priv_num);
> > +		mac[0] |= 0x2;
> > +	}
> 
> Please see my concern on this in the beginning of the email.
> 
> > @@ -1364,10 +1366,6 @@ void mwifiex_init_priv_params(struct mwifiex_private *priv,
> >  	priv->assocresp_idx = MWIFIEX_AUTO_IDX_MASK;
> >  	priv->gen_idx = MWIFIEX_AUTO_IDX_MASK;
> >  	priv->num_tx_timeout = 0;
> > -	if (is_valid_ether_addr(dev->dev_addr))
> > -		ether_addr_copy(priv->curr_addr, dev->dev_addr);
> > -	else
> > -		ether_addr_copy(priv->curr_addr, priv->adapter->perm_addr);
> 
> With this change, when mfg_mode is true, priv->curr_addr will be not
> initialized. Wanted?

Not wanted, just me being ignorant. Let's have a look:

priv->adapter->perm_addr is initialized in the response handling of the
HostCmd_CMD_GET_HW_SPEC command. This command is only issued when
mfg_mode is false, so in mfg mode priv->adapter->perm_addr will be the
zero address.

The only documentation we have for mfg_mode is:

manufacturing mode enable:1, disable:0

I don't know what this really is about, but I could imagine that this
is for initial factory bringup when the chip is not parametrized and thus
doesn't have a permanent MAC address yet.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |