Hi,
On 21/05/2024 14:15, Remi Pommarel wrote:
On Tue, May 21, 2024 at 09:43:56AM +0200, Antonio Quartulli wrote:
Hi,
On 18/05/2024 17:50, Remi Pommarel wrote:
Wiphy should be locked before calling rdev_get_station() (see lockdep
assert in ieee80211_get_station()).
Adding the lock is fine as nowadays it is taken in pre_doit and released in
post_doit (with some exceptions). Therefore when invoking .get_station from
a side path the lock should be taken too.
It was actually a05829a7222e9d10c416dd2dbbf3929fe6646b89 that introduced
this requirement AFAICS.
IIUC lock requirement was already there before this commit, only it was on
rtnl lock to be taken instead of wiphy one.
You're right.
This fixes the following kernel NULL dereference:
As already said by Johannes, I am not sure it truly fixes this NULL
dereference though.
Have you checked where in ath10k_sta_statistics this is exactly happening?
Do you think some sta was partly released and thus fields were NULLified?
ath10k_sta_statistics+0x10 is associated with the arsta->arvif->ar
statement. It crashes because arsta->arvif is NULL.
Here is a scenario that explains the crash where the same sta
disconnects then reconnects quickly (e.g. OPEN network):
CPU0: CPU1:
batadv_v_elp_periodic_work()
queue_work(batadv_v_elp_get_throughput)
ieee80211_del_station()
ieee80211_add_station()
sta_info_insert()
list_add_tail_rcu()
ath10k_sta_state()
memset(arsta, 0, sizeof(arsta))
batadv_v_elp_get_throughput()
cfg80211_get_station()
ieee80211_get_station() <-- Find sta with its MAC in list
ath10k_sta_statistics()
arsta->arvif->ar <-- arsta is still zeroed
In other words if the same sta has enough time to disconnect then
reconnect before batadv_v_elp_get_throughput get scheduled, sta could be
partially initialized when ath10k_sta_statistics() is called. Locking
the wiphy ensure sta is fully initialized if found.
We were just wondering how you could get the arvif being NULL and your
explanation makes sense.
This said, holding the lock is required when invoking get_station via
netlink, therefore it's meaningful to hold it even when side invoking it
from another module.
Acked-by: Antonio Quartulli <a@xxxxxxxxxxx>
--
Antonio Quartulli
