From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.6 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0D008C433DB for ; Fri, 5 Feb 2021 18:23:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C901864FD0 for ; Fri, 5 Feb 2021 18:23:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233165AbhBEQki (ORCPT ); Fri, 5 Feb 2021 11:40:38 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:40566 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233231AbhBEQic (ORCPT ); Fri, 5 Feb 2021 11:38:32 -0500 Received: from 1.general.cking.uk.vpn ([10.172.193.212]) by youngberry.canonical.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1l85iG-0006ge-1Y; Fri, 05 Feb 2021 18:20:12 +0000 Subject: Re: Potential invalid ~ operator in net/mac80211/cfg.c From: Colin Ian King To: Johannes Berg Cc: "David S. Miller" , Jakub Kicinski , "linux-wireless@vger.kernel.org" , "netdev@vger.kernel.org" References: <4bb65f2f-48f9-7d9c-ab2e-15596f15a4d8@canonical.com> <15f435a791b0c4b853c8c6b284042c7057d6efaf.camel@sipsolutions.net> <1383c6f1-1317-daed-ecc7-e5cc3f309c41@canonical.com> Message-ID: <86c1e5aa-459d-6d76-69e4-f7bc177214bf@canonical.com> Date: Fri, 5 Feb 2021 18:20:11 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <1383c6f1-1317-daed-ecc7-e5cc3f309c41@canonical.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 >> >