Re: [PATCH] libertas: skb dereferenced after netif_rx

Re: [PATCH] libertas: skb dereferenced after netif_rx

[John W. Linville]

On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> In libertas_process_rxed_packet() and process_rxed_802_11_packet() the 
> skb is dereferenced after being passed to netif_rx (called from 
> libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
 
Relocating the libertas_upload_rx_packet call is fine, but...

> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error 
> check is dead code - might as well take it out.

Is this merely an implementation detail?  Or an absolute fact?
If the former is true, then we should preserve the error
checking.  If the latter, then we should change the signature of
libertas_upload_rx_packet to return void.

> Signed-off-by: Florin Malita <fmalita@gmail.com>

> 	lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
> -	if (libertas_upload_rx_packet(priv, skb)) {
> -		lbs_pr_debug(1, "RX error: libertas_upload_rx_packet"
> -		       " returns failure\n");
> -		ret = -1;
> -		goto done;
> -	}
> 	priv->stats.rx_bytes += skb->len;
> 	priv->stats.rx_packets++;
> 
> +	libertas_upload_rx_packet(priv, skb);
> +
> 	ret = 0;
> done:
> 	LEAVE();

Another potential patch is to remove the "ret = 0" line before the
"done" label, since ret is initialized at the head of the function.
Come to think of it, you can probably remove the "= 0" part of ret's
declaration as well (in both functions).

Hth!

John

P.S.  Also, please make sure to send wireless patches to
linux-wireless@vger.kernel.org and CC me.
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] libertas: skb dereferenced after netif_rx

[Florin Malita]

John W. Linville wrote:
>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error 
>> check is dead code - might as well take it out.
>>     
>
> Is this merely an implementation detail?  Or an absolute fact?
>   

I believe it's an absolute fact that got lost among implementation 
details ;)

All libertas_upload_rx_packet does is set a few fields in the skb, then 
pass it up to the stack via netif_rx:

139 int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
140 {
141         lbs_pr_debug(1, "skb->data=%p\n", skb->data);
142
143         if(IS_MESH_FRAME(skb))
144                 skb->dev = priv->mesh_dev;
145         else
146                 skb->dev = priv->wlan_dev.netdev;
147         skb->protocol = eth_type_trans(skb, priv->wlan_dev.netdev);
148         skb->ip_summed = CHECKSUM_UNNECESSARY;
149
150         netif_rx(skb);
151
152         return 0;
153 }


Since netif_rx always succeeds, so should libertas_upload_rx_packet - 
there's no reason for passing back a success code (especially one that's 
hardcoded to 0).

> If the latter, then we should change the signature of
> libertas_upload_rx_packet to return void.
>   

Makes sense, updated patch below.

> Another potential patch is to remove the "ret = 0" line before the
> "done" label, since ret is initialized at the head of the function.
> Come to think of it, you can probably remove the "= 0" part of ret's
> declaration as well (in both functions).
>   

Right, even more: looks like both process_rxed_802_11_packet & 
libertas_process_rxed_packet can only return 0 so we could drop the 
return code altogether and change their signature to void too (nobody 
seems to care about their return code anyway). I will send a separate 
cleanup patch but this might be leaning more on the implementation 
detail side (planning to extend the functions and make the return code 
meaningful in the future?) so somebody familiar with the driver should 
make the call.

Thanks,
Florin


Signed-off-by: Florin Malita <fmalita@gmail.com>
---

 decl.h |    2 +-
 rx.c   |   22 +++++-----------------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/libertas/decl.h b/drivers/net/wireless/libertas/decl.h
index 606bdd0..dfe2764 100644
--- a/drivers/net/wireless/libertas/decl.h
+++ b/drivers/net/wireless/libertas/decl.h
@@ -46,7 +46,7 @@ u32 libertas_index_to_data_rate(u8 index);
 u8 libertas_data_rate_to_index(u32 rate);
 void libertas_get_fwversion(wlan_adapter * adapter, char *fwversion, int maxlen);
 
-int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb);
+void libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb);
 
 /** The proc fs interface */
 int libertas_process_rx_command(wlan_private * priv);
diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
index d17924f..b19b5aa 100644
--- a/drivers/net/wireless/libertas/rx.c
+++ b/drivers/net/wireless/libertas/rx.c
@@ -136,7 +136,7 @@ static void wlan_compute_rssi(wlan_private * priv, struct rxpd *p_rx_pd)
 	LEAVE();
 }
 
