libertas: skb dereferenced after netif_rx

PATCH In review Florin Malita 8 participants 11 messages May 16, 2007 – May 21, 2007 4 messages not in the archive

Subsystems (3)

Networking Network drivers Networking drivers (wireless)

lore

Share & tools

lore

Florin Malita

author

2007-05-16 21:01 UTC #0 lore raw

PATCH 1/1 libertas: skb dereferenced after netif_rx

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).

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

Signed-off-by Florin Malita <[email protected]>

---

rx.c | 18 ++++--------------

1 file changed, 4 insertions(+), 14 deletions(-)

drivers/net/wireless/libertas/rx.c +0 -0

index d17924f..5e98055 100644
@@ -269,15 +269,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 +434,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();

A message that is not in the archive (sent off-list or not yet received): [email protected]

John W. Linville

2007-05-18 18:09 UTC #2 lore raw

Quoted 3 lines · Florin Malita

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...

Quoted 2 lines

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.

Quoted 14 lines

Signed-off-by Florin Malita <[email protected]>

	lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);

(quoted diff) +0 -6

-	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++;

(quoted diff) +2 -0

+	libertas_upload_rx_packet(priv, skb);
+

Quoted 3 lines

	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 [email protected] and CC me.

Signature · 2 lines

John W. Linville
[email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Florin Malita

2007-05-18 20:04 UTC #4 lore raw

Quoted 5 lines · John W. Linville

Quoted 2 lines
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).

Quoted 3 lines

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

Makes sense, updated patch below.

Quoted 5 lines

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 <[email protected]>

---

decl.h | 2 +-
rx.c | 22 +++++-----------------

2 files changed, 6 insertions(+), 18 deletions(-)

drivers/net/wireless/libertas/decl.h +0 -0

index 606bdd0..dfe2764 100644
@@ -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);

drivers/net/wireless/libertas/rx.c +0 -0

index d17924f..b19b5aa 100644
@@ -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();

Stephen Hemminger

2007-05-19 05:23 UTC #5 lore raw

In reply to a message not in the archive

Quoted 7 lines · On Fri, 18 May 2007 14:09:03 -0400 "John W. Linville"…

Quoted 3 lines · Florin Malita
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...

Quoted 7 lines

Quoted 2 lines
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.

Jeff Garzik

2007-05-19 05:37 UTC #6 lore raw

Quoted 1 line · Stephen Hemminger

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

netif_rx() returns a value in current kernels.

Jeff

Dan Williams

2007-05-20 00:54 UTC #7 lore raw

In reply to John W. Linville

Quoted 7 lines · John W. Linville

Quoted 3 lines · Florin Malita
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...

Quoted 7 lines

Quoted 2 lines
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

Quoted 29 lines

Quoted 14 lines
Signed-off-by Florin Malita <[email protected]>
	lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
(quoted diff) +0 -6
-	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++;
(quoted diff) +2 -0
+	libertas_upload_rx_packet(priv, skb);
+
Quoted 3 lines
	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 [email protected] and CC me.

Jeff Garzik

2007-05-20 01:47 UTC #8 lore raw

Quoted 7 lines · Dan Williams

Quoted 6 lines · John W. Linville
Quoted 1 line · Florin Malita
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...
Quoted 1 line
Also, libertas_upload_rx_packet() unconditionally returns 0 so the error check is dead code - might as well take it out.

Quoted 8 lines

Quoted 4 lines
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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stephen Hemminger

2007-05-20 05:20 UTC #10 lore raw

Quoted 16 lines · On Sat, 19 May 2007 21:47:00 -0400 Jeff Garzik…

Quoted 15 lines · Dan Williams
Quoted 13 lines · John W. Linville
Quoted 3 lines · Florin Malita
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...
Quoted 2 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Signature · 1 line

Stephen Hemminger <shemminger-de/[email protected]>

Jeff Garzik

2007-05-20 06:37 UTC #11 lore raw

Quoted 1 line · Stephen Hemminger

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

David Miller

2007-05-20 07:38 UTC #12 lore raw

In reply to a message not in the archive

From: Jeff Garzik <[email protected]>
Date: Sat, 19 May 2007 21:47:00 -0400

Quoted 16 lines

Quoted 15 lines · Dan Williams
Quoted 13 lines · John W. Linville
Quoted 3 lines · Florin Malita
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...
Quoted 2 lines
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.

Quoted 5 lines

Quoted 3 lines
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.

A message that is not in the archive (sent off-list or not yet received): 20070520.003832.59470116.davem-fT/PcQaiUtIeIZ0/[email protected]

Florin Malita

2007-05-21 14:51 UTC #14 lore raw

Quoted 9 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Sat, 19 May 2007 21:47:00 -0400
Quoted 2 lines
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 <[email protected]>

---

dev.c | 3 ---

1 file changed, 3 deletions(-)

net/core/dev.c +0 -3

index f2b6111..79f5d90 100644
@@ -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)
 *
 */