Messages in this thread | | | Subject | Re: [PATCH] net: mac80211: Add NULL checks for sta->sdata | From | Jia-Ju Bai <> | Date | Tue, 21 Mar 2023 17:37:06 +0800 |
| |
On 2023/3/21 16:36, Simon Horman wrote: >>>> However, in the same call stack, sta->sdata is also used in the >>>> following functions: >>>> >>>> ieee80211_ba_session_work() >>>> ___ieee80211_stop_rx_ba_session(sta) >>>> ht_dbg(sta->sdata, ...); -> No check >>>> sdata_info(sta->sdata, ...); -> No check >>>> ieee80211_send_delba(sta->sdata, ...) -> No check >>>> ___ieee80211_start_rx_ba_session(sta) >>>> ht_dbg(sta->sdata, ...); -> No check >>>> ht_dbg_ratelimited(sta->sdata, ...); -> No check >>>> ieee80211_tx_ba_session_handle_start(sta) >>>> sdata = sta->sdata; if (!sdata) -> Add check by previous commit >>>> ___ieee80211_stop_tx_ba_session(sdata) >>>> ht_dbg(sta->sdata, ...); -> No check >>>> ieee80211_start_tx_ba_cb(sdata) >>>> sdata = sta->sdata; local = sdata->local -> No check >>>> ieee80211_stop_tx_ba_cb(sdata) >>>> ht_dbg(sta->sdata, ...); -> No check >>> I wonder if it would be better to teach ht_* do do nothing >>> if the first argument is NULL. >> Okay, I will use this way in patch v2. > Maybe it is not a good idea. > But I think it is worth trying, at least locally, to see how it goes.
After checking the code, I find that ht_* is actually a macro from _sdata_*. Many code points use ht_* and _sdata_*, and thus I am not sure it is fine to try this way.
For simplification, I still think checking sdata before the calls to ht_* or _sdata_* should be more proper :)
> >>> Also, are these theoretical bugs? >>> Or something that has been observed? >>> And has a reproducer? >> These bugs are found by my static analysis tool, by extending a known bug >> fixed in a previous commit 69403bad97aa. >> Thus, they could be theoretical bugs. > Thanks, understood. > I think it would be worth making that a bit clearer in the > patch description (commit message).
Okay.
I have sent the v2 patch, please have a look. Thanks a lot :)
Best wishes, Jia-Ju Bai
| |