From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-oi0-f66.google.com ([209.85.218.66]:36714 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752770AbdDDXlE (ORCPT ); Tue, 4 Apr 2017 19:41:04 -0400 Received: by mail-oi0-f66.google.com with SMTP id b187so27282732oif.3 for ; Tue, 04 Apr 2017 16:41:04 -0700 (PDT) Subject: Re: [PATCH] staging: Add rtl8723bs sdio wifi driver To: Hans de Goede , Arend Van Spriel References: <20170329174751.13184-1-hdegoede@redhat.com> <4d1d3c02-eff6-0410-cee0-9360ec645e13@lwfinger.net> <049673a4-647b-a270-bb9c-15bb2ab49fc4@redhat.com> Cc: Bastien Nocera , Jes Sorensen , linux-wireless@vger.kernel.org From: Larry Finger Message-ID: (sfid-20170405_014132_814274_891C83B6) Date: Tue, 4 Apr 2017 18:41:02 -0500 MIME-Version: 1.0 In-Reply-To: <049673a4-647b-a270-bb9c-15bb2ab49fc4@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 04/04/2017 04:53 PM, Hans de Goede wrote: > Hi, > > On 04/04/2017 11:38 PM, Arend Van Spriel wrote: >> >> >> On 4-4-2017 20:53, Hans de Goede wrote: >>> 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. >> >> So get_list_head(queue) can never return NULL? > > I don't know. > >> Otherwise I don't know >> how close I need to get for a simplified look ;-) > > I simplified things so as to make clear that Larry's worry was > not really a problem, I do agree the smatch complaint is valid. I think the following fixes the problem: diff --git a/drivers/staging/rtl8723bs/core/rtw_debug.c b/drivers/staging/rtl8723bs/core/rtw_debug.c index d34747b29309..51cef55d3f76 100644 --- a/drivers/staging/rtl8723bs/core/rtw_debug.c +++ b/drivers/staging/rtl8723bs/core/rtw_debug.c @@ -451,7 +451,6 @@ int proc_get_survey_info(struct seq_file *m, void *v) 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; Pointer plist is set in the 3rd line, thus the second set of it can be removed. Larry