Potential invalid ~ operator in net/mac80211/cfg.c

Potential invalid ~ operator in net/mac80211/cfg.c

[Colin Ian King]

Hi there,

while working through a backlog of older static analysis reports from
Coverity I found an interesting use of the ~ operator that looks
incorrect to me in function ieee80211_set_bitrate_mask():

                for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) {
                        if (~sdata->rc_rateidx_mcs_mask[i][j]) {
                                sdata->rc_has_mcs_mask[i] = true;
                                break;
                        }
                }

                for (j = 0; j < NL80211_VHT_NSS_MAX; j++) {
                        if (~sdata->rc_rateidx_vht_mcs_mask[i][j]) {
                                sdata->rc_has_vht_mcs_mask[i] = true;
                                break;
                        }
                }

For the ~ operator in both if stanzas, Coverity reports:

Logical vs. bitwise operator (CONSTANT_EXPRESSION_RESULT)
logical_vs_bitwise:

~sdata->rc_rateidx_mcs_mask[i][j] is always 1/true regardless of the
values of its operand. This occurs as the logical operand of if.
    Did you intend to use ! rather than ~?

I've checked the results of this and it does seem that ~ is incorrect
and always returns true for the if expression. So it probably should be
!, but I'm not sure if I'm missing something deeper here and wondering
why this has always worked.

Colin

Re: Potential invalid ~ operator in net/mac80211/cfg.c

[Johannes Berg]

Hi Colin,

> while working through a backlog of older static analysis reports from
> Coverity

So ... yeah. Every time I look at Coverity (not frequently, I must
admit) I see the same thing, and get confused.

> I found an interesting use of the ~ operator that looks
> incorrect to me in function ieee80211_set_bitrate_mask():
> 
>                 for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) {
>                         if (~sdata->rc_rateidx_mcs_mask[i][j]) {
>                                 sdata->rc_has_mcs_mask[i] = true;
>                                 break;
>                         }
>                 }
> 
>                 for (j = 0; j < NL80211_VHT_NSS_MAX; j++) {
>                         if (~sdata->rc_rateidx_vht_mcs_mask[i][j]) {
>                                 sdata->rc_has_vht_mcs_mask[i] = true;
>                                 break;
>                         }
>                 }
> 
> For the ~ operator in both if stanzas, Coverity reports:
> 
> Logical vs. bitwise operator (CONSTANT_EXPRESSION_RESULT)
> logical_vs_bitwise:
> 
> ~sdata->rc_rateidx_mcs_mask[i][j] is always 1/true regardless of the
> values of its operand. This occurs as the logical operand of if.
>     Did you intend to use ! rather than ~?
> 
> I've checked the results of this and it does seem that ~ is incorrect
> and always returns true for the if expression. So it probably should be
> !, but I'm not sure if I'm missing something deeper here and wondering
> why this has always worked.

But is it really always true?

I _think_ it was intended to check that it's not 0xffffffff or
something?

https://lore.kernel.org/linux-wireless/516C0C7F.3000204@openwrt.org/

But maybe that isn't actually quite right due to integer promotion?
OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
to 0 also?

johannes

Re: Potential invalid ~ operator in net/mac80211/cfg.c

[Colin Ian King]

On 05/02/2021 18:05, Johannes Berg wrote:
> Hi Colin,
> 
>> while working through a backlog of older static analysis reports from
>> Coverity
> 
> So ... yeah. Every time I look at Coverity (not frequently, I must
> admit) I see the same thing, and get confused.
> 
>> I found an interesting use of the ~ operator that looks
>> incorrect to me in function ieee80211_set_bitrate_mask():
>>
>>                 for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) {
>>                         if (~sdata->rc_rateidx_mcs_mask[i][j]) {
>>                                 sdata->rc_has_mcs_mask[i] = true;
>>                                 break;
>>                         }
>>                 }
>>
>>                 for (j = 0; j < NL80211_VHT_NSS_MAX; j++) {
>>                         if (~sdata->rc_rateidx_vht_mcs_mask[i][j]) {
>>                                 sdata->rc_has_vht_mcs_mask[i] = true;
>>                                 break;
>>                         }
>>                 }
>>
>> For the ~ operator in both if stanzas, Coverity reports:
>>
>> Logical vs. bitwise operator (CONSTANT_EXPRESSION_RESULT)
>> logical_vs_bitwise:
>>
>> ~sdata->rc_rateidx_mcs_mask[i][j] is always 1/true regardless of the
>> values of its operand. This occurs as the logical operand of if.
>>     Did you intend to use ! rather than ~?
>>
>> I've checked the results of this and it does seem that ~ is incorrect
>> and always returns true for the if expression. So it probably should be
>> !, but I'm not sure if I'm missing something deeper here and wondering
>> why this has always worked.
> 
> But is it really always true?
> 
> I _think_ it was intended to check that it's not 0xffffffff or
> something?
> 
> https://lore.kernel.org/linux-wireless/516C0C7F.3000204@openwrt.org/
> 
> But maybe that isn't actually quite right due to integer promotion?
> OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
> to 0 also?