-int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
+void libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
 {
 	lbs_pr_debug(1, "skb->data=%p\n", skb->data);
 
@@ -148,8 +148,6 @@ int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	netif_rx(skb);
-
-	return 0;
 }
 
 /**
@@ -269,15 +267,11 @@ int libertas_process_rxed_packet(wlan_private * priv, struct sk_buff *skb)
 	wlan_compute_rssi(priv, p_rx_pd);
 
 	lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
-	if (libertas_upload_rx_packet(priv, skb)) {
-		lbs_pr_debug(1, "RX error: libertas_upload_rx_packet"
-		       " returns failure\n");
-		ret = -1;
-		goto done;
-	}
 	priv->stats.rx_bytes += skb->len;
 	priv->stats.rx_packets++;
 
+	libertas_upload_rx_packet(priv, skb);
+
 	ret = 0;
 done:
 	LEAVE();
@@ -438,17 +432,11 @@ static int process_rxed_802_11_packet(wlan_private * priv, struct sk_buff *skb)
 	wlan_compute_rssi(priv, prxpd);
 
 	lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
-
-	if (libertas_upload_rx_packet(priv, skb)) {
-		lbs_pr_debug(1, "RX error: libertas_upload_rx_packet "
-			"returns failure\n");
-		ret = -1;
-		goto done;
-	}
-
 	priv->stats.rx_bytes += skb->len;
 	priv->stats.rx_packets++;
 
+	libertas_upload_rx_packet(priv, skb);
+
 	ret = 0;
 done:
 	LEAVE();

Re: [PATCH] libertas: skb dereferenced after netif_rx

[Stephen Hemminger]

On Fri, 18 May 2007 14:09:03 -0400
"John W. Linville" <linville@tuxdriver.com> wrote:

> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> > In libertas_process_rxed_packet() and process_rxed_802_11_packet() the 
> > skb is dereferenced after being passed to netif_rx (called from 
> > libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
>  
> Relocating the libertas_upload_rx_packet call is fine, but...
> 
> > Also, libertas_upload_rx_packet() unconditionally returns 0 so the error 
> > check is dead code - might as well take it out.
> 
> Is this merely an implementation detail?  Or an absolute fact?
> If the former is true, then we should preserve the error
> checking.  If the latter, then we should change the signature of
> libertas_upload_rx_packet to return void.

netif_rx() used to return a value in older kernels.

Re: [PATCH] libertas: skb dereferenced after netif_rx

[Jeff Garzik]

Stephen Hemminger wrote:
> netif_rx() used to return a value in older kernels.

netif_rx() returns a value in current kernels.

	Jeff

Re: [PATCH] libertas: skb dereferenced after netif_rx

[Dan Williams]

On Fri, 2007-05-18 at 14:09 -0400, John W. Linville wrote:
> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> > In libertas_process_rxed_packet() and process_rxed_802_11_packet() the 
> > skb is dereferenced after being passed to netif_rx (called from 
> > libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
>  
> Relocating the libertas_upload_rx_packet call is fine, but...
> 
> > Also, libertas_upload_rx_packet() unconditionally returns 0 so the error 
> > check is dead code - might as well take it out.
> 
> Is this merely an implementation detail?  Or an absolute fact?
> If the former is true, then we should preserve the error
> checking.  If the latter, then we should change the signature of
> libertas_upload_rx_packet to return void.

According to the comments, netif_rx always succeeds.  I think we should
just change the return type to void since there's nothing else in that
function that can fail.

Dan


> > Signed-off-by: Florin Malita <fmalita@gmail.com>
> 
> > 	lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
> > -	if (libertas_upload_rx_packet(priv, skb)) {
> > -		lbs_pr_debug(1, "RX error: libertas_upload_rx_packet"
> > -		       " returns failure\n");
> > -		ret = -1;
> > -		goto done;
> > -	}
> > 	priv->stats.rx_bytes += skb->len;
> > 	priv->stats.rx_packets++;
> > 
> > +	libertas_upload_rx_packet(priv, skb);
> > +
> > 	ret = 0;
> > done:
> > 	LEAVE();
> 
> Another potential patch is to remove the "ret = 0" line before the
> "done" label, since ret is initialized at the head of the function.
> Come to think of it, you can probably remove the "= 0" part of ret's
> declaration as well (in both functions).
> 
> Hth!
> 
> John
> 
> P.S.  Also, please make sure to send wireless patches to
> linux-wireless@vger.kernel.org and CC me.

Re: [PATCH] libertas: skb dereferenced after netif_rx

[Jeff Garzik]

Dan Williams wrote:
> On Fri, 2007-05-18 at 14:09 -0400, John W. Linville wrote:
>> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
>>> In libertas_process_rxed_packet() and process_rxed_802_11_packet() the 
>>> skb is dereferenced after being passed to netif_rx (called from 
>>> libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
>>  
>> Relocating the libertas_upload_rx_packet call is fine, but...
>>
>>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error 
>>> check is dead code - might as well take it out.
>> Is this merely an implementation detail?  Or an absolute fact?
>> If the former is true, then we should preserve the error
>> checking.  If the latter, then we should change the signature of
>> libertas_upload_rx_packet to return void.
> 
> According to the comments, netif_rx always succeeds.  I think we should
> just change the return type to void since there's nothing else in that
> function that can fail.

According to the implementation, netif_rx() can fail.

	Jeff

Re: [PATCH] libertas: skb dereferenced after netif_rx

[Stephen Hemminger]

On Sat, 19 May 2007 21:47:00 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Dan Williams wrote:
> > On Fri, 2007-05-18 at 14:09 -0400, John W. Linville wrote:
> >> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> >>> In libertas_process_rxed_packet() and process_rxed_802_11_packet() the 
> >>> skb is dereferenced after being passed to netif_rx (called from 
> >>> libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
> >>  
> >> Relocating the libertas_upload_rx_packet call is fine, but...
> >>
> >>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error 
> >>> check is dead code - might as well take it out.
> >> Is this merely an implementation detail?  Or an absolute fact?
> >> If the former is true, then we should preserve the error
> >> checking.  If the latter, then we should change the signature of
> >> libertas_upload_rx_packet to return void.
> > 
> > According to the comments, netif_rx always succeeds.  I think we should
> > just change the return type to void since there's nothing else in that
> > function that can fail.
> 
> According to the implementation, netif_rx() can fail.
> 
> 	Jeff

Yeah, it was the old congestion levels that got dropped.
The skb is always consumed so the the return value is informational only.


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH] libertas: skb dereferenced after netif_rx

[Jeff Garzik]

Stephen Hemminger wrote:
> The skb is always consumed so the the return value is informational only.

Yes, this is true.  Handing off an skb to netif_rx() relieves the driver 
of further responsibility for it.

	Jeff

Re: [PATCH] libertas: skb dereferenced after netif_rx

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Sat, 19 May 2007 21:47:00 -0400

> Dan Williams wrote:
> > On Fri, 2007-05-18 at 14:09 -0400, John W. Linville wrote:
> >> On Wed, May 16, 2007 at 05:01:27PM -0400, Florin Malita wrote:
> >>> In libertas_process_rxed_packet() and process_rxed_802_11_packet() the 
> >>> skb is dereferenced after being passed to netif_rx (called from 
> >>> libertas_upload_rx_packet). Spotted by Coverity (1658, 1659).
> >>  
> >> Relocating the libertas_upload_rx_packet call is fine, but...
> >>
> >>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error 
> >>> check is dead code - might as well take it out.
> >> Is this merely an implementation detail?  Or an absolute fact?
> >> If the former is true, then we should preserve the error
> >> checking.  If the latter, then we should change the signature of
> >> libertas_upload_rx_packet to return void.
> > 
> > According to the comments, netif_rx always succeeds.  I think we should
> > just change the return type to void since there's nothing else in that
> > function that can fail.
> 
> According to the implementation, netif_rx() can fail.

It doesn't exactly "fail", but it does give return values
which indicate RX congestion.

Re: [PATCH] libertas: skb dereferenced after netif_rx

[Florin Malita]

David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Sat, 19 May 2007 21:47:00 -0400
>   
>> According to the implementation, netif_rx() can fail.
>>     
>
> It doesn't exactly "fail", but it does give return values
> which indicate RX congestion.
>   

Assuming you're referring to NET_RX_CN_*, this doesn't seem to be the 
case anymore: it appears netif_rx can only return NET_RX_SUCCESS or 
NET_RX_DROP.

The congestion level constants are hardly used at all - if they are 
left-overs, would it make sense ripping them out completely?

Either way I believe the following should be OK:


Remove inaccurate netif_rx() return value comments.

Signed-off-by: Florin Malita <fmalita@gmail.com>
---

 dev.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f2b6111..79f5d90 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1628,9 +1628,6 @@ DEFINE_PER_CPU(struct netif_rx_stats, netdev_rx_stat) = { 0, };
  *
  *	return values:
  *	NET_RX_SUCCESS	(no congestion)
- *	NET_RX_CN_LOW   (low congestion)
- *	NET_RX_CN_MOD   (moderate congestion)
- *	NET_RX_CN_HIGH  (high congestion)
  *	NET_RX_DROP     (packet was dropped)
  *
  */