Re: [PATCH] staging: Add rtl8723bs sdio wifi driver

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 04/04/2017 08:31 PM, Larry Finger wrote:
> On 03/29/2017 12:47 PM, Hans de Goede wrote:
>> The rtl8723bs is found on quite a few systems used by Linux users,
>> such as on Atom systems (Intel Computestick and various other
>> Atom based devices) and on many (budget) ARM boards such as
>> the CHIP.
>>
>> The plan moving forward with this is for the new clean,
>> written from scratch, rtl8xxxu driver to eventually gain
>> support for sdio devices. But there is no clear timeline
>> for that, so lets add this driver included in staging for now.
>
> Hans,
>
> I started looking at the Smatch errors. This one may be the result of a serious problem:
>
>   CHECK   drivers/staging/rtl8723bs/core/rtw_debug.c
> drivers/staging/rtl8723bs/core/rtw_debug.c:454 proc_get_survey_info() error: we previously assumed 'phead' could be null (see line 453)
> drivers/staging/rtl8723bs/core/rtw_debug.c:455 proc_get_survey_info() warn: variable dereferenced before check 'phead' (see line 454)
>
> A snippet of the code in question is as follows:
>
>         spin_lock_bh(&(pmlmepriv->scanned_queue.lock));
>         phead = get_list_head(queue);
>         plist = phead ? get_next(phead) : NULL;
>         plist = get_next(phead);
>         if ((!phead) || (!plist)) {
>                 spin_unlock_bh(&(pmlmepriv->scanned_queue.lock));
>                 return 0;
>         }
>
> This code comes directly from the hadess repo, but I am suspicious of the double call to get_next(phead). I cannot imagine any valid reason to skip every other entry on that list.

If you look closer and simplify the first getnext line what is written is:

          plist = get_next(phead);
          plist = get_next(phead);

Which indeed looks like it could use improvement, but I don't
think it is seriously broken.

Regards,

Hans