Skip to contentSkip to conversation

[2.6.26] [CAN] Fix can_send() handling on dev_queue_xmit() failures

PATCH Applied Oliver Hartkopp 4 participants 6 messages May 6, 2008 – May 13, 2008

Subsystems (3)
Networking Network drivers CAN network layer
lore
Share & tools
lore

Oliver Hartkopp

author
#0 lore raw

PATCH 1/1 [2.6.26] [CAN] Fix can_send() handling on dev_queue_xmit() failures

from Oliver Hartkopp <[email protected]>

The tx packet counting and the local loopback of CAN frames should only happen in the case that the CAN frame has been enqueued to the netdevice tx queue successfully.

Thanks to Andre Naujoks <[email protected]> for reporting this issue.

Signed-off-by Oliver Hartkopp <[email protected]>
Signed-off-by Urs Thuermann <[email protected]>

---

Hello Dave,

this patch should go into 2.6.26 and also applies fine on 2.6.25.1.

IMO this issue is not that 'critical' that it *requires* to become part of 2.6.25.2. Please forward the patch to Greg at your own opinion.

Many Thanks!
Oliver

net/can/af_can.c +12 -4
index 2759b76..7e8ca28 100644
@@ -208,6 +208,7 @@ static int can_create(struct net *net, struct socket *sock, int protocol)
  */
 int can_send(struct sk_buff *skb, int loop)
 {
+	struct sk_buff *newskb = NULL;
 	int err;
 
 	if (skb->dev->type != ARPHRD_CAN) {

@@ -244,8 +245,7 @@ int can_send(struct sk_buff *skb, int loop)
 			 * If the interface is not capable to do loopback
 			 * itself, we do it here.
 			 */
-			struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
-
+			newskb = skb_clone(skb, GFP_ATOMIC);
 			if (!newskb) {
 				kfree_skb(skb);
 				return -ENOMEM;

@@ -254,7 +254,6 @@ int can_send(struct sk_buff *skb, int loop)
 			newskb->sk = skb->sk;
 			newskb->ip_summed = CHECKSUM_UNNECESSARY;
 			newskb->pkt_type = PACKET_BROADCAST;
-			netif_rx(newskb);
 		}
 	} else {
 		/* indication for the CAN driver: no loopback required */

@@ -266,11 +265,20 @@ int can_send(struct sk_buff *skb, int loop)
 	if (err > 0)
 		err = net_xmit_errno(err);
 
+	if (err) {
+		if (newskb)
+			kfree_skb(newskb);
+		return err;
+	}
+
+	if (newskb)
+		netif_rx(newskb);
+
 	/* update statistics */
 	can_stats.tx_frames++;
 	can_stats.tx_frames_delta++;
 
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(can_send);
maintainer Applied
#1 lore raw

From: Oliver Hartkopp <[email protected]>
Date: Tue, 06 May 2008 21:49:57 +0200

Quoted 8 lines

The tx packet counting and the local loopback of CAN frames should only happen in the case that the CAN frame has been enqueued to the netdevice tx queue successfully.

Thanks to Andre Naujoks <[email protected]> for reporting this issue.

Signed-off-by Oliver Hartkopp <[email protected]>
Signed-off-by Urs Thuermann <[email protected]>

Applied, thanks Oliver.

Quoted 4 lines

this patch should go into 2.6.26 and also applies fine on 2.6.25.1.

IMO this issue is not that 'critical' that it *requires* to become part of 2.6.25.2. Please forward the patch to Greg at your own opinion.

I think I'll push it to -stable, thanks.

author
#2 lore raw
Quoted 10 lines · David Miller
Quoted 6 lines

this patch should go into 2.6.26 and also applies fine on 2.6.25.1.

IMO this issue is not that 'critical' that it *requires* to become part of 2.6.25.2. Please forward the patch to Greg at your own opinion.

I think I'll push it to -stable, thanks.

Thanks!

Maybe we should also consider to push the patch from Sam regarding the wrong copy_from_user() results interpretation to the stable tree:

can: Fix copy_from_user() results interpretation

author Sam Ravnborg <[email protected]>

	Sun, 27 Apr 2008 05:57:25 +0000 (22:57 -0700)

committer David S. Miller <[email protected]>

	Sun, 27 Apr 2008 21:26:51 +0000 (14:26 -0700)

commit 3f91bd420a955803421f2db17b2e04aacfbb2bb8

Regards,
Oliver

#3 lore raw
Quoted 19 lines · Oliver Hartkopp
Quoted 10 lines · David Miller
Quoted 6 lines

this patch should go into 2.6.26 and also applies fine on 2.6.25.1.

IMO this issue is not that 'critical' that it *requires* to become part of 2.6.25.2. Please forward the patch to Greg at your own opinion.

I think I'll push it to -stable, thanks.

Thanks!

Maybe we should also consider to push the patch from Sam regarding the wrong copy_from_user() results interpretation to the stable tree:

can: Fix copy_from_user() results interpretation

Quoted 7 lines

author Sam Ravnborg <[email protected]>

	Sun, 27 Apr 2008 05:57:25 +0000 (22:57 -0700)

committer David S. Miller <[email protected]>

	Sun, 27 Apr 2008 21:26:51 +0000 (14:26 -0700)

commit 3f91bd420a955803421f2db17b2e04aacfbb2bb8

I recall tat I considered stable material when I did this patch. But as I merely just did a codingstyle update of the old patch I dropped it.

But looks like possible -stable materail so I've added [email protected] to the to list so they can judge.

Sam

author
#4 lore raw
Quoted 17 lines · Sam Ravnborg
Quoted 6 lines · Oliver Hartkopp

Maybe we should also consider to push the patch from Sam regarding the wrong copy_from_user() results interpretation to the stable tree:

can: Fix copy_from_user() results interpretation

Quoted 8 lines

author Sam Ravnborg <[email protected]>

	Sun, 27 Apr 2008 05:57:25 +0000 (22:57 -0700)

committer David S. Miller <[email protected]>

	Sun, 27 Apr 2008 21:26:51 +0000 (14:26 -0700)

commit 3f91bd420a955803421f2db17b2e04aacfbb2bb8

Quoted 7 lines

I recall tat I considered stable material when I did this patch. But as I merely just did a codingstyle update of the old patch I dropped it.

But looks like possible -stable materail so I've added [email protected] to the to list so they can judge.

Yep. Thanks.

Indeed you updated the codingstyle from a patch suggested by Pavel - but his (original) patch fixes the copy_from_user() result interpretation. So it was not only a codingstyle patch which was commited here.

Best regards,
Oliver

#5 lore raw
In reply to Sam Ravnborg
Quoted 28 lines · Sam Ravnborg
Quoted 19 lines · Oliver Hartkopp
Quoted 10 lines · David Miller
Quoted 6 lines

this patch should go into 2.6.26 and also applies fine on 2.6.25.1.

IMO this issue is not that 'critical' that it *requires* to become part of 2.6.25.2. Please forward the patch to Greg at your own opinion.

I think I'll push it to -stable, thanks.

Thanks!

Maybe we should also consider to push the patch from Sam regarding the wrong copy_from_user() results interpretation to the stable tree:

can: Fix copy_from_user() results interpretation

Quoted 7 lines

author Sam Ravnborg <[email protected]>

	Sun, 27 Apr 2008 05:57:25 +0000 (22:57 -0700)

committer David S. Miller <[email protected]>

	Sun, 27 Apr 2008 21:26:51 +0000 (14:26 -0700)

commit 3f91bd420a955803421f2db17b2e04aacfbb2bb8

Quoted 6 lines

I recall tat I considered stable material when I did this patch. But as I merely just did a codingstyle update of the old patch I dropped it.

But looks like possible -stable materail so I've added [email protected] to the to list so they can judge.

I trust David to send the -stable team any networking patches he things needs to go there. So it's really up to him.

thanks,

greg k-h