| Message ID | 20220919121155.3069765-2-quic_vjakkam@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2,1/4] wifi: nl80211: Add support for randomizing TA of auth and deauth frames | expand |
Hi, So I was going to apply patches 1 and 4 (with a leak fix to 4, please check before you resend), but have some issues with this one, and that raises a design question ... +Jouni because of that. > +++ b/include/net/mac80211.h > @@ -1832,6 +1832,7 @@ struct ieee80211_vif_cfg { > * @offload_flags: 802.3 -> 802.11 enapsulation offload flags, see > * &enum ieee80211_offload_flags. > * @mbssid_tx_vif: Pointer to the transmitting interface if MBSSID is enabled. > + * @random_addr: random address in use for this interface. That could be in vif_cfg, no? > > @@ -4164,6 +4167,11 @@ struct ieee80211_prep_tx_info { > * Note that a sta can also be inserted or removed with valid links, > * i.e. passed to @sta_add/@sta_state with sta->valid_links not zero. > * In fact, cannot change from having valid_links and not having them. > + * @config_random_mac: Configure random MAC address to send acknowledgment when > + * RA of the received frame matches with configured random MAC address. > + * Also, clear random MAC address configuration if zero MAC address set. > + * Driver must register callback for this when advertise > + * %NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA. And then maybe this isn't necessary, and we can use a new BSS change flag instead? Either way, you didn't document the context requirements here. > +++ b/net/mac80211/rx.c > @@ -4211,6 +4211,21 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx) > u8 *bssid = ieee80211_get_bssid(hdr, skb->len, sdata->vif.type); > bool multicast = is_multicast_ether_addr(hdr->addr1) || > ieee80211_is_s1g_beacon(hdr->frame_control); > + bool skip_addr1_check = false; > + > + if ((ieee80211_is_auth(hdr->frame_control) || > + ieee80211_is_deauth(hdr->frame_control)) && > + wiphy_ext_feature_isset( > + sdata->local->hw.wiphy, > + NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) && No need for the support check - if not supported then vif.random_addr on the next line cannot be set properly? > + ether_addr_equal(sdata->vif.random_addr, hdr->addr1)) { > + skip_addr1_check = true; > + if (ieee80211_is_deauth(hdr->frame_control)) { > + eth_zero_addr(sdata->vif.random_addr); > + drv_config_random_mac(sdata->local, > + rx->sdata->vif.random_addr); > + } > + } maybe some unlikely() would be appropriate, e.g. around even only the first is_auth||is_deauth since those are already unlikely and the rest of the checks can be done separately? I also _really_ don't like the call to drv_config_random_mac() in the middle of this flow here - coming from the driver back into the driver is always a pain. This might be a reason *not* to do the vif config and all that, but you should document that more clearly (the design decision process in the commit message, and the actual context requirements in the kernel-doc.) That said, aren't many drivers going to anyway punt this to a worker, maybe we should here and then we can put it together with other vif config? I'm also asking myself when this is going to get cancelled if the frame is *not* a deauth? For something like PASN you'd expect one response for every TX, so maybe that's OK and we can cancel when you stop waiting? OTOH, maybe this really shouldn't be called "random TA" but rather "temporary address", and userspace picks in both mgmt-TX and remain-on- channel? As it is, until you deauth, you keep the random address alive in the driver, afaict? > @@ -4657,6 +4676,21 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx, > } addrs __aligned(2); > struct link_sta_info *link_sta; > struct ieee80211_sta_rx_stats *stats; > + bool skip_addr1_check = false; > + > + if ((ieee80211_is_auth(hdr->frame_control) || > + ieee80211_is_deauth(hdr->frame_control)) && > + wiphy_ext_feature_isset( > + rx->sdata->local->hw.wiphy, > + NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) && > + ether_addr_equal(rx->sdata->vif.random_addr, hdr->addr1)) { > + skip_addr1_check = true; > + if (ieee80211_is_deauth(hdr->frame_control)) { > + eth_zero_addr(rx->sdata->vif.random_addr); > + drv_config_random_mac(rx->sdata->local, > + rx->sdata->vif.random_addr); > + } > + } Um, no, there's really no point in changing this function??? How did that happen? > +++ b/net/mac80211/tx.c > @@ -2048,6 +2048,7 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, > struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data; > int headroom; > enum ieee80211_encrypt encrypt; > + bool our_addr = true; > > if (info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT) > encrypt = ENCRYPT_NO; > @@ -2071,6 +2072,32 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, > hdr = (struct ieee80211_hdr *) skb->data; > info->control.vif = &sdata->vif; > > + switch (sdata->vif.type) { This doesn't seem like an appropriate place for this logic - why not do that closer to userspace? We go through this for *every* mac80211 TX, but we only *need* it for userspace TX - i.e. somewhere mgmt_tx or so? Anyway, I still think there's a design issue here in how you handle the lifetime of this temporary address, and would think that it might make more sense to address that within the context of remain-on-channel and the response waiting time offchannel period. Remain-on-channel might not even be needed for the purpose here since you'd get a response and then give up waiting for a response? But it might be needed in some retry cases perhaps, I don't know. johannes
On 12/1/2022 7:02 PM, Johannes Berg wrote: > > Anyway, I still think there's a design issue here in how you handle the > lifetime of this temporary address, and would think that it might make > more sense to address that within the context of remain-on-channel and > the response waiting time offchannel period. Remain-on-channel might not > even be needed for the purpose here since you'd get a response and then > give up waiting for a response? But it might be needed in some retry > cases perhaps, I don't know. > > johannes Thanks Johannes for reviewing the patches. The suggested approach to keep the allowed random/temporary address till the response waiting time off-channel period looks more cleaner and better. I will send new patch series with the suggested changes. Also, we are considering to drop authentication offload changes patch (4th patch) from this series. We think it would be good to align with mac80211 and use MLD addresses in the frames during authentication offload case also. I will send a separate to patch for supporting authentication offload with MLD addressed frames. -- veeru
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index ac2bad57933f..662b5db84fe3 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1832,6 +1832,7 @@ struct ieee80211_vif_cfg { * @offload_flags: 802.3 -> 802.11 enapsulation offload flags, see * &enum ieee80211_offload_flags. * @mbssid_tx_vif: Pointer to the transmitting interface if MBSSID is enabled. + * @random_addr: random address in use for this interface. */ struct ieee80211_vif { enum nl80211_iftype type; @@ -1861,6 +1862,8 @@ struct ieee80211_vif { struct ieee80211_vif *mbssid_tx_vif; + u8 random_addr[ETH_ALEN] __aligned(2); + /* must be last */ u8 drv_priv[] __aligned(sizeof(void *)); }; @@ -4164,6 +4167,11 @@ struct ieee80211_prep_tx_info { * Note that a sta can also be inserted or removed with valid links, * i.e. passed to @sta_add/@sta_state with sta->valid_links not zero. * In fact, cannot change from having valid_links and not having them. + * @config_random_mac: Configure random MAC address to send acknowledgment when + * RA of the received frame matches with configured random MAC address. + * Also, clear random MAC address configuration if zero MAC address set. + * Driver must register callback for this when advertise + * %NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA. */ struct ieee80211_ops { void (*tx)(struct ieee80211_hw *hw, @@ -4519,6 +4527,7 @@ struct ieee80211_ops { struct ieee80211_vif *vif, struct ieee80211_sta *sta, u16 old_links, u16 new_links); + void (*config_random_mac)(struct ieee80211_hw *hw, const u8 *mac_addr); }; /** diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h index 81e40b0a3b16..ea2cd8048638 100644 --- a/net/mac80211/driver-ops.h +++ b/net/mac80211/driver-ops.h @@ -1479,4 +1479,14 @@ int drv_change_sta_links(struct ieee80211_local *local, struct ieee80211_sta *sta, u16 old_links, u16 new_links); +static inline void drv_config_random_mac(struct ieee80211_local *local, + const u8 *mac_addr) +{ + if (local->ops->config_random_mac) { + trace_drv_config_random_mac(local, mac_addr); + local->ops->config_random_mac(&local->hw, mac_addr); + trace_drv_return_void(local); + } +} + #endif /* __MAC80211_DRIVER_OPS */ diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 46f3eddc2388..707b38865227 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -953,6 +953,11 @@ int ieee80211_register_hw(struct ieee80211_hw *hw) !local->ops->tdls_recv_channel_switch)) return -EOPNOTSUPP; + if ((hw->wiphy->features & + NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) && + !local->ops->config_random_mac) + return -EOPNOTSUPP; + if (WARN_ON(ieee80211_hw_check(hw, SUPPORTS_TX_FRAG) && !local->ops->set_frag_threshold)) return -EINVAL; diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index a57811372027..e76653795c1c 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -4211,6 +4211,21 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx) u8 *bssid = ieee80211_get_bssid(hdr, skb->len, sdata->vif.type); bool multicast = is_multicast_ether_addr(hdr->addr1) || ieee80211_is_s1g_beacon(hdr->frame_control); + bool skip_addr1_check = false; + + if ((ieee80211_is_auth(hdr->frame_control) || + ieee80211_is_deauth(hdr->frame_control)) && + wiphy_ext_feature_isset( + sdata->local->hw.wiphy, + NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) && + ether_addr_equal(sdata->vif.random_addr, hdr->addr1)) { + skip_addr1_check = true; + if (ieee80211_is_deauth(hdr->frame_control)) { + eth_zero_addr(sdata->vif.random_addr); + drv_config_random_mac(sdata->local, + rx->sdata->vif.random_addr); + } + } switch (sdata->vif.type) { case NL80211_IFTYPE_STATION: @@ -4220,6 +4235,8 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx) return false; if (multicast) return true; + if (skip_addr1_check) + return true; return ieee80211_is_our_addr(sdata, hdr->addr1, &rx->link_id); case NL80211_IFTYPE_ADHOC: if (!bssid) @@ -4232,7 +4249,7 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx) return true; if (!ieee80211_bssid_match(bssid, sdata->u.ibss.bssid)) return false; - if (!multicast && + if (!multicast && !skip_addr1_check && !ether_addr_equal(sdata->vif.addr, hdr->addr1)) return false; if (!rx->sta) { @@ -4252,7 +4269,7 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx) return false; if (!is_broadcast_ether_addr(bssid)) return false; - if (!multicast && + if (!multicast && !skip_addr1_check && !ether_addr_equal(sdata->dev->dev_addr, hdr->addr1)) return false; if (!rx->sta) { @@ -4270,10 +4287,12 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx) return false; if (multicast) return true; + if (skip_addr1_check) + return true; return ether_addr_equal(sdata->vif.addr, hdr->addr1); case NL80211_IFTYPE_AP_VLAN: case NL80211_IFTYPE_AP: - if (!bssid) + if (!bssid && !skip_addr1_check) return ieee80211_is_our_addr(sdata, hdr->addr1, &rx->link_id); @@ -4285,7 +4304,7 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx) * and location updates. Note that mac80211 * itself never looks at these frames. */ - if (!multicast && + if (!multicast && !skip_addr1_check && !ieee80211_is_our_addr(sdata, hdr->addr1, &rx->link_id)) return false; @@ -4657,6 +4676,21 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx, } addrs __aligned(2); struct link_sta_info *link_sta; struct ieee80211_sta_rx_stats *stats; + bool skip_addr1_check = false; + + if ((ieee80211_is_auth(hdr->frame_control) || + ieee80211_is_deauth(hdr->frame_control)) && + wiphy_ext_feature_isset( + rx->sdata->local->hw.wiphy, + NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) && + ether_addr_equal(rx->sdata->vif.random_addr, hdr->addr1)) { + skip_addr1_check = true; + if (ieee80211_is_deauth(hdr->frame_control)) { + eth_zero_addr(rx->sdata->vif.random_addr); + drv_config_random_mac(rx->sdata->local, + rx->sdata->vif.random_addr); + } + } /* for parallel-rx, we need to have DUP_VALIDATED, otherwise we write * to a common data structure; drivers can implement that per queue @@ -4690,7 +4724,8 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx, * punting here will make it go through the full checks in * ieee80211_accept_frame(). */ - if (!ether_addr_equal(fast_rx->vif_addr, hdr->addr1)) + if (!skip_addr1_check && + !ether_addr_equal(fast_rx->vif_addr, hdr->addr1)) return false; if ((hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_FROMDS | diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h index 9f4377566c42..4996fd5a6887 100644 --- a/net/mac80211/trace.h +++ b/net/mac80211/trace.h @@ -3026,6 +3026,25 @@ TRACE_EVENT(stop_queue, ) ); +TRACE_EVENT(drv_config_random_mac, + TP_PROTO(struct ieee80211_local *local, + const u8 *mac_addr), + + TP_ARGS(local, mac_addr), + + TP_STRUCT__entry( + LOCAL_ENTRY + __array(char, mac_addr, ETH_ALEN) + ), + + TP_fast_assign( + LOCAL_ASSIGN; + memcpy(__entry->mac_addr, mac_addr, ETH_ALEN); + ), + + TP_printk(LOCAL_PR_FMT ", addr:%pM", LOCAL_PR_ARG, __entry->mac_addr) +); + #endif /* !__MAC80211_DRIVER_TRACE || TRACE_HEADER_MULTI_READ */ #undef TRACE_INCLUDE_PATH diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 24c0a1706b92..76e2457050cb 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2048,6 +2048,7 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data; int headroom; enum ieee80211_encrypt encrypt; + bool our_addr = true; if (info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT) encrypt = ENCRYPT_NO; @@ -2071,6 +2072,32 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata, hdr = (struct ieee80211_hdr *) skb->data; info->control.vif = &sdata->vif; + switch (sdata->vif.type) { + case NL80211_IFTYPE_OCB: + if (!ether_addr_equal(sdata->dev->dev_addr, hdr->addr2)) + our_addr = false; + break; + default: + if (!ieee80211_is_our_addr(sdata, hdr->addr2, NULL)) + our_addr = false; + break; + } + + if (!our_addr && + wiphy_ext_feature_isset( + local->hw.wiphy, + NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA)) { + if (ieee80211_is_auth(hdr->frame_control)) { + drv_config_random_mac(local, hdr->addr2); + ether_addr_copy(sdata->vif.random_addr, hdr->addr2); + } else if (ieee80211_is_deauth(hdr->frame_control) && + ether_addr_equal(sdata->vif.random_addr, + hdr->addr2)) { + eth_zero_addr(sdata->vif.random_addr); + drv_config_random_mac(local, sdata->vif.random_addr); + } + } + if (ieee80211_vif_is_mesh(&sdata->vif)) { if (ieee80211_is_data(hdr->frame_control) && is_unicast_ether_addr(hdr->addr1)) {
Configure randomized transmit address of the authentication frames to the driver when advertise NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA to enable receive behavior to accept frames with the same random MAC address as RA also those frames need to be acknowledged similarly to the frames sent to the local permanent MAC address. Disable the random MAC address configuration when deauthentication frames with same random MAC address received or transmitted. Signed-off-by: Veerendranath Jakkam <quic_vjakkam@quicinc.com> --- include/net/mac80211.h | 9 ++++++++ net/mac80211/driver-ops.h | 10 +++++++++ net/mac80211/main.c | 5 +++++ net/mac80211/rx.c | 45 ++++++++++++++++++++++++++++++++++----- net/mac80211/trace.h | 19 +++++++++++++++++ net/mac80211/tx.c | 27 +++++++++++++++++++++++ 6 files changed, 110 insertions(+), 5 deletions(-)