[PATCH net-next-2.6 v3] can: Unify droping of invalid tx skbs and netdev stats

[PATCH net-next-2.6 v3] can: Unify droping of invalid tx skbs and netdev stats

[Oliver Hartkopp]

As suggested by Marc and Wolfgang here is the v3 version:

To prevent the CAN drivers to operate on invalid socketbuffers the skbs are
now checked and silently dropped at the xmit-function consistently.

Also the netdev stats are consistently using the CAN data length code (dlc)
for [rx|tx]_bytes now.

Signed-off-by: Oliver Hartkopp <oliver-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>

---

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 166cc7e..f728749 100644
--- a/drivers/net/can/at91_can.c
+++ b/drivers/net/can/at91_can.c
@@ -342,6 +342,9 @@ static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	unsigned int mb, prio;
 	u32 reg_mid, reg_mcr;
 
+	if (can_dropped_invalid_skb(dev, skb))
+		return NETDEV_TX_OK;
+
 	mb = get_tx_next_mb(priv);
 	prio = get_tx_next_prio(priv);
 
diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c
index 0ec1524..7e1926e 100644
--- a/drivers/net/can/bfin_can.c
+++ b/drivers/net/can/bfin_can.c
@@ -318,6 +318,9 @@ static int bfin_can_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u16 val;
 	int i;
 
+	if (can_dropped_invalid_skb(dev, skb))
+		return NETDEV_TX_OK;
+
 	netif_stop_queue(dev);
 
 	/* fill id */
diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index 9c5a153..d46d084 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -494,12 +494,8 @@ static netdev_tx_t mcp251x_hard_start_xmit(struct sk_buff *skb,
 		return NETDEV_TX_BUSY;
 	}
 
-	if (skb->len != sizeof(struct can_frame)) {
-		dev_err(&spi->dev, "dropping packet - bad length\n");
-		dev_kfree_skb(skb);
-		net->stats.tx_dropped++;
+	if (can_dropped_invalid_skb(net, skb))
 		return NETDEV_TX_OK;
-	}
 
 	netif_stop_queue(net);
 	priv->tx_skb = skb;
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 542a4f7..345304d 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -249,6 +249,9 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 	uint8_t dreg;
 	int i;
 
+	if (can_dropped_invalid_skb(dev, skb))
+		return NETDEV_TX_OK;
+
 	netif_stop_queue(dev);
 
 	fi = dlc = cf->can_dlc;
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 5c993c2..7d370e3 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -477,6 +477,9 @@ static netdev_tx_t ti_hecc_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u32 mbxno, mbx_mask, data;
 	unsigned long flags;
 
+	if (can_dropped_invalid_skb(ndev, skb))
+		return NETDEV_TX_OK;
+
 	mbxno = get_tx_head_mb(priv);
 	mbx_mask = BIT(mbxno);
 	spin_lock_irqsave(&priv->mbx_lock, flags);
@@ -491,7 +494,6 @@ static netdev_tx_t ti_hecc_xmit(struct sk_buff *skb, struct net_device *ndev)
 	spin_unlock_irqrestore(&priv->mbx_lock, flags);
 
 	/* Prepare mailbox for transmission */
-	data = min_t(u8, cf->can_dlc, 8);
 	if (cf->can_id & CAN_RTR_FLAG) /* Remote transmission request */
 		data |= HECC_CANMCF_RTR;
 	data |= get_tx_head_prio(priv) << 8;
diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index efbb05c..ddb17e2 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -767,6 +767,9 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 	size_t size = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN
 			+ sizeof(struct cpc_can_msg);
 
