* (bug report) iwlwifi: inconsitent NULL checking
@ 2015-11-26 12:03 Dan Carpenter
2015-11-26 12:16 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-11-26 12:03 UTC (permalink / raw)
To: johannes.berg; +Cc: linux-wireless
[ All old wireless warnings are showing up as new because the files
moved around in linux-next. - dan ]
Hello Johannes Berg,
The patch 8ca151b568b6: "iwlwifi: add the MVM driver" from Jan 24,
2013, leads to the following static checker warning:
drivers/net/wireless/intel/iwlwifi/mvm/rs.c:2802 rs_get_rate()
error: we previously assumed 'lq_sta' could be null (see line 2793)
drivers/net/wireless/intel/iwlwifi/mvm/rs.c
2790 /* TODO: handle rate_idx_mask and rate_idx_mcs_mask */
2791
2792 /* Treat uninitialized rate scaling data same as non-existing. */
2793 if (lq_sta && !lq_sta->pers.drv) {
^^^^^^
Check.
2794 IWL_DEBUG_RATE(mvm, "Rate scaling not initialized yet.\n");
2795 mvm_sta = NULL;
2796 }
2797
2798 /* Send management frames and NO_ACK data using lowest rate. */
2799 if (rate_control_send_low(sta, mvm_sta, txrc))
2800 return;
2801
2802 iwl_mvm_hwrate_to_tx_rate(lq_sta->last_rate_n_flags,
^^^^^^^^^^^^^^^^^^^^^^^^^
Uchecked dereference.
2803 info->band, &info->control.rates[0]);
2804 info->control.rates[0].count = 1;
2805
See also:
drivers/net/wireless/intel/iwlwifi/dvm/rs.c:2742 rs_get_rate() error: we previously assumed 'lq_sta' could be null (see line 2733)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: (bug report) iwlwifi: inconsitent NULL checking
2015-11-26 12:03 (bug report) iwlwifi: inconsitent NULL checking Dan Carpenter
@ 2015-11-26 12:16 ` Johannes Berg
2015-11-26 12:37 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2015-11-26 12:16 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-wireless
On Thu, 2015-11-26 at 15:03 +0300, Dan Carpenter wrote:
>
> 2793 if (lq_sta && !lq_sta->pers.drv) {
> ^^^^^^
> Check.
>
> 2794 IWL_DEBUG_RATE(mvm, "Rate scaling not
> initialized yet.\n");
> 2795 mvm_sta = NULL;
> 2796 }
> 2797
> 2798 /* Send management frames and NO_ACK data using
> lowest rate. */
> 2799 if (rate_control_send_low(sta, mvm_sta, txrc))
> 2800 return;
> 2801
> 2802 iwl_mvm_hwrate_to_tx_rate(lq_sta->last_rate_n_flags,
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> Uchecked dereference.
>
Yeah, this is a bit tricky. If you look into rate_control_send_low(),
I'm sure it'll return true when mvm_sta argument is NULL. Not sure how,
if at all, we could make that more explicit here.
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: (bug report) iwlwifi: inconsitent NULL checking
2015-11-26 12:16 ` Johannes Berg
@ 2015-11-26 12:37 ` Dan Carpenter
2015-11-26 12:51 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-11-26 12:37 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
On Thu, Nov 26, 2015 at 01:16:51PM +0100, Johannes Berg wrote:
> On Thu, 2015-11-26 at 15:03 +0300, Dan Carpenter wrote:
> >
> > 2793 if (lq_sta && !lq_sta->pers.drv) {
> > ^^^^^^
> > Check.
> >
> > 2794 IWL_DEBUG_RATE(mvm, "Rate scaling not
> > initialized yet.\n");
> > 2795 mvm_sta = NULL;
> > 2796 }
> > 2797
> > 2798 /* Send management frames and NO_ACK data using
> > lowest rate. */
> > 2799 if (rate_control_send_low(sta, mvm_sta, txrc))
> > 2800 return;
> > 2801
> > 2802 iwl_mvm_hwrate_to_tx_rate(lq_sta->last_rate_n_flags,
> > ^^^^^^^^^^^^^^^^^^^^^^^^^
> > Uchecked dereference.
> >
>
> Yeah, this is a bit tricky. If you look into rate_control_send_low(),
> I'm sure it'll return true when mvm_sta argument is NULL. Not sure how,
> if at all, we could make that more explicit here.
Ah. Thanks for looking at this. Eventually Smatch will be smart enough
to figure this implication out but not yet. No need to change the code
because the static checker isn't capable enough...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: (bug report) iwlwifi: inconsitent NULL checking
2015-11-26 12:37 ` Dan Carpenter
@ 2015-11-26 12:51 ` Johannes Berg
2015-11-26 19:44 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2015-11-26 12:51 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-wireless
On Thu, 2015-11-26 at 15:37 +0300, Dan Carpenter wrote:
>
> Ah. Thanks for looking at this. Eventually Smatch will be smart
> enoughto figure this implication out but not yet. No need to change
> the code because the static checker isn't capable enough...
>
Actually I just had an idea - we should put those two *checks* into an
inline anyway since they're actually really unlikely and we might not
want to take the function call every time...
So I thought that smatch would then be able to see through it, since
it's now an inline, but it doesn't seem like it can?
https://p.sipsolutions.net/a162ca550f675800.txt
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: (bug report) iwlwifi: inconsitent NULL checking
2015-11-26 12:51 ` Johannes Berg
@ 2015-11-26 19:44 ` Dan Carpenter
2015-11-26 20:21 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-11-26 19:44 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
There are two issues in Smatch that would need to be fixed:
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rs.c b/drivers/net/wireless/intel/iwlwifi/mvm/rs.c
index d1ad103..d3bc193 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rs.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rs.c
@@ -2767,6 +2767,7 @@ static void rs_initialize_lq(struct iwl_mvm *mvm,
iwl_mvm_send_lq_cmd(mvm, &lq_sta->lq, init);
}
+#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
static void rs_get_rate(void *mvm_r, struct ieee80211_sta *sta, void *mvm_sta,
struct ieee80211_tx_rate_control *txrc)
{
@@ -2795,9 +2796,13 @@ static void rs_get_rate(void *mvm_r, struct ieee80211_sta *sta, void *mvm_sta,
mvm_sta = NULL;
}
+ if (mvm_sta)
+ __smatch_implied(lq_sta);
+
/* Send management frames and NO_ACK data using lowest rate. */
if (rate_control_send_low(sta, mvm_sta, txrc))
return;
+ __smatch_implied(mvm_sta);
iwl_mvm_hwrate_to_tx_rate(lq_sta->last_rate_n_flags,
info->band, &info->control.rates[0]);
If apply that patch (with changes for your system) and run
kchecker drivers/net/wireless/intel/iwlwifi/mvm/rs.c then it prints:
drivers/net/wireless/intel/iwlwifi/mvm/rs.c:2800 rs_get_rate() implied: lq_sta = '0,4096-2117777777777777777'
drivers/net/wireless/intel/iwlwifi/mvm/rs.c:2805 rs_get_rate() implied: mvm_sta = 's64min-(-1),1-s64max'
The first problem is that it doesn't see that if mvm_sta is non-NULL
then that implies lq_sta is non-NULL. But even if that worked, after we
do the function call, it sees that the function that mvm_sta is non-NULL
but it wouldn't see that that means lq_sta is non-NULL. It's not so
terribly far from working but it's not there yet.
regards,
dan carpenter
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: (bug report) iwlwifi: inconsitent NULL checking
2015-11-26 19:44 ` Dan Carpenter
@ 2015-11-26 20:21 ` Johannes Berg
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Berg @ 2015-11-26 20:21 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-wireless
On Thu, 2015-11-26 at 22:44 +0300, Dan Carpenter wrote:
>
> The first problem is that it doesn't see that if mvm_sta is non-NULL
> then that implies lq_sta is non-NULL. But even if that worked, after
> we do the function call, it sees that the function that mvm_sta is
> non-NULL but it wouldn't see that that means lq_sta is non-
> NULL. It's not so terribly far from working but it's not there yet.
>
Ah, yes, I completely forgot that it's two (related) variables :)
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-26 20:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-26 12:03 (bug report) iwlwifi: inconsitent NULL checking Dan Carpenter
2015-11-26 12:16 ` Johannes Berg
2015-11-26 12:37 ` Dan Carpenter
2015-11-26 12:51 ` Johannes Berg
2015-11-26 19:44 ` Dan Carpenter
2015-11-26 20:21 ` Johannes Berg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).