| Message ID | 4B4BB1F9.3010808@hartkopp.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Oliver Hartkopp <socketcan@hartkopp.net> 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()? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Oliver Hartkopp <socketcan@hartkopp.net> > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Oliver Hartkopp <socketcan@hartkopp.net> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp wrote: > David Miller wrote: >> From: Oliver Hartkopp <socketcan@hartkopp.net> >> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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@hartkopp.net> --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html