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@xxxxxxxxxxx>
| 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@xxxxxxxxxxx>
| Signed-off-by: Kalle Valo <kvalo@xxxxxxxxxxxxxx>
>
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > 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 |
