via-velocity skb_over_panic

via-velocity skb_over_panic

[Timo Teras]

Hi,

I recently saw via-velocity skb_over_panic() on one of my locations.
The panic happened with two separate hardware devices, so it appears to
be network related, not broken hardware.

I did not get the actual over_panic printk, as I got only screen shot
of them monitor. But the visible part of call trace says:
 <IRQ>
 skb_put
 velocity_poll
 net_rx_action
 __do_softirq
 irq_exit
 common_interrupt
 <EOI>

The was recurring every few hours, so I patched via-velocity with the
following after looking the code a bit:

--- a/drivers/net/ethernet/via/via-velocity.c
+++ b/drivers/net/ethernet/via/via-velocity.c
@@ -2060,6 +2060,11 @@ static int velocity_receive_frame(struct velocity_info *vptr, int idx)
 		stats->rx_length_errors++;
 		return -EINVAL;
 	}
+	if (pkt_len < 4 || pkt_len > vptr->rx.buf_sz) {
+		VELOCITY_PRT(MSG_LEVEL_VERBOSE, KERN_ERR " %s : the received frame size %d is inconsistent.\n", vptr->netdev->name, pkt_len);
+		stats->rx_length_errors++;
+		return -EINVAL;
+	}
 
 	if (rd->rdesc0.RSR & RSR_MAR)
 		stats->multicast++;

This seems to have fixed the panics. And I do see one of the NIC's
ethtool report's in_range_length_errors increasing once in a while. For
some reason I don't see the above debug message though, so I'm not sure
on what pkt_len triggers it.

In any case, the cade a bit later on does unconditionally:
	skb_put(skb, pkt_len - 4);

So it's possible that some bad packets make the NIC return unexpected
packet sizes, and the current code can panic on it.

Any suggestions for better fix?

Thanks,
Timo

Re: via-velocity skb_over_panic

[David Miller]

From: Timo Teras <timo.teras@iki.fi>
Date: Fri, 13 Nov 2015 18:49:47 +0200

