On Thu, 2023-08-24 at 10:26 +0800, Wen Gong wrote: > On 8/23/2023 2:34 PM, Johannes Berg wrote: > > On Wed, 2023-08-23 at 10:28 +0800, Wen Gong wrote: > > > > /* need to have local link addresses for MLO connections */ > > > > > > > > WARN_ON(cr.ap_mld_addr && !cr.links[link_id].addr); > > > > > > > > makes no sense anymore. Not sure if that's the only one. > > > After this patch, the cr.links[link_id].addr will be a valid local link > > > address from > > > > > > struct cfg80211_rx_assoc_resp, so I think it is not needed remove now. > > You don't understand. > > > > The issue is that it's set the line above. > > > > > cr.links[link_id].addr = data->links[link_id].addr; > > > /* need to have local link addresses for MLO connections */ > > > WARN_ON(cr.ap_mld_addr && !cr.links[link_id].addr); > > But look at that! What values can cr.links[link_id].addr get? Note how > > it's a pointer - assigned from an array. > > Oh, I know it now. the cr.links[link_id].addr will always NOT 0 because > it is pointer > > to an array "u8 addr[ETH_ALEN]" of struct cfg80211_rx_assoc_resp. Yep. > So maybe we can choose one of the 2 things to do: > > 1. remove the "WARN_ON(cr.ap_mld_addr && !cr.links[link_id].addr);" > > 2. change like this: > > WARN_ON(cr.ap_mld_addr && !is_valid_ether_addr(cr.links[link_id].addr)); > I think we should check that it's valid, because if you don't fill it, it'll (hopefully) point to zeroed data somewhere. johannes
