(bug report) iwlwifi: inconsitent NULL checking

(bug report) iwlwifi: inconsitent NULL checking

[Dan Carpenter]

[ 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

Re: (bug report) iwlwifi: inconsitent NULL checking

[Johannes Berg]

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

Re: (bug report) iwlwifi: inconsitent NULL checking

[Dan Carpenter]

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

Re: (bug report) iwlwifi: inconsitent NULL checking

[Johannes Berg]

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

Re: (bug report) iwlwifi: inconsitent NULL checking

[Dan Carpenter]

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

Re: (bug report) iwlwifi: inconsitent NULL checking

[Johannes Berg]

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