> On 28. Feb 2023, at 18:44, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > > On Thu, 2023-01-26 at 17:53 +0100, Jonas Jelonek wrote: >>> >>> On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote: >>>> @@ -4846,16 +4989,32 @@ static int >>>> hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2, >>>> >>>> tx_attempts = (struct hwsim_tx_rate *)nla_data( >>>> info->attrs[HWSIM_ATTR_TX_INFO]); >>>> + tx_attempts_flags = (struct hwsim_tx_rate_flag *)nla_data( >>>> + info->attrs[HWSIM_ATTR_TX_INFO_FLAGS]); >>>> + sta = (struct ieee80211_sta *)txi->rate_driver_data[1]; >>> >>> That seems dangerous - what if the STA was freed already? You don't >>> walk >>> the pending list or something if the STA goes away. >> >> Yes, I see. Is it in general a bad idea to take the sta reference from >> ieee80211_control, put >> it in rate_driver_data and use it for tx-status? I guess I should pass >> sta to tx_status_ext whenever >> possible because it is used for several statistics. > > Well you have to think about the lifetime. In most cases you do a lookup > of the STA (under RCU) etc. but > >> I could think of two ways: >> - add NULL checks for the case that the sta pointer might be freed as >> you said > > How would that pointer even go NULL though? The pointer would remain, > but the STA can be freed, no? Yes, you’re right. Sorry, I mixed that up somehow. > >> - get sta by using, e.g., sta_info_get_by_addrs to get the sta if it >> is available. However, this always >> loops through the sta list. Might be a performance issue? > > It should use the hashtable? > >> Or do you suggest something different? > > Well you could keep it here and walk the list of queued skbs (?) when a > STA is removed, and kill them all at that point, or something. Not sure > it's worth it vs. the hash table lookup, this is just hwsim after all. > > johannes That’s also correct. Sorry, I didn’t have a deeper look into this function at the time of writing. So I guess I will stick to this for now. Jonas