rc_rateidx_vht_mcs_mask is a u64, so I think the expression could be
expressed as:

if ((uint16_t)~sdata->rc_rateidx_mcs_mask[i][j]) ..

this is only true if all the 16 bits in the mask are 0xffff

> 
> johannes
> 

Re: Potential invalid ~ operator in net/mac80211/cfg.c

[Colin Ian King]

On 05/02/2021 18:19, Colin Ian King wrote:
> On 05/02/2021 18:05, Johannes Berg wrote:
>> Hi Colin,
>>
>>> while working through a backlog of older static analysis reports from
>>> Coverity
>>
>> So ... yeah. Every time I look at Coverity (not frequently, I must
>> admit) I see the same thing, and get confused.
>>
>>> I found an interesting use of the ~ operator that looks
>>> incorrect to me in function ieee80211_set_bitrate_mask():
>>>
>>>                 for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) {
>>>                         if (~sdata->rc_rateidx_mcs_mask[i][j]) {
>>>                                 sdata->rc_has_mcs_mask[i] = true;
>>>                                 break;
>>>                         }
>>>                 }
>>>
>>>                 for (j = 0; j < NL80211_VHT_NSS_MAX; j++) {
>>>                         if (~sdata->rc_rateidx_vht_mcs_mask[i][j]) {
>>>                                 sdata->rc_has_vht_mcs_mask[i] = true;
>>>                                 break;
>>>                         }
>>>                 }
>>>
>>> For the ~ operator in both if stanzas, Coverity reports:
>>>
>>> Logical vs. bitwise operator (CONSTANT_EXPRESSION_RESULT)
>>> logical_vs_bitwise:
>>>
>>> ~sdata->rc_rateidx_mcs_mask[i][j] is always 1/true regardless of the
>>> values of its operand. This occurs as the logical operand of if.
>>>     Did you intend to use ! rather than ~?
>>>
>>> I've checked the results of this and it does seem that ~ is incorrect
>>> and always returns true for the if expression. So it probably should be
>>> !, but I'm not sure if I'm missing something deeper here and wondering
>>> why this has always worked.
>>
>> But is it really always true?
>>
>> I _think_ it was intended to check that it's not 0xffffffff or
>> something?
>>
>> https://lore.kernel.org/linux-wireless/516C0C7F.3000204@openwrt.org/
>>
>> But maybe that isn't actually quite right due to integer promotion?
>> OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
>> to 0 also?
> 
> rc_rateidx_vht_mcs_mask is a u64, so I think the expression could be
> expressed as:

oops, fat fingered that, it is a u16 not a u64

> 
> if ((uint16_t)~sdata->rc_rateidx_mcs_mask[i][j]) ..
> 
> this is only true if all the 16 bits in the mask are 0xffff
> 
>>
>> johannes
>>
> 

Re: Potential invalid ~ operator in net/mac80211/cfg.c

[Johannes Berg]

On Fri, 2021-02-05 at 18:20 +0000, Colin Ian King wrote:
> 
> > > https://lore.kernel.org/linux-wireless/516C0C7F.3000204@openwrt.org/
> > > 
> > > But maybe that isn't actually quite right due to integer promotion?
> > > OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
> > > to 0 also?
> > 
> > rc_rateidx_vht_mcs_mask is a u64, so I think the expression could be
> > expressed as:
> 
> oops, fat fingered that, it is a u16 not a u64

Right, u16, I must've looked at some ancient version or something.

But no, I was obviously wrong with what I said above.

So of course the condition is always true, like you said.

However, what was intended doesn't look like !, but rather == 0xff and
== 0xffff respectively, I'll send a patch.

johannes