Date:   Fri, 28 Sep 2018 09:48:34 +0100
From:   Aymen Qader <qader.aymen@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Wolfram Sang <wsa@...-dreams.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] staging: ks7010: Add null pointer check for skb

On Fri, Sep 28, 2018 at 11:13:25AM +0300, Dan Carpenter wrote:
> You might want to try running Smatch on your patches.  This is the
> second one where maybe the results would have been interesting.
> 
> git clone http://repo.or.cz/w/smatch.git
> cd smatch
> make
> cd ~/kernel/src/
> ~/smatch/smatch_scripts/kchecker drivers/staging/ks7010/ks_hostif.c
> 
> This patch introduced a problem, but the earlier one fixed a potential
> problem in rtl8723bs:
> 
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:1274 OnAssocReq() error: uninitialized symbol 'ie_len'.
> 
> Unfortunately, Smatch missed the potential NULL dereference in
> OnAssocReq()...  It wouldn't be that hard to fix...
> 
> regards,
> dan carpenter

Oh thanks, I've not used Smatch before--that's really helpful, I'm going
to use that a lot in the future.

On Fri, Sep 28, 2018 at 11:16:01AM +0300, Dan Carpenter wrote:
> Btw, if you have the cross function DB built then Smatch says that the
> NULL check can be removed here no problem.
> 
> $ smdb hostif_data_request
> [ snip ]
> drivers/staging/ks7010/ks_wlan_net.c |   ks_wlan_start_xmit |  hostif_data_request |  PARAM_VALUE |  1 | skb | s64min-(-1),1-s64max
> 
> regards,
> dan carpenter
> 

Again, thanks very much! That's really cool, Smatch looks like a great
tool, I wish I found it before!

On Fri, Sep 28, 2018 at 11:22:39AM +0300, Dan Carpenter wrote:
> On Thu, Sep 27, 2018 at 07:04:43PM +0100, Aymen Qader wrote:
> > Retraction: in hindsight I see that with the current usage of this
> > function, there is already a check for the socket buffer so this check
> > is unnecessary. However, I'm not sure if it's considered good practice
> > to keep this check anyway--in any case, ENOMEM isn't the right error
> > to return.
> 
> When we find inconsistent NULL checks then we fix it to make sense.
> Generally, we prefer a minimal style, with no extra code for future
> proofing.  (The future seldom goes the way you expect and those extra
> NULL checks would be easy to add back).

Hm I understand, so if a parameter can currently never be null there's
no need to add unnecessary checks for in case that changes in the
future. I'll be sure to keep that in mind from now on.

> 
> So, yes, do remove the NULL check but also fix the indenting while
> you're at it.
> 
> Take your time to write patches.  I write them, then I sit on them over
> night then I send them in the morning.  It means that sometimes other
> people have already sent it but that's fine.  If you have to redo a
> patch then don't send the v2 patch on the same day.  v2 patches are
> stressful and you imagine that everyone is waiting for you to send it
> or something.  We are not waiting for you.  We don't care if you wait
> until next week to send these...
> 
> So when you write a v2 patch wait until the next day to send it.  Then
> you will be calm when you review it.
> 
> regards,
> dan carpenter
>  

Thank you very much for all of these tips, I really appreciate you
taking the time. I'm definitely going to try and apply this philosophy
in the future, I think I've been too impulsive with my patches so far.
I've been thinking "oh no, I made a mistake--I need to quickly send a
v2!" and then end up making even more mistakes, so I completely
understand what you mean.

Thanks again.

Kind regards,
Aymen Qader

Powered by blists - more mailing lists