diff mbox series

[net-next,15/18] can: kvaser_usb: Update stats and state even if alloc_can_err_skb() fails

Message ID 20250110112712.3214173-16-mkl@pengutronix.de (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series [net-next,01/18] dt-bindings: can: mpfs: add PIC64GX CAN compatibility | expand

Commit Message

Marc Kleine-Budde Jan. 10, 2025, 11:04 a.m. UTC 
From: Jimmy Assarsson <extja@kvaser.com>

Ensure statistics, error counters, and CAN state are updated consistently,
even when alloc_can_err_skb() fails during state changes or error message
frame reception.

Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
Link: https://patch.msgid.link/20241230142645.128244-1-extja@kvaser.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 124 +++++++-----------
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  |  31 ++---
 2 files changed, 60 insertions(+), 95 deletions(-)

Comments

Simon Horman Jan. 10, 2025, 12:58 p.m. UTC | #1 
On Fri, Jan 10, 2025 at 12:04:23PM +0100, Marc Kleine-Budde wrote:
> From: Jimmy Assarsson <extja@kvaser.com>
> 
> Ensure statistics, error counters, and CAN state are updated consistently,
> even when alloc_can_err_skb() fails during state changes or error message
> frame reception.
> 
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> Link: https://patch.msgid.link/20241230142645.128244-1-extja@kvaser.com
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

...

> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c

...

> @@ -1187,11 +1169,18 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
>  		if (priv->can.restart_ms &&
>  		    old_state == CAN_STATE_BUS_OFF &&
>  		    new_state < CAN_STATE_BUS_OFF) {
> -			cf->can_id |= CAN_ERR_RESTARTED;
> +			if (cf)
> +				cf->can_id |= CAN_ERR_RESTARTED;
>  			netif_carrier_on(priv->netdev);
>  		}
>  	}
>  
> +	if (!skb) {
> +		stats->rx_dropped++;
> +		netdev_warn(priv->netdev, "No memory left for err_skb\n");
> +		return;
> +	}
> +
>  	switch (dev->driver_info->family) {
>  	case KVASER_LEAF:
>  		if (es->leaf.error_factor) {

Hi Jimmy and Marc,

The next line of this function is:

			cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;

Which dereferences cf. However, the check added at the top of
this hunk assumes that cf may be NULL. This doesn't seem consistent.

Flagged by Smatch.

> -- 
> 2.45.2
> 
> 
>
Marc Kleine-Budde Jan. 10, 2025, 1:10 p.m. UTC | #2 
On 10.01.2025 12:58:03, Simon Horman wrote:
> On Fri, Jan 10, 2025 at 12:04:23PM +0100, Marc Kleine-Budde wrote:
> > From: Jimmy Assarsson <extja@kvaser.com>
> > 
> > Ensure statistics, error counters, and CAN state are updated consistently,
> > even when alloc_can_err_skb() fails during state changes or error message
> > frame reception.
> > 
> > Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> > Link: https://patch.msgid.link/20241230142645.128244-1-extja@kvaser.com
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> ...
> 
> > diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> 
> ...
> 
> > @@ -1187,11 +1169,18 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
> >  		if (priv->can.restart_ms &&
> >  		    old_state == CAN_STATE_BUS_OFF &&
> >  		    new_state < CAN_STATE_BUS_OFF) {
> > -			cf->can_id |= CAN_ERR_RESTARTED;
> > +			if (cf)
> > +				cf->can_id |= CAN_ERR_RESTARTED;
> >  			netif_carrier_on(priv->netdev);
> >  		}
> >  	}
> >  
> > +	if (!skb) {
> > +		stats->rx_dropped++;
> > +		netdev_warn(priv->netdev, "No memory left for err_skb\n");
> > +		return;
> > +	}
> > +
> >  	switch (dev->driver_info->family) {
> >  	case KVASER_LEAF:
> >  		if (es->leaf.error_factor) {
> 
> Hi Jimmy and Marc,
> 
> The next line of this function is:
> 
> 			cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> 
> Which dereferences cf. However, the check added at the top of
> this hunk assumes that cf may be NULL. This doesn't seem consistent.

The driver allocates the skb with:

	skb = alloc_can_err_skb(priv->netdev, &cf);

Which in turn calls alloc_can_skb(), which finally calls:

        *cf = skb_put_zero(skb, sizeof(struct can_frame));

To put the cf into the skb.

The newly added check "if (!skb)", takes care of skb allocation errors,
so that the de-referencing of "cf" is OK after this point.

regards,
Marc

P.S.:
IIRC smatch stumbled over the same pattern in another driver a while
back. Is there anything we can do about it?
diff mbox series

Patch

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index 3764b263add3..5ec8b300bebf 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -926,6 +926,42 @@  kvaser_usb_hydra_bus_status_to_can_state(const struct kvaser_usb_net_priv *priv,
 	}
 }
 
+static void kvaser_usb_hydra_change_state(struct kvaser_usb_net_priv *priv,
+					  const struct can_berr_counter *bec,
+					  struct can_frame *cf,
+					  enum can_state new_state)
+{
+	struct net_device *netdev = priv->netdev;
+	enum can_state old_state = priv->can.state;
+	enum can_state tx_state, rx_state;
+
+	tx_state = (bec->txerr >= bec->rxerr) ?
+				new_state : CAN_STATE_ERROR_ACTIVE;
+	rx_state = (bec->txerr <= bec->rxerr) ?
+				new_state : CAN_STATE_ERROR_ACTIVE;
+	can_change_state(netdev, cf, tx_state, rx_state);
+
+	if (new_state == CAN_STATE_BUS_OFF && old_state < CAN_STATE_BUS_OFF) {
+		if (priv->can.restart_ms == 0)
+			kvaser_usb_hydra_send_simple_cmd_async(priv, CMD_STOP_CHIP_REQ);
+
+		can_bus_off(netdev);
+	}
+
+	if (priv->can.restart_ms &&
+	    old_state >= CAN_STATE_BUS_OFF &&
+	    new_state < CAN_STATE_BUS_OFF) {
+		priv->can.can_stats.restarts++;
+		if (cf)
+			cf->can_id |= CAN_ERR_RESTARTED;
+	}
+	if (cf && new_state != CAN_STATE_BUS_OFF) {
+		cf->can_id |= CAN_ERR_CNT;
+		cf->data[6] = bec->txerr;
+		cf->data[7] = bec->rxerr;
+	}
+}
+
 static void kvaser_usb_hydra_update_state(struct kvaser_usb_net_priv *priv,
 					  u8 bus_status,
 					  const struct can_berr_counter *bec)
@@ -951,41 +987,11 @@  static void kvaser_usb_hydra_update_state(struct kvaser_usb_net_priv *priv,
 		return;
 
 	skb = alloc_can_err_skb(netdev, &cf);
-	if (skb) {
-		enum can_state tx_state, rx_state;
-
-		tx_state = (bec->txerr >= bec->rxerr) ?
-					new_state : CAN_STATE_ERROR_ACTIVE;
-		rx_state = (bec->txerr <= bec->rxerr) ?
-					new_state : CAN_STATE_ERROR_ACTIVE;
-		can_change_state(netdev, cf, tx_state, rx_state);
-	}
-
-	if (new_state == CAN_STATE_BUS_OFF && old_state < CAN_STATE_BUS_OFF) {
-		if (!priv->can.restart_ms)
-			kvaser_usb_hydra_send_simple_cmd_async
-						(priv, CMD_STOP_CHIP_REQ);
-
-		can_bus_off(netdev);
-	}
-
-	if (!skb) {
+	kvaser_usb_hydra_change_state(priv, bec, cf, new_state);
+	if (skb)
+		netif_rx(skb);
+	else
 		netdev_warn(netdev, "No memory left for err_skb\n");
-		return;
-	}
-
-	if (priv->can.restart_ms &&
-	    old_state >= CAN_STATE_BUS_OFF &&
-	    new_state < CAN_STATE_BUS_OFF)
-		priv->can.can_stats.restarts++;
-
-	if (new_state != CAN_STATE_BUS_OFF) {
-		cf->can_id |= CAN_ERR_CNT;
-		cf->data[6] = bec->txerr;
-		cf->data[7] = bec->rxerr;
-	}
-
-	netif_rx(skb);
 }
 
 static void kvaser_usb_hydra_state_event(const struct kvaser_usb *dev,
@@ -1080,7 +1086,6 @@  kvaser_usb_hydra_error_frame(struct kvaser_usb_net_priv *priv,
 	struct net_device_stats *stats = &netdev->stats;
 	struct can_frame *cf;
 	struct sk_buff *skb;
-	struct skb_shared_hwtstamps *shhwtstamps;
 	struct can_berr_counter bec;
 	enum can_state new_state, old_state;
 	u8 bus_status;
@@ -1097,51 +1102,22 @@  kvaser_usb_hydra_error_frame(struct kvaser_usb_net_priv *priv,
 						 &new_state);
 
 	skb = alloc_can_err_skb(netdev, &cf);
+	if (new_state != old_state)
+		kvaser_usb_hydra_change_state(priv, &bec, cf, new_state);
 
-	if (new_state != old_state) {
-		if (skb) {
-			enum can_state tx_state, rx_state;
+	if (skb) {
+		struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
 
-			tx_state = (bec.txerr >= bec.rxerr) ?
-					new_state : CAN_STATE_ERROR_ACTIVE;
-			rx_state = (bec.txerr <= bec.rxerr) ?
-					new_state : CAN_STATE_ERROR_ACTIVE;
-
-			can_change_state(netdev, cf, tx_state, rx_state);
-
-			if (priv->can.restart_ms &&
-			    old_state >= CAN_STATE_BUS_OFF &&
-			    new_state < CAN_STATE_BUS_OFF)
-				cf->can_id |= CAN_ERR_RESTARTED;
-		}
-
-		if (new_state == CAN_STATE_BUS_OFF) {
-			if (!priv->can.restart_ms)
-				kvaser_usb_hydra_send_simple_cmd_async
-						(priv, CMD_STOP_CHIP_REQ);
-
-			can_bus_off(netdev);
-		}
-	}
-
-	if (!skb) {
-		stats->rx_dropped++;
-		netdev_warn(netdev, "No memory left for err_skb\n");
-		return;
-	}
-
-	shhwtstamps = skb_hwtstamps(skb);
-	shhwtstamps->hwtstamp = hwtstamp;
-
-	cf->can_id |= CAN_ERR_BUSERROR;
-	if (new_state != CAN_STATE_BUS_OFF) {
-		cf->can_id |= CAN_ERR_CNT;
+		shhwtstamps->hwtstamp = hwtstamp;
+		cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_CNT;
 		cf->data[6] = bec.txerr;
 		cf->data[7] = bec.rxerr;
+		netif_rx(skb);
+	} else {
+		stats->rx_dropped++;
+		netdev_warn(netdev, "No memory left for err_skb\n");
 	}
 
-	netif_rx(skb);
-
 	priv->bec.txerr = bec.txerr;
 	priv->bec.rxerr = bec.rxerr;
 }
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 6b9122ab1464..0491b4a6d8e8 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -1121,8 +1121,6 @@  static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
 				     const struct kvaser_usb_err_summary *es)
 {
 	struct can_frame *cf;
-	struct can_frame tmp_cf = { .can_id = CAN_ERR_FLAG,
-				    .len = CAN_ERR_DLC };
 	struct sk_buff *skb;
 	struct net_device_stats *stats;
 	struct kvaser_usb_net_priv *priv;
@@ -1143,18 +1141,9 @@  static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
 	if (!netif_running(priv->netdev))
 		return;
 
-	/* Update all of the CAN interface's state and error counters before
-	 * trying any memory allocation that can actually fail with -ENOMEM.
-	 *
-	 * We send a temporary stack-allocated error CAN frame to
-	 * can_change_state() for the very same reason.
-	 *
-	 * TODO: Split can_change_state() responsibility between updating the
-	 * CAN interface's state and counters, and the setting up of CAN error
-	 * frame ID and data to userspace. Remove stack allocation afterwards.
-	 */
 	old_state = priv->can.state;
-	kvaser_usb_leaf_rx_error_update_can_state(priv, es, &tmp_cf);
+	skb = alloc_can_err_skb(priv->netdev, &cf);
+	kvaser_usb_leaf_rx_error_update_can_state(priv, es, cf);
 	new_state = priv->can.state;
 
 	/* If there are errors, request status updates periodically as we do
@@ -1168,13 +1157,6 @@  static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
 		schedule_delayed_work(&leaf->chip_state_req_work,
 				      msecs_to_jiffies(500));
 
-	skb = alloc_can_err_skb(priv->netdev, &cf);
-	if (!skb) {
-		stats->rx_dropped++;
-		return;
-	}
-	memcpy(cf, &tmp_cf, sizeof(*cf));
-
 	if (new_state != old_state) {
 		if (es->status &
 		    (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
@@ -1187,11 +1169,18 @@  static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
 		if (priv->can.restart_ms &&
 		    old_state == CAN_STATE_BUS_OFF &&
 		    new_state < CAN_STATE_BUS_OFF) {
-			cf->can_id |= CAN_ERR_RESTARTED;
+			if (cf)
+				cf->can_id |= CAN_ERR_RESTARTED;
 			netif_carrier_on(priv->netdev);
 		}
 	}
 
+	if (!skb) {
+		stats->rx_dropped++;
+		netdev_warn(priv->netdev, "No memory left for err_skb\n");
+		return;
+	}
+
 	switch (dev->driver_info->family) {
 	case KVASER_LEAF:
 		if (es->leaf.error_factor) {