* [PATCH v3] staging: ks7010: Add null pointer check for skb @ 2018-09-27 15:16 Aymen Qader 2018-09-27 18:04 ` Aymen Qader 2018-09-28 8:13 ` Dan Carpenter 0 siblings, 2 replies; 6+ messages in thread From: Aymen Qader @ 2018-09-27 15:16 UTC (permalink / raw) Cc: Wolfram Sang, Aymen Qader, Greg Kroah-Hartman, devel, linux-kernel Add a null pointer check for the socket buffer in ks_hostif.c to avoid a possible null pointer deference, and remove a later now-redundant null pointer check. Signed-off-by: Aymen Qader <qader.aymen@gmail.com> --- v2: Remove redundant pointer check v3: Style fix drivers/staging/ks7010/ks_hostif.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c index 0e554e3359b5..95b6c7557e84 100644 --- a/drivers/staging/ks7010/ks_hostif.c +++ b/drivers/staging/ks7010/ks_hostif.c @@ -1011,6 +1011,11 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *skb) size_t size; int ret; + if (!skb) { + ret = -ENOMEM; + goto err_kfree; + } + skb_len = skb->len; if (skb_len > ETH_FRAME_LEN) { netdev_err(priv->net_dev, "bad length skb_len=%d\n", skb_len); @@ -1023,7 +1028,6 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *skb) priv->wpa.mic_failure.stop) { if (netif_queue_stopped(priv->net_dev)) netif_wake_queue(priv->net_dev); - if (skb) dev_kfree_skb(skb); return 0; -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] staging: ks7010: Add null pointer check for skb 2018-09-27 15:16 [PATCH v3] staging: ks7010: Add null pointer check for skb Aymen Qader @ 2018-09-27 18:04 ` Aymen Qader 2018-09-28 8:22 ` Dan Carpenter 2018-09-28 8:13 ` Dan Carpenter 1 sibling, 1 reply; 6+ messages in thread From: Aymen Qader @ 2018-09-27 18:04 UTC (permalink / raw) To: Wolfram Sang, Greg Kroah-Hartman, devel, linux-kernel 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. On Thu, Sep 27, 2018 at 04:16:13PM +0100, Aymen Qader wrote: > Add a null pointer check for the socket buffer in ks_hostif.c to avoid a > possible null pointer deference, and remove a later now-redundant null > pointer check. > > Signed-off-by: Aymen Qader <qader.aymen@gmail.com> > --- > v2: Remove redundant pointer check > v3: Style fix > > drivers/staging/ks7010/ks_hostif.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c > index 0e554e3359b5..95b6c7557e84 100644 > --- a/drivers/staging/ks7010/ks_hostif.c > +++ b/drivers/staging/ks7010/ks_hostif.c > @@ -1011,6 +1011,11 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *skb) > size_t size; > int ret; > > + if (!skb) { > + ret = -ENOMEM; > + goto err_kfree; > + } > + > skb_len = skb->len; > if (skb_len > ETH_FRAME_LEN) { > netdev_err(priv->net_dev, "bad length skb_len=%d\n", skb_len); > @@ -1023,7 +1028,6 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *skb) > priv->wpa.mic_failure.stop) { > if (netif_queue_stopped(priv->net_dev)) > netif_wake_queue(priv->net_dev); > - if (skb) > dev_kfree_skb(skb); > > return 0; > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] staging: ks7010: Add null pointer check for skb 2018-09-27 18:04 ` Aymen Qader @ 2018-09-28 8:22 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2018-09-28 8:22 UTC (permalink / raw) To: Aymen Qader; +Cc: Wolfram Sang, Greg Kroah-Hartman, devel, linux-kernel 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). 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] staging: ks7010: Add null pointer check for skb 2018-09-27 15:16 [PATCH v3] staging: ks7010: Add null pointer check for skb Aymen Qader 2018-09-27 18:04 ` Aymen Qader @ 2018-09-28 8:13 ` Dan Carpenter 2018-09-28 8:16 ` Dan Carpenter 1 sibling, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2018-09-28 8:13 UTC (permalink / raw) To: Aymen Qader; +Cc: devel, Greg Kroah-Hartman, linux-kernel, Wolfram Sang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] staging: ks7010: Add null pointer check for skb 2018-09-28 8:13 ` Dan Carpenter @ 2018-09-28 8:16 ` Dan Carpenter 2018-09-28 8:48 ` Aymen Qader 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2018-09-28 8:16 UTC (permalink / raw) To: Aymen Qader; +Cc: devel, Greg Kroah-Hartman, linux-kernel, Wolfram Sang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] staging: ks7010: Add null pointer check for skb 2018-09-28 8:16 ` Dan Carpenter @ 2018-09-28 8:48 ` Aymen Qader 0 siblings, 0 replies; 6+ messages in thread From: Aymen Qader @ 2018-09-28 8:48 UTC (permalink / raw) To: Dan Carpenter; +Cc: Wolfram Sang, Greg Kroah-Hartman, devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-28 8:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-27 15:16 [PATCH v3] staging: ks7010: Add null pointer check for skb Aymen Qader 2018-09-27 18:04 ` Aymen Qader 2018-09-28 8:22 ` Dan Carpenter 2018-09-28 8:13 ` Dan Carpenter 2018-09-28 8:16 ` Dan Carpenter 2018-09-28 8:48 ` Aymen Qader
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®