DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
@ 2019-11-02 15:59 Christophe JAILLET
  2019-11-04 10:42 ` Jerome Pouiller
  0 siblings, 1 reply; 3+ messages in thread
From: Christophe JAILLET @ 2019-11-02 15:59 UTC (permalink / raw)
  To: jerome.pouiller, gregkh
  Cc: devel, Christophe JAILLET, kernel-janitors, linux-kernel

The current code is a no-op, because all it can do is 'dev_kfree_skb(NULL)'
Remove the test before 'dev_kfree_skb()'

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
V2: remove the 'if(...)', 'dev_kfree_skb()' can handle NULL.
---
 drivers/staging/wfx/sta.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 688586e823c0..93f3739b5f3a 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -906,8 +906,7 @@ static int wfx_upload_beacon(struct wfx_vif *wvif)
 	wfx_fwd_probe_req(wvif, false);
 
 done:
-	if (!skb)
-		dev_kfree_skb(skb);
+	dev_kfree_skb(skb);
 	return ret;
 }
 
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
  2019-11-02 15:59 [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon' Christophe JAILLET
@ 2019-11-04 10:42 ` Jerome Pouiller
  2019-11-04 11:03   ` Dan Carpenter
  0 siblings, 1 reply; 3+ messages in thread
From: Jerome Pouiller @ 2019-11-04 10:42 UTC (permalink / raw)
  To: Christophe JAILLET; +Cc: devel, gregkh, kernel-janitors, linux-kernel

On Saturday 2 November 2019 16:59:45 CET Christophe JAILLET wrote:
> The current code is a no-op, because all it can do is 'dev_kfree_skb(NULL)'
> Remove the test before 'dev_kfree_skb()'
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> V2: remove the 'if(...)', 'dev_kfree_skb()' can handle NULL.
> ---
>  drivers/staging/wfx/sta.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 688586e823c0..93f3739b5f3a 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -906,8 +906,7 @@ static int wfx_upload_beacon(struct wfx_vif *wvif)
>         wfx_fwd_probe_req(wvif, false);
> 
>  done:
> -       if (!skb)
> -               dev_kfree_skb(skb);
> +       dev_kfree_skb(skb);
>         return ret;
>  }
> 
> --
> 2.20.1
> 

In add, value of skb is tested earlier in function. So, it is guaranteed to be 
never NULL.

Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

-- 
Jérôme Pouiller

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
  2019-11-04 10:42 ` Jerome Pouiller
@ 2019-11-04 11:03   ` Dan Carpenter
  0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2019-11-04 11:03 UTC (permalink / raw)
  To: Jerome Pouiller
  Cc: devel, gregkh, kernel-janitors, Christophe JAILLET, linux-kernel

On Mon, Nov 04, 2019 at 10:42:43AM +0000, Jerome Pouiller wrote:
> On Saturday 2 November 2019 16:59:45 CET Christophe JAILLET wrote:
> > The current code is a no-op, because all it can do is 'dev_kfree_skb(NULL)'
> > Remove the test before 'dev_kfree_skb()'
> > 
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > V2: remove the 'if(...)', 'dev_kfree_skb()' can handle NULL.
> > ---
> >  drivers/staging/wfx/sta.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> > index 688586e823c0..93f3739b5f3a 100644
> > --- a/drivers/staging/wfx/sta.c
> > +++ b/drivers/staging/wfx/sta.c
> > @@ -906,8 +906,7 @@ static int wfx_upload_beacon(struct wfx_vif *wvif)
> >         wfx_fwd_probe_req(wvif, false);
> > 
> >  done:
> > -       if (!skb)
> > -               dev_kfree_skb(skb);
> > +       dev_kfree_skb(skb);
> >         return ret;
> >  }
> > 
> > --
> > 2.20.1
> > 
> 
> In add, value of skb is tested earlier in function. So, it is guaranteed to be 
> never NULL.

See the start of the function:

   868  static int wfx_upload_beacon(struct wfx_vif *wvif)
   869  {
   870          int ret = 0;
   871          struct sk_buff *skb = NULL;
   872          struct ieee80211_mgmt *mgmt;
   873          struct hif_mib_template_frame *p;
   874  
   875          if (wvif->vif->type == NL80211_IFTYPE_STATION ||
   876              wvif->vif->type == NL80211_IFTYPE_MONITOR ||
   877              wvif->vif->type == NL80211_IFTYPE_UNSPECIFIED)
   878                  goto done;
                        ^^^^^^^^^
This why I argue that goto done is monkey poop.  You see this and you
wonder "what on earth does goto done do?"  We haven't allocated anything
so what are we going to free?  The name doesn't give any hints at all.
So you have to scroll down.  Now you have lost your place and cleared
out your short term memory about the code that you were reading.  But
then you get to goto done and you see it is a complicated no op, and it
return ret.  So you have to scroll back up to the very start of the
function to see what ret is.

And the you wonder, is this really a success path?  It could be
deliberate or it could be accidental.  Who knows!  The "goto done;" is
ambiguous and take a long time to understand but "return 0;" is
instantly clear.

   879  
   880          skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
   881  
   882          if (!skb)
   883                  return -ENOMEM;
   884  
   885          p = (struct hif_mib_template_frame *) skb_push(skb, 4);

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-02 15:59 [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon' Christophe JAILLET
2019-11-04 10:42 ` Jerome Pouiller
2019-11-04 11:03   ` Dan Carpenter

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git