+	if (can_dropped_invalid_skb(netdev, skb))
+		return NETDEV_TX_OK;
+
 	/* create a URB, and a buffer for it, and copy the data to the URB */
 	urb = usb_alloc_urb(0, GFP_ATOMIC);
 	if (!urb) {
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 80ac563..66411d5 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -70,10 +70,11 @@ MODULE_PARM_DESC(echo, "Echo sent frames (for testing). Default: 0 (Off)");
 
 static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 {
+	struct can_frame *cf = (struct can_frame *)skb->data;
 	struct net_device_stats *stats = &dev->stats;
 
 	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
+	stats->rx_bytes += cf->can_dlc;
 
 	skb->protocol  = htons(ETH_P_CAN);
 	skb->pkt_type  = PACKET_BROADCAST;
@@ -85,11 +86,18 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 
 static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 {
+	struct can_frame *cf = (struct can_frame *)skb->data;
 	struct net_device_stats *stats = &dev->stats;
 	int loop;
 
+	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
+		kfree_skb(skb);
+		stats->tx_dropped++;
+		return NETDEV_TX_OK;
+	}
+
 	stats->tx_packets++;
-	stats->tx_bytes += skb->len;
+	stats->tx_bytes += cf->can_dlc;
 
 	/* set flag whether this packet has to be looped back */
 	loop = skb->pkt_type == PACKET_LOOPBACK;
@@ -103,7 +111,7 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 			 * CAN core already did the echo for us
 			 */
 			stats->rx_packets++;
-			stats->rx_bytes += skb->len;
+			stats->rx_bytes += cf->can_dlc;
 		}
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 3db7767..7e7c98a 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -60,6 +60,21 @@ struct can_priv {
  */
 #define get_can_dlc(i)	(min_t(__u8, (i), 8))
 
+/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
+static inline int can_dropped_invalid_skb(struct net_device *dev,
+					  struct sk_buff *skb)
+{
+	const struct can_frame *cf = (struct can_frame *)skb->data;
+
+	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
+		kfree_skb(skb);
+		dev->stats.tx_dropped++;
+		return 1;
+	}
+
+	return 0;
+}
+
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
 void free_candev(struct net_device *dev);

Re: [PATCH net-next-2.6 v3] can: Unify droping of invalid tx skbs and netdev stats

[David Miller]

From: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
Date: Tue, 12 Jan 2010 00:19:21 +0100

>  static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
>  {
> +	struct can_frame *cf = (struct can_frame *)skb->data;
>  	struct net_device_stats *stats = &dev->stats;
>  	int loop;
>  
> +	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
> +		kfree_skb(skb);
> +		stats->tx_dropped++;
> +		return NETDEV_TX_OK;
> +	}
> +
 ...
> +/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
> +static inline int can_dropped_invalid_skb(struct net_device *dev,
> +					  struct sk_buff *skb)
> +{
> +	const struct can_frame *cf = (struct can_frame *)skb->data;
> +
> +	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
> +		kfree_skb(skb);
> +		dev->stats.tx_dropped++;
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);

Why are you not using the new helper function in vcan_tx()?

Re: [PATCH net-next-2.6 v3] can: Unify droping of invalid tx skbs and netdev stats

[Oliver Hartkopp]

David Miller wrote:
> From: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
> Date: Tue, 12 Jan 2010 00:19:21 +0100
> 
>>  static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
>>  {
>> +	struct can_frame *cf = (struct can_frame *)skb->data;
>>  	struct net_device_stats *stats = &dev->stats;
>>  	int loop;
>>  
>> +	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
>> +		kfree_skb(skb);
>> +		stats->tx_dropped++;
>> +		return NETDEV_TX_OK;
>> +	}
>> +
>  ...
>> +/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
>> +static inline int can_dropped_invalid_skb(struct net_device *dev,
>> +					  struct sk_buff *skb)
>> +{
>> +	const struct can_frame *cf = (struct can_frame *)skb->data;
>> +
>> +	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
>> +		kfree_skb(skb);
>> +		dev->stats.tx_dropped++;
>> +		return 1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
> 
> Why are you not using the new helper function in vcan_tx()?

I just wanted the vcan driver keep off including "include/linux/can/dev.h"
which is intended for 'real' CAN hardware.

As the vcan software devices do not need the bitrate-setting and skb echo
handling from the driver library for real CAN devices, this inline function
would be the only reason to include ".../dev.h"

But i don't have a strong preference to do it like this. Do you think i
should change it to used the defined inline function?

Regards,
Oliver

Re: [PATCH net-next-2.6 v3] can: Unify droping of invalid tx skbs and netdev stats

[David Miller]

From: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
Date: Tue, 12 Jan 2010 09:32:11 +0100

> But i don't have a strong preference to do it like this. Do you think i
> should change it to used the defined inline function?

Yes, less copies of the same code less bugs.

Re: [PATCH net-next-2.6 v3] can: Unify droping of invalid tx skbs and netdev stats

[Wolfgang Grandegger]

Oliver Hartkopp wrote:
> David Miller wrote:
>> From: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
>> Date: Tue, 12 Jan 2010 00:19:21 +0100
>>
>>>  static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
>>>  {
>>> +	struct can_frame *cf = (struct can_frame *)skb->data;
>>>  	struct net_device_stats *stats = &dev->stats;
>>>  	int loop;
>>>  
>>> +	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
>>> +		kfree_skb(skb);
>>> +		stats->tx_dropped++;
>>> +		return NETDEV_TX_OK;
>>> +	}
>>> +
>>  ...
>>> +/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
>>> +static inline int can_dropped_invalid_skb(struct net_device *dev,
>>> +					  struct sk_buff *skb)
>>> +{
>>> +	const struct can_frame *cf = (struct can_frame *)skb->data;
>>> +
>>> +	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
>>> +		kfree_skb(skb);
>>> +		dev->stats.tx_dropped++;
>>> +		return 1;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
>> Why are you not using the new helper function in vcan_tx()?
> 
> I just wanted the vcan driver keep off including "include/linux/can/dev.h"
> which is intended for 'real' CAN hardware.
> 
> As the vcan software devices do not need the bitrate-setting and skb echo
> handling from the driver library for real CAN devices, this inline function
> would be the only reason to include ".../dev.h"
> 
> But i don't have a strong preference to do it like this. Do you think i
> should change it to used the defined inline function?

Yes, for the sake of consistency.

Wolfgang.