* re: rt2x00: Don't call ieee80211_get_tx_rate for MCS rates
@ 2012-07-19 11:33 Dan Carpenter
2012-07-19 11:46 ` Helmut Schaa
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2012-07-19 11:33 UTC (permalink / raw)
To: helmut.schaa; +Cc: linux-wireless
Hello Helmut Schaa,
I'm going through some old static checker warnings and I was concerned
about this one:
The patch 55b585e29095: "rt2x00: Don't call ieee80211_get_tx_rate for
MCS rates" from Mar 3, 2011, leads to the following warning:
drivers/net/wireless/rt2x00/rt2x00queue.c:508
rt2x00queue_create_tx_descriptor()
error: potential NULL dereference 'hwrate'.
482 /*
483 * Determine rate modulation.
484 */
485 if (txrate->flags & IEEE80211_TX_RC_GREEN_FIELD)
486 txdesc->rate_mode = RATE_MODE_HT_GREENFIELD;
hwrate is NULL here.
487 else if (txrate->flags & IEEE80211_TX_RC_MCS)
488 txdesc->rate_mode = RATE_MODE_HT_MIX;
489 else {
490 rate = ieee80211_get_tx_rate(rt2x00dev->hw, tx_info);
491 hwrate = rt2x00_get_rate(rate->hw_value);
492 if (hwrate->flags & DEV_RATE_OFDM)
493 txdesc->rate_mode = RATE_MODE_OFDM;
494 else
495 txdesc->rate_mode = RATE_MODE_CCK;
496 }
497
498 /*
499 * Apply TX descriptor handling by components
500 */
501 rt2x00crypto_create_tx_descriptor(rt2x00dev, skb, txdesc);
502 rt2x00queue_create_tx_descriptor_seq(rt2x00dev, skb, txdesc);
503
504 if (test_bit(REQUIRE_HT_TX_DESC, &rt2x00dev->cap_flags))
505 rt2x00queue_create_tx_descriptor_ht(rt2x00dev, skb, txdesc,
506 hwrate);
On this path we dereference dereference hwrate if IEEE80211_TX_RC_MCS is
not set, but we don't check for IEEE80211_TX_RC_GREEN_FIELD.
507 else
508 rt2x00queue_create_tx_descriptor_plcp(rt2x00dev, skb, txdesc,
509 hwrate);
On this patch we dereference hwrate unconditionally.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: rt2x00: Don't call ieee80211_get_tx_rate for MCS rates
2012-07-19 11:33 rt2x00: Don't call ieee80211_get_tx_rate for MCS rates Dan Carpenter
@ 2012-07-19 11:46 ` Helmut Schaa
2012-07-20 7:28 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Helmut Schaa @ 2012-07-19 11:46 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-wireless
Hi Dan,
On Thu, Jul 19, 2012 at 1:33 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Helmut Schaa,
>
> I'm going through some old static checker warnings and I was concerned
> about this one:
>
> The patch 55b585e29095: "rt2x00: Don't call ieee80211_get_tx_rate for
> MCS rates" from Mar 3, 2011, leads to the following warning:
> drivers/net/wireless/rt2x00/rt2x00queue.c:508
> rt2x00queue_create_tx_descriptor()
> error: potential NULL dereference 'hwrate'.
>
> 482 /*
> 483 * Determine rate modulation.
> 484 */
> 485 if (txrate->flags & IEEE80211_TX_RC_GREEN_FIELD)
> 486 txdesc->rate_mode = RATE_MODE_HT_GREENFIELD;
>
> hwrate is NULL here.
>
> 487 else if (txrate->flags & IEEE80211_TX_RC_MCS)
> 488 txdesc->rate_mode = RATE_MODE_HT_MIX;
> 489 else {
> 490 rate = ieee80211_get_tx_rate(rt2x00dev->hw, tx_info);
> 491 hwrate = rt2x00_get_rate(rate->hw_value);
> 492 if (hwrate->flags & DEV_RATE_OFDM)
> 493 txdesc->rate_mode = RATE_MODE_OFDM;
> 494 else
> 495 txdesc->rate_mode = RATE_MODE_CCK;
> 496 }
> 497
> 498 /*
> 499 * Apply TX descriptor handling by components
> 500 */
> 501 rt2x00crypto_create_tx_descriptor(rt2x00dev, skb, txdesc);
> 502 rt2x00queue_create_tx_descriptor_seq(rt2x00dev, skb, txdesc);
> 503
> 504 if (test_bit(REQUIRE_HT_TX_DESC, &rt2x00dev->cap_flags))
> 505 rt2x00queue_create_tx_descriptor_ht(rt2x00dev, skb, txdesc,
> 506 hwrate);
>
> On this path we dereference dereference hwrate if IEEE80211_TX_RC_MCS is
> not set, but we don't check for IEEE80211_TX_RC_GREEN_FIELD.
Right, I guess the basic assumption is that IEEE80211_TX_RC_GREEN_FIELD is
only used in conjunction with IEEE80211_TX_RC_MCS.
>
> 507 else
> 508 rt2x00queue_create_tx_descriptor_plcp(rt2x00dev, skb, txdesc,
> 509 hwrate);
>
> On this patch we dereference hwrate unconditionally.
mac80211 should never request a MCS rate for devices that don't register
MCS rate support.
I think we could make the static checker happy by moving the hwrate
dereferencing
into rt2x00queue_create_tx_descriptor_plcp and
rt2x00queue_create_tx_descriptor_ht?
Any better ideas?
Helmut
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: rt2x00: Don't call ieee80211_get_tx_rate for MCS rates
2012-07-19 11:46 ` Helmut Schaa
@ 2012-07-20 7:28 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2012-07-20 7:28 UTC (permalink / raw)
To: Helmut Schaa; +Cc: linux-wireless
On Thu, Jul 19, 2012 at 01:46:15PM +0200, Helmut Schaa wrote:
> Hi Dan,
>
> On Thu, Jul 19, 2012 at 1:33 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Hello Helmut Schaa,
> >
> > I'm going through some old static checker warnings and I was concerned
> > about this one:
> >
> > The patch 55b585e29095: "rt2x00: Don't call ieee80211_get_tx_rate for
> > MCS rates" from Mar 3, 2011, leads to the following warning:
> > drivers/net/wireless/rt2x00/rt2x00queue.c:508
> > rt2x00queue_create_tx_descriptor()
> > error: potential NULL dereference 'hwrate'.
> >
> > 482 /*
> > 483 * Determine rate modulation.
> > 484 */
> > 485 if (txrate->flags & IEEE80211_TX_RC_GREEN_FIELD)
> > 486 txdesc->rate_mode = RATE_MODE_HT_GREENFIELD;
> >
> > hwrate is NULL here.
> >
> > 487 else if (txrate->flags & IEEE80211_TX_RC_MCS)
> > 488 txdesc->rate_mode = RATE_MODE_HT_MIX;
> > 489 else {
> > 490 rate = ieee80211_get_tx_rate(rt2x00dev->hw, tx_info);
> > 491 hwrate = rt2x00_get_rate(rate->hw_value);
> > 492 if (hwrate->flags & DEV_RATE_OFDM)
> > 493 txdesc->rate_mode = RATE_MODE_OFDM;
> > 494 else
> > 495 txdesc->rate_mode = RATE_MODE_CCK;
> > 496 }
> > 497
> > 498 /*
> > 499 * Apply TX descriptor handling by components
> > 500 */
> > 501 rt2x00crypto_create_tx_descriptor(rt2x00dev, skb, txdesc);
> > 502 rt2x00queue_create_tx_descriptor_seq(rt2x00dev, skb, txdesc);
> > 503
> > 504 if (test_bit(REQUIRE_HT_TX_DESC, &rt2x00dev->cap_flags))
> > 505 rt2x00queue_create_tx_descriptor_ht(rt2x00dev, skb, txdesc,
> > 506 hwrate);
> >
> > On this path we dereference dereference hwrate if IEEE80211_TX_RC_MCS is
> > not set, but we don't check for IEEE80211_TX_RC_GREEN_FIELD.
>
> Right, I guess the basic assumption is that IEEE80211_TX_RC_GREEN_FIELD is
> only used in conjunction with IEEE80211_TX_RC_MCS.
>
> >
> > 507 else
> > 508 rt2x00queue_create_tx_descriptor_plcp(rt2x00dev, skb, txdesc,
> > 509 hwrate);
> >
> > On this patch we dereference hwrate unconditionally.
>
> mac80211 should never request a MCS rate for devices that don't register
> MCS rate support.
>
> I think we could make the static checker happy by moving the hwrate
> dereferencing
> into rt2x00queue_create_tx_descriptor_plcp and
> rt2x00queue_create_tx_descriptor_ht?
>
> Any better ideas?
If IEEE80211_TX_RC_GREEN_FIELD implies IEEE80211_TX_RC_MCS and
REQUIRE_HT_TX_DESC are set then the code is fine as is.
I'm confused by your suggestion, because the "hwrate" dereferencing
is already inside the functions. Smatch only cares about the call
to rt2x00queue_create_tx_descriptor_plcp() because that is an
unconditional dereference. But when I was reviewing the error
message I looked at the dereference inside
rt2x00queue_create_tx_descriptor_ht() as well.
Anyway, we shouldn't ever make the code less readable for static
checkers so lets just leave it as is.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-07-20 7:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-19 11:33 rt2x00: Don't call ieee80211_get_tx_rate for MCS rates Dan Carpenter
2012-07-19 11:46 ` Helmut Schaa
2012-07-20 7:28 ` Dan Carpenter
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).