* [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
@ 2019-11-02 15:59 ` Christophe JAILLET
0 siblings, 0 replies; 9+ messages in thread
From: Christophe JAILLET @ 2019-11-02 15:59 UTC (permalink / raw)
To: jerome.pouiller, gregkh
Cc: devel, linux-kernel, kernel-janitors, Christophe JAILLET
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
@ 2019-11-02 15:59 ` Christophe JAILLET
0 siblings, 0 replies; 9+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
@ 2019-11-02 15:59 ` Christophe JAILLET
0 siblings, 0 replies; 9+ 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 related [flat|nested] 9+ messages in thread
* Re: [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
-1 siblings, 0 replies; 9+ messages in thread
From: Jerome Pouiller @ 2019-11-04 10:42 UTC (permalink / raw)
To: Christophe JAILLET; +Cc: gregkh, devel, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
@ 2019-11-04 10:42 ` Jerome Pouiller
0 siblings, 0 replies; 9+ 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
@ 2019-11-04 10:42 ` Jerome Pouiller
0 siblings, 0 replies; 9+ 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] 9+ 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
-1 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2019-11-04 11:03 UTC (permalink / raw)
To: Jerome Pouiller
Cc: Christophe JAILLET, devel, gregkh, kernel-janitors, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
@ 2019-11-04 11:03 ` Dan Carpenter
0 siblings, 0 replies; 9+ 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: wfx: Fix a memory leak in 'wfx_upload_beacon'
@ 2019-11-04 11:03 ` Dan Carpenter
0 siblings, 0 replies; 9+ 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] 9+ messages in thread
end of thread, other threads:[~2019-11-04 11:04 UTC | newest]
Thread overview: 9+ 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-02 15:59 ` Christophe JAILLET
2019-11-02 15:59 ` Christophe JAILLET
2019-11-04 10:42 ` Jerome Pouiller
2019-11-04 10:42 ` Jerome Pouiller
2019-11-04 10:42 ` Jerome Pouiller
2019-11-04 11:03 ` Dan Carpenter
2019-11-04 11:03 ` Dan Carpenter
2019-11-04 11:03 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.