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.
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 derived from the permanent address.
With this patch MWIFIEX_BSS_TYPE_ANY becomes unused, so it's removed.
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/decl.h | 1 -
drivers/net/wireless/marvell/mwifiex/init.c | 1 -
drivers/net/wireless/marvell/mwifiex/main.c | 54 ++++++++++++-------------
drivers/net/wireless/marvell/mwifiex/main.h | 5 ++-
5 files changed, 30 insertions(+), 35 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 5697a02e6b8d3..d3e1424bea390 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -962,8 +962,6 @@ mwifiex_init_new_priv_params(struct mwifiex_private *priv,
adapter->rx_locked = false;
spin_unlock_bh(&adapter->rx_proc_lock);
- mwifiex_set_mac_address(priv, dev, false, NULL);
-
return 0;
}
@@ -3115,7 +3113,7 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
priv->netdev = dev;
if (!adapter->mfg_mode) {
- mwifiex_set_mac_address(priv, dev, false, NULL);
+ mwifiex_set_default_mac_address(priv, dev);
ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,
HostCmd_ACT_GEN_SET, 0, NULL, true);
diff --git a/drivers/net/wireless/marvell/mwifiex/decl.h b/drivers/net/wireless/marvell/mwifiex/decl.h
index 84603f1e7f6e0..d84773321dc47 100644
--- a/drivers/net/wireless/marvell/mwifiex/decl.h
+++ b/drivers/net/wireless/marvell/mwifiex/decl.h
@@ -139,7 +139,6 @@ enum mwifiex_bss_type {
MWIFIEX_BSS_TYPE_STA = 0,
MWIFIEX_BSS_TYPE_UAP = 1,
MWIFIEX_BSS_TYPE_P2P = 2,
- MWIFIEX_BSS_TYPE_ANY = 0xff,
};
enum mwifiex_bss_role {
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 8b61e45cd6678..0259c9f88486b 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -71,7 +71,6 @@ int mwifiex_init_priv(struct mwifiex_private *priv)
u32 i;
priv->media_connected = false;
- eth_broadcast_addr(priv->curr_addr);
priv->port_open = false;
priv->usb_port = MWIFIEX_USB_EP_DATA;
priv->pkt_tx_ctrl = 0;
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;
-
- 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;
+ }
+
+ return mwifiex_set_mac_address(priv, dev, mac);
+}
+
/* CFG802.11 network device handler for setting MAC address.
*/
static int
@@ -1023,7 +1025,7 @@ mwifiex_ndo_set_mac_address(struct net_device *dev, void *addr)
struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
struct sockaddr *hw_addr = addr;
- return mwifiex_set_mac_address(priv, dev, true, hw_addr->sa_data);
+ return mwifiex_set_mac_address(priv, dev, hw_addr->sa_data);
}
/*
@@ -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);
if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA ||
GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_UAP) {
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 529863edd7a25..dc07eb11f7752 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1694,8 +1694,9 @@ void mwifiex_process_multi_chan_event(struct mwifiex_private *priv,
struct sk_buff *event_skb);
void mwifiex_multi_chan_resync(struct mwifiex_adapter *adapter);
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 mwifiex_set_default_mac_address(struct mwifiex_private *priv,
+ struct net_device *dev);
void mwifiex_devdump_tmo_func(unsigned long function_context);
#ifdef CONFIG_DEBUG_FS
--
2.39.5Hi Sascha, Sorry for being a bit slow here. And even now, I probably don't have enough time to review this whole series today. But I'll still share some initial thoughts, in case you can help address them before I next look at this: On Wed, Sep 18, 2024 at 01:10:27PM +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. Ack, the "change_virtual_intf" part looks wrong. > 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 derived from the permanent address. I think I'm generally supportive of the direction this changes things, but I'm a bit hesitant about two things: 1. the potential user-visible changes and 2. the linux-stable backport (Cc stable below) For 1: MAC addresses are important in some contexts, and this might significantly change the addresses that devices get in practice. Such users might not really care about the weird details of when the address incremented; but they *probably* care that a certain sequence of "boot device; run hostapd with <foo> config file" produces the same address. Also, I'm not sure I know enough of the implications of potential over-use of the locally administered bit. Are there significant downsides to it (aside from the simple fact that it's a different address)? And I see that you rightly don't know how many addresses are actually reserved, but I have an educated guess that it's not just 1. For one, this driver used to default-create 3 interfaces: 1211c961170c mwifiex: do not create AP and P2P interfaces upon driver loading and when we stopped doing that, we still kept support for a module parameter for the old way: 0013c7cebed6 mwifiex: module load parameter for interface creation Perhaps these "initial" interfaces should at least be allowed permanent addresses? So anyway, I don't really know for sure the right answer, but I want to log my concerns, in case you had more thoughts on backward compatibility. And given all the uncertainty above, I'm extra hesitant about backporting likely-user-visible changes to stable (#2). > With this patch MWIFIEX_BSS_TYPE_ANY becomes unused, so it's removed. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > Cc: stable@vger.kernel.org Regards, Brian
Hi Brian,
It's been a while, but I'd like to get this forward now.
On Fri, Oct 04, 2024 at 04:15:32PM -0700, Brian Norris wrote:
> I think I'm generally supportive of the direction this changes things,
> but I'm a bit hesitant about two things:
> 1. the potential user-visible changes and
> 2. the linux-stable backport (Cc stable below)
>
> For 1: MAC addresses are important in some contexts, and this might
> significantly change the addresses that devices get in practice. Such
> users might not really care about the weird details of when the address
> incremented; but they *probably* care that a certain sequence of "boot
> device; run hostapd with <foo> config file" produces the same address.
>
> Also, I'm not sure I know enough of the implications of potential
> over-use of the locally administered bit. Are there significant
> downsides to it (aside from the simple fact that it's a different
> address)?
Not that I know of, but that doesn't mean much.
>
> And I see that you rightly don't know how many addresses are actually
> reserved, but I have an educated guess that it's not just 1.
Even if there are more addresses reserved, we don't know which these
are, see below.
> For one,
> this driver used to default-create 3 interfaces:
> 1211c961170c mwifiex: do not create AP and P2P interfaces upon driver loading
>
> and when we stopped doing that, we still kept support for a module
> parameter for the old way:
> 0013c7cebed6 mwifiex: module load parameter for interface creation
>
> Perhaps these "initial" interfaces should at least be allowed permanent
> addresses?
I started up a board with the downstream driver. It comes up with these
MAC addresses:
wlp1s0 Link encap:Ethernet HWaddr 34:6F:24:4E:E0:3D
uap0 Link encap:Ethernet HWaddr 36:6F:24:4E:E1:3D
wfd0 Link encap:Ethernet HWaddr 36:6F:24:4E:E0:3D
The permanent address from EEPROM is 34:6F:24:4E:E0:3D which is
used for wlp1s0. For the other addresses the locally admistered bit is
set (34 -> 36). Here's the corresponding code:
if (priv->bss_type == MLAN_BSS_TYPE_WIFIDIRECT) {
if (priv->bss_virtual) {
...
} else {
priv->current_addr[0] |= 0x02;
}
}
if (priv->bss_type != MLAN_BSS_TYPE_WIFIDIRECT) {
if (priv->bss_index) {
priv->current_addr[0] |= 0x02;
priv->current_addr[4] += priv->bss_index;
}
}
See https://github.com/nxp-imx/mwifiex/blob/lf-6.6.3_1.0.0/mxm_wifiex/wlan_src/mlinux/moal_main.c#L8383
Note this behaviour was changed in the driver in a0835444f1
("mxm_wifiex: update to mxm5x17344.p1 release"). Before that the driver
has just done a priv->current_addr[4] += priv->bss_index without
setting the locally admistered bit. Of course the commit message
says nothing about the reasons for this change.
The downstream driver puts the bss_num (or bss_index) into different
bits than the upstream driver does. It uses current_addr[4] whereas the
upstream driver uses current_addr[5]. So even when there's more than
one MAC address reserved for one chip, both drivers disagree on which
addresses these are.
Given that, I think our safest bet is to always set the locally
admistered bit for derived MAC addresses.
>
> So anyway, I don't really know for sure the right answer, but I want to
> log my concerns, in case you had more thoughts on backward
> compatibility.
>
> And given all the uncertainty above, I'm extra hesitant about
> backporting likely-user-visible changes to stable (#2).
I can remove the stable tag if makes you feel more comfortable.
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 |
On Fri, Oct 04, 2024 at 04:15:32PM -0700, Brian Norris wrote: > On Wed, Sep 18, 2024 at 01:10:27PM +0200, Sascha Hauer wrote: > > 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 derived from the permanent address. > > I think I'm generally supportive of the direction this changes things, > but I'm a bit hesitant about two things: FTR, I am hesitant for similar reasons. In addition there is my comment from the previous version on a specific use case potentially broken. > And I see that you rightly don't know how many addresses are actually > reserved, but I have an educated guess that it's not just 1. For one, > this driver used to default-create 3 interfaces: The MAC addresses on the module are not allocated by the Wi-Fi chip vendor (NXP, and before Marvell), but by whom is integrating the chip. I can try to ask a couple of vendors what they are doing on this regard, and if this is somehow suggested by NXP or they are just doing whatever they believe is right. Just to add a couple of more data points to this discussion. Francesco
© 2016 - 2024 Red Hat, Inc.