Re: Build regressions/improvements in v6.4 (wireless/airo)

[Randy Dunlap]

On 7/9/23 06:23, Geert Uytterhoeven wrote:
> Hi Randy,
> 
> On Sun, Jul 9, 2023 at 3:12 PM Randy Dunlap <rdunlap@xxxxxxxxxxxxx> wrote:
>> On 7/9/23 01:27, Geert Uytterhoeven wrote:
>>> On Sun, Jul 9, 2023 at 4:46 AM Randy Dunlap <rdunlap@xxxxxxxxxxxxx> wrote:
>>>> On 6/26/23 01:24, Geert Uytterhoeven wrote:
>>>>> On Mon, 26 Jun 2023, Geert Uytterhoeven wrote:
>>>>>> JFYI, when comparing v6.4[1] to v6.4-rc7[3], the summaries are:
>>>>>>  - build errors: +1/-0
>>>>>
>>>>>   + /kisskb/src/drivers/net/wireless/cisco/airo.c: error: 'status_rid.currentXmitRate' is used uninitialized [-Werror=uninitialized]:  => 6163:45
>>>>
>>>> I cannot reproduce this build error. (I don't doubt the problem, just having a problem
>>>> making it happen for me.)
>>>> I have tried it with gcc-11.1.0, gcc-11.3.0, and gcc-13.1.0.
>>>
>>> Which is similar to kisskb, using sh4-linux-gcc (GCC) 11.3.0...
>>>
>>>> I'm surprised that this is the only instance that gcc found/warned about.
>>>
>>> Indeed.
>>>
>>>>
>>>> I have a patch for this one instance, but there are 40+ instances of
>>>>         readStatusRid()
>>>>         readConfigRid()
>>>>         readSsidRid()
>>>>         readStatsRid()
>>>>         readCapabilityRid()
>>>> that don't check the return status of the function calls.
>>>>
>>>> I suppose that a patch can quieten the compiler error/warning, but given
>>>> the 40+ other problems, it won't make the driver any noticeably better IMO.
>>>
>>> Indeed...
>>>
>>>>> sh4-gcc11/sh-allmodconfig
>>>>> seen before
>>>>>
>>>>> This is actually a real issue, and it's been here since basically forever.
>>>>>
>>>>> drivers/net/wireless/cisco/airo.c:
>>>>>
>>>>>     static int airo_get_rate(struct net_device *dev,
>>>>>                              struct iw_request_info *info,
>>>>>                              union iwreq_data *wrqu,
>>>>>                              char *extra)
>>>>>     {
>>>>>             struct iw_param *vwrq = &wrqu->bitrate;
>>>>>             struct airo_info *local = dev->ml_priv;
>>>>>             StatusRid status_rid;           /* Card status info */
>>>>>
>>>>>             readStatusRid(local, &status_rid, 1);
>>>>>
>>>>> ==>         vwrq->value = le16_to_cpu(status_rid.currentXmitRate) * 500000;
>>>>>             ...
>>>>>     }
>>>>>
>>>>>     static int readStatusRid(struct airo_info *ai, StatusRid *statr, int lock)
>>>>>     {
>>>>>             return PC4500_readrid(ai, RID_STATUS, statr, sizeof(*statr), lock);
>>>>>     }
>>>>>
>>>>>     static int PC4500_readrid(struct airo_info *ai, u16 rid, void *pBuf, int len, int lock)
>>>>>     {
>>>>>             u16 status;
>>>>>             int rc = SUCCESS;
>>>>>
>>>>>             if (lock) {
>>>>>                     if (down_interruptible(&ai->sem))
>>>>>                             return ERROR;
>>>>>
>>>>> pBuf output buffer contents not initialized.
>>>>>
>>>>>             }
>>>>>             ...
>>>>>     }
>>>>>
>>>>>
>>>>>> [1] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/6995e2de6891c724bfeb2db33d7b87775f913ad1/ (all 160 configs)
>>>>>> [3] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/45a3e24f65e90a047bef86f927ebdc4c710edaa1/ (all 160 configs)
>>>>
>>>> I appreciate the synopsis.  Here's a patch.  WDYT?
>>>> ---
>>>> From: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
>>>> Subject: [PATCH] wifi: airo: avoid uninitialized warning in airo_get_rate()
>>>>
>>>> Quieten a gcc (11.3.0) build error or warning by checking the function
>>>> call status and returning -EIO if the function call failed.
>>>> This is similar to what several other wireless drivers do for the
>>>> SIOCGIWRATE ioctl call.
>>>>
>>>> drivers/net/wireless/cisco/airo.c: error: 'status_rid.currentXmitRate' is used uninitialized [-Werror=uninitialized]
>>>>
>>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>>> Signed-off-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
>>>> Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>>>> Link: lore.kernel.org/r/39abf2c7-24a-f167-91da-ed4c5435d1c4@xxxxxxxxxxxxxx
>>>> Cc: Kalle Valo <kvalo@xxxxxxxxxx>
>>>> Cc: linux-wireless@xxxxxxxxxxxxxxx
>>>> ---
>>>>  drivers/net/wireless/cisco/airo.c |    5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff -- a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
>>>> --- a/drivers/net/wireless/cisco/airo.c
>>>> +++ b/drivers/net/wireless/cisco/airo.c
>>>> @@ -6157,8 +6157,11 @@ static int airo_get_rate(struct net_devi
>>>>         struct iw_param *vwrq = &wrqu->bitrate;
>>>>         struct airo_info *local = dev->ml_priv;
>>>>         StatusRid status_rid;           /* Card status info */
>>>> +       int ret;
>>>>
>>>> -       readStatusRid(local, &status_rid, 1);
>>>> +       ret = readStatusRid(local, &status_rid, 1);
>>>> +       if (ret)
>>>> +               return -EIO;
>>>
>>> That's about the best we can do without further surgery.
>>> E.g. PC4500_readrid() should return a proper error code instead of
>>> just ERROR/SUCCESS.
>>> The case gcc complains about is when lock = 1 and the call to
>>> down_interruptible() fails, for which -EBUSY would be appropriate.
>>
>> Yes, I saw that return value as one of the options.
>> I'll change it to that and submit it.
> 
> Unfortunately that is not the only possible failure mode.
> But it is the one where gcc can _prove_ the output buffer is not initialized ;-)
> 
> OMG, PC4500_readrid() can also read from the passed buffer...

Yes, such messy fun. :(

-- 
~Randy