> +	if (pkt_len < 4 || pkt_len > vptr->rx.buf_sz) {
> +		VELOCITY_PRT(MSG_LEVEL_VERBOSE, KERN_ERR " %s : the received frame size %d is inconsistent.\n", vptr->netdev->name, pkt_len);
 ...
> This seems to have fixed the panics. And I do see one of the NIC's
> ethtool report's in_range_length_errors increasing once in a while. For
> some reason I don't see the above debug message though, so I'm not sure
> on what pkt_len triggers it.

You have to set the driver message level >= MSG_LEVEL_VERBOSE (3) to
see it.

Re: via-velocity skb_over_panic

[Francois Romieu]

Timo Teras <timo.teras@iki.fi> :
[...]
> So it's possible that some bad packets make the NIC return unexpected
> packet sizes, and the current code can panic on it.
> 
> Any suggestions for better fix?

        if (vptr->flags & VELOCITY_FLAGS_VAL_PKT_LEN) {
                if (rd->rdesc0.RSR & RSR_RL) {

Huh ?

[...]
        velocity_set_bool_opt(&opts->flags, ValPktLen[index], VAL_PKT_LEN_DEF, VELOCITY_FLAGS_VAL_PKT_LEN, "ValPktLen", devname);
[...]
#define VAL_PKT_LEN_DEF     0
/* ValPktLen[] is used for setting the checksum offload ability of NIC.
   0: Receive frame with invalid layer 2 length (Default)
   1: Drop frame with invalid layer 2 length
*/
VELOCITY_PARAM(ValPktLen, "Receiving or Drop invalid 802.3 frame");

*spleen*

RSR_RL should be set on packet length error. You can imnsvho remove the
VELOCITY_FLAGS_VAL_PKT_LEN and VAL_PKT_LEN_DEF stuff altogether.
He who cares about this option should add NETIF_F_RXALL support to the
via-velocity driver.

-- 
Ueimor

[PATCH] via-velocity: unconditionally drop frames with bad l2 length

[Timo Teräs]

By default the driver allowed incorrect frames to be received. What is
worse the code does not handle very short frames correctly. The FCS
length is unconditionally subtracted, and the underflow can cause
skb_put to be called with large number after implicit cast to unsigned.
And indeed, an skb_over_panic() was observed with via-velocity.

This removes the module parameter as it does not work in it's
current state, and should be implemented via NETIF_F_RXALL if needed.

Suggested-by: Francois Romieu <romieu@fr.zoreil.com>
Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
Francois, is this something like you had in mind? I can try give this
a test spin in the known bad location, if this looks otherwise ok.

 drivers/net/ethernet/via/via-velocity.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/via/via-velocity.c b/drivers/net/ethernet/via/via-velocity.c
index a43e849..03ce386 100644
--- a/drivers/net/ethernet/via/via-velocity.c
+++ b/drivers/net/ethernet/via/via-velocity.c
@@ -345,13 +345,6 @@ VELOCITY_PARAM(flow_control, "Enable flow control ability");
 */
 VELOCITY_PARAM(speed_duplex, "Setting the speed and duplex mode");
 
-#define VAL_PKT_LEN_DEF     0
-/* ValPktLen[] is used for setting the checksum offload ability of NIC.
-   0: Receive frame with invalid layer 2 length (Default)
-   1: Drop frame with invalid layer 2 length
-*/
-VELOCITY_PARAM(ValPktLen, "Receiving or Drop invalid 802.3 frame");
-
 #define WOL_OPT_DEF     0
 #define WOL_OPT_MIN     0
 #define WOL_OPT_MAX     7
@@ -494,7 +487,6 @@ static void velocity_get_options(struct velocity_opt *opts, int index,
 
 	velocity_set_int_opt(&opts->flow_cntl, flow_control[index], FLOW_CNTL_MIN, FLOW_CNTL_MAX, FLOW_CNTL_DEF, "flow_control", devname);
 	velocity_set_bool_opt(&opts->flags, IP_byte_align[index], IP_ALIG_DEF, VELOCITY_FLAGS_IP_ALIGN, "IP_byte_align", devname);
-	velocity_set_bool_opt(&opts->flags, ValPktLen[index], VAL_PKT_LEN_DEF, VELOCITY_FLAGS_VAL_PKT_LEN, "ValPktLen", devname);
 	velocity_set_int_opt((int *) &opts->spd_dpx, speed_duplex[index], MED_LNK_MIN, MED_LNK_MAX, MED_LNK_DEF, "Media link mode", devname);
 	velocity_set_int_opt(&opts->wol_opts, wol_opts[index], WOL_OPT_MIN, WOL_OPT_MAX, WOL_OPT_DEF, "Wake On Lan options", devname);
 	opts->numrx = (opts->numrx & ~3);
@@ -2055,8 +2047,9 @@ static int velocity_receive_frame(struct velocity_info *vptr, int idx)
 	int pkt_len = le16_to_cpu(rd->rdesc0.len) & 0x3fff;
 	struct sk_buff *skb;
 
-	if (rd->rdesc0.RSR & (RSR_STP | RSR_EDP)) {
-		VELOCITY_PRT(MSG_LEVEL_VERBOSE, KERN_ERR " %s : the received frame spans multiple RDs.\n", vptr->netdev->name);
+	if (unlikely(rd->rdesc0.RSR & (RSR_STP | RSR_EDP | RSR_RL))) {
+		if (rd->rdesc0.RSR & (RSR_STP | RSR_EDP))
+			VELOCITY_PRT(MSG_LEVEL_VERBOSE, KERN_ERR " %s : the received frame spans multiple RDs.\n", vptr->netdev->name);
 		stats->rx_length_errors++;
 		return -EINVAL;
 	}
@@ -2069,17 +2062,6 @@ static int velocity_receive_frame(struct velocity_info *vptr, int idx)
 	dma_sync_single_for_cpu(vptr->dev, rd_info->skb_dma,
 				    vptr->rx.buf_sz, DMA_FROM_DEVICE);
 
-	/*
-	 *	Drop frame not meeting IEEE 802.3
-	 */
-
-	if (vptr->flags & VELOCITY_FLAGS_VAL_PKT_LEN) {
-		if (rd->rdesc0.RSR & RSR_RL) {
-			stats->rx_length_errors++;
-			return -EINVAL;
-		}
-	}
-
 	velocity_rx_csum(rd, skb);
 
 	if (velocity_rx_copy(&skb, pkt_len, vptr) < 0) {
-- 
2.6.3

Re: [PATCH] via-velocity: unconditionally drop frames with bad l2 length

[David Miller]

From: Timo Teräs <timo.teras@iki.fi>
Date: Mon, 16 Nov 2015 14:36:32 +0200

> By default the driver allowed incorrect frames to be received. What is
> worse the code does not handle very short frames correctly. The FCS
> length is unconditionally subtracted, and the underflow can cause
> skb_put to be called with large number after implicit cast to unsigned.
> And indeed, an skb_over_panic() was observed with via-velocity.
> 
> This removes the module parameter as it does not work in it's
> current state, and should be implemented via NETIF_F_RXALL if needed.
> 
> Suggested-by: Francois Romieu <romieu@fr.zoreil.com>
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>

Applied, thanks Timo.