[PATCH 1/3] can: peak_usb: fix potential "UBSAN: shitf-out-of-bounds" issue

[PATCH 1/3] can: peak_usb: fix potential "UBSAN: shitf-out-of-bounds" issue

[Stephane Grosjean]

Shift exponent 32 is too large for 32-bit type 'int' so u64 values are
used instead.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 1efa39e134f4..edecb6e09c3c 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -108,10 +108,10 @@ void peak_usb_update_ts_now(struct peak_time_ref *time_ref, u32 ts_now)
 
 	/* should wait at least two passes before computing */
 	if (ktime_to_ns(time_ref->tv_host) > 0) {
-		u32 delta_ts = time_ref->ts_dev_2 - time_ref->ts_dev_1;
+		u64 delta_ts = time_ref->ts_dev_2 - time_ref->ts_dev_1;
 
 		if (time_ref->ts_dev_2 < time_ref->ts_dev_1)
-			delta_ts &= (1 << time_ref->adapter->ts_used_bits) - 1;
+			delta_ts &= (1UL << time_ref->adapter->ts_used_bits) - 1;
 
 		time_ref->ts_total += delta_ts;
 	}
-- 
2.34.1

[PATCH 2/3] can: peak_usb: increase rx buffer size used for USB bulk transfer

[Stephane Grosjean]

In case of high bud load, 2KB can sometimes be too small for the device,
leading to potential fragmentation of USB records, while 4KB better suits
its internal buffer size.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 4d85b29a17b7..a1c339716776 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -30,7 +30,7 @@
 #define PCAN_UFD_CMD_TIMEOUT_MS		1000
 
 /* PCAN-USB Pro FD rx/tx buffers size */
-#define PCAN_UFD_RX_BUFFER_SIZE		2048
+#define PCAN_UFD_RX_BUFFER_SIZE		4096
 #define PCAN_UFD_TX_BUFFER_SIZE		512
 
 /* struct pcan_ufd_fw_info::type */
-- 
2.34.1

[PATCH 3/3] can: peak_usb: fix potential kernel log flooding

[Stephane Grosjean]

In rare cases of very high bus load, the firmware can generate messages
warning that the receive cache capacity is about to be exceeded.
This modification prevents the driver from flooding the kernel log with
messages and memory dumps that are far too verbose in such cases,
by limiting their production to once per session.

Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
 drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index a1c339716776..d444ff0fa7cc 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -70,6 +70,7 @@ struct pcan_usb_fd_if {
 struct pcan_usb_fd_device {
 	struct peak_usb_device	dev;
 	struct can_berr_counter	bec;
+	bool			rx_cache_warn_handled;
 	struct pcan_usb_fd_if	*usb_if;
 	u8			*cmd_buffer_addr;
 };
@@ -667,6 +668,28 @@ static int pcan_usb_fd_decode_error(struct pcan_usb_fd_if *usb_if,
 	return 0;
 }
 
+/* Handle uCAN Rx cache warning messages.
+ *
+ * Such messages SHOULD NOT occur. If they do, then this might come from
+ * massive PING host flooding that prevents PCAN-USB Pro FD HW v4 to handle
+ * CAN traffic anymore. In this case, the driver itself manages the display of
+ * the warning message.
+ */
+static void pcan_usb_fd_handle_rx_cache_warn(struct peak_usb_device *dev,
+					     struct pucan_msg *rx_msg)
+{
+	struct pcan_usb_fd_device *pdev =
+			container_of(dev, struct pcan_usb_fd_device, dev);
+
+	if (pdev->rx_cache_warn_handled)
+		return;
+
+	netdev_warn(dev->netdev,
+		    "Rx cache size warning! Possible loss of frames\n");
+
+	pdev->rx_cache_warn_handled = true;
+}
+
 /* handle uCAN overrun message */
 static int pcan_usb_fd_decode_overrun(struct pcan_usb_fd_if *usb_if,
 				      struct pucan_msg *rx_msg)
@@ -768,6 +791,14 @@ static int pcan_usb_fd_decode_buf(struct peak_usb_device *dev, struct urb *urb)
 				goto fail;
 			break;
 
+		case PUCAN_MSG_CACHE_CRITICAL:
+			pcan_usb_fd_handle_rx_cache_warn(dev, rx_msg);
+
+			/* Rx cache warning means possible overrun cases in
+			 * the device.
+			 */
+			fallthrough;
+
 		case PCAN_UFD_MSG_OVERRUN:
 			err = pcan_usb_fd_decode_overrun(usb_if, rx_msg);
 			if (err < 0)
@@ -885,6 +916,11 @@ static int pcan_usb_fd_start(struct peak_usb_device *dev)
 	pdev->bec.txerr = 0;
 	pdev->bec.rxerr = 0;
 
+	/* warn of a cache problem only once per session, to avoid flooding
+	 * the kernel log.
+	 */
+	pdev->rx_cache_warn_handled = false;
+
 	return err;
 }
 
-- 
2.34.1

Re: [PATCH 3/3] can: peak_usb: fix potential kernel log flooding

[Vincent Mailhol]

Hi Stéphane,

On Fri. 16 Feb. 2024 at 00:27, Stephane Grosjean
<s.grosjean@peak-system.com> wrote:
> In rare cases of very high bus load, the firmware can generate messages
> warning that the receive cache capacity is about to be exceeded.
> This modification prevents the driver from flooding the kernel log with
> messages and memory dumps that are far too verbose in such cases,
> by limiting their production to once per session.
>
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index a1c339716776..d444ff0fa7cc 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -70,6 +70,7 @@ struct pcan_usb_fd_if {
>  struct pcan_usb_fd_device {
>         struct peak_usb_device  dev;
>         struct can_berr_counter bec;
> +       bool                    rx_cache_warn_handled;
>         struct pcan_usb_fd_if   *usb_if;
>         u8                      *cmd_buffer_addr;
>  };
> @@ -667,6 +668,28 @@ static int pcan_usb_fd_decode_error(struct pcan_usb_fd_if *usb_if,
>         return 0;
>  }
>
> +/* Handle uCAN Rx cache warning messages.
> + *
> + * Such messages SHOULD NOT occur. If they do, then this might come from
> + * massive PING host flooding that prevents PCAN-USB Pro FD HW v4 to handle
> + * CAN traffic anymore. In this case, the driver itself manages the display of
> + * the warning message.
> + */
> +static void pcan_usb_fd_handle_rx_cache_warn(struct peak_usb_device *dev,
> +                                            struct pucan_msg *rx_msg)
> +{
> +       struct pcan_usb_fd_device *pdev =
> +                       container_of(dev, struct pcan_usb_fd_device, dev);
> +
> +       if (pdev->rx_cache_warn_handled)
> +               return;
> +
> +       netdev_warn(dev->netdev,
> +                   "Rx cache size warning! Possible loss of frames\n");

Did you consider using netdev_warn_once?

  https://elixir.bootlin.com/linux/v6.7/source/include/net/net_debug.h#L46

This seems to do pretty much what you want.

FYI, the net_ratelimit() may also be helpful here:

        if (net_ratelimit())
                netdev_warn(...);

> +       pdev->rx_cache_warn_handled = true;
> +}
> +
>  /* handle uCAN overrun message */
>  static int pcan_usb_fd_decode_overrun(struct pcan_usb_fd_if *usb_if,
>                                       struct pucan_msg *rx_msg)
> @@ -768,6 +791,14 @@ static int pcan_usb_fd_decode_buf(struct peak_usb_device *dev, struct urb *urb)
>                                 goto fail;
>                         break;
>
> +               case PUCAN_MSG_CACHE_CRITICAL:
> +                       pcan_usb_fd_handle_rx_cache_warn(dev, rx_msg);
> +
> +                       /* Rx cache warning means possible overrun cases in
> +                        * the device.
> +                        */
> +                       fallthrough;
> +
>                 case PCAN_UFD_MSG_OVERRUN:
>                         err = pcan_usb_fd_decode_overrun(usb_if, rx_msg);
>                         if (err < 0)
> @@ -885,6 +916,11 @@ static int pcan_usb_fd_start(struct peak_usb_device *dev)
>         pdev->bec.txerr = 0;
>         pdev->bec.rxerr = 0;
>
> +       /* warn of a cache problem only once per session, to avoid flooding
> +        * the kernel log.
> +        */
> +       pdev->rx_cache_warn_handled = false;
> +
>         return err;
>  }

RE: [PATCH 3/3] can: peak_usb: fix potential kernel log flooding

[Stéphane Grosjean]

Hi Vincent,

Oh ok I didn't know this facility. I'm pushing a new patch that includes it. Thanks for that!
FYI the frequency and number of these notifications means net_ratelimit() may not be enough in the long run.

— Stéphane


De : Vincent Mailhol <vincent.mailhol@gmail.com>
Envoyé : vendredi 16 février 2024 02:37
À : Stéphane Grosjean <s.grosjean@peak-system.com>
Cc : linux-can Mailing List <linux-can@vger.kernel.org>
Objet : Re: [PATCH 3/3] can: peak_usb: fix potential kernel log flooding

Hi Stéphane,

On Fri. 16 Feb. 2024 at 00:27, Stephane Grosjean
<s.grosjean@peak-system.com> wrote:
> In rare cases of very high bus load, the firmware can generate messages
> warning that the receive cache capacity is about to be exceeded.
> This modification prevents the driver from flooding the kernel log with
> messages and memory dumps that are far too verbose in such cases,
> by limiting their production to once per session.
>
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
> ---
>  drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index a1c339716776..d444ff0fa7cc 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -70,6 +70,7 @@ struct pcan_usb_fd_if {
>  struct pcan_usb_fd_device {
>         struct peak_usb_device  dev;
>         struct can_berr_counter bec;
> +       bool                    rx_cache_warn_handled;
>         struct pcan_usb_fd_if   *usb_if;
>         u8                      *cmd_buffer_addr;
>  };
> @@ -667,6 +668,28 @@ static int pcan_usb_fd_decode_error(struct pcan_usb_fd_if *usb_if,
>         return 0;
>  }
>
> +/* Handle uCAN Rx cache warning messages.
> + *
> + * Such messages SHOULD NOT occur. If they do, then this might come from
> + * massive PING host flooding that prevents PCAN-USB Pro FD HW v4 to handle
> + * CAN traffic anymore. In this case, the driver itself manages the display of
> + * the warning message.
> + */
> +static void pcan_usb_fd_handle_rx_cache_warn(struct peak_usb_device *dev,
> +                                            struct pucan_msg *rx_msg)
> +{
> +       struct pcan_usb_fd_device *pdev =
> +                       container_of(dev, struct pcan_usb_fd_device, dev);
> +
> +       if (pdev->rx_cache_warn_handled)
> +               return;
> +
> +       netdev_warn(dev->netdev,
> +                   "Rx cache size warning! Possible loss of frames\n");

Did you consider using netdev_warn_once?

  https://elixir.bootlin.com/linux/v6.7/source/include/net/net_debug.h#L46

This seems to do pretty much what you want.

FYI, the net_ratelimit() may also be helpful here:

        if (net_ratelimit())
                netdev_warn(...);

> +       pdev->rx_cache_warn_handled = true;
> +}
> +
>  /* handle uCAN overrun message */
>  static int pcan_usb_fd_decode_overrun(struct pcan_usb_fd_if *usb_if,
>                                       struct pucan_msg *rx_msg)
> @@ -768,6 +791,14 @@ static int pcan_usb_fd_decode_buf(struct peak_usb_device *dev, struct urb *urb)
>                                 goto fail;
>                         break;
>
> +               case PUCAN_MSG_CACHE_CRITICAL:
> +                       pcan_usb_fd_handle_rx_cache_warn(dev, rx_msg);
> +
> +                       /* Rx cache warning means possible overrun cases in
> +                        * the device.
> +                        */
> +                       fallthrough;
> +
>                 case PCAN_UFD_MSG_OVERRUN:
>                         err = pcan_usb_fd_decode_overrun(usb_if, rx_msg);
>                         if (err < 0)
> @@ -885,6 +916,11 @@ static int pcan_usb_fd_start(struct peak_usb_device *dev)
>         pdev->bec.txerr = 0;
>         pdev->bec.rxerr = 0;
>
> +       /* warn of a cache problem only once per session, to avoid flooding
> +        * the kernel log.
> +        */
> +       pdev->rx_cache_warn_handled = false;
> +
>         return err;
>  }

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
GL: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung finden Sie hier
www.peak-system.com/Datenschutz.483.0.html

Re: [PATCH 1/3] can: peak_usb: fix potential "UBSAN: shitf-out-of-bounds" issue

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On 15.02.2024 16:26:54, Stephane Grosjean wrote:
> Shift exponent 32 is too large for 32-bit type 'int' so u64 values are
> used instead.

FYI: Consider migrating this code to timecounter_read/cyclecounter. See

| https://elixir.bootlin.com/linux/v6.7/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c

for an example.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] can: peak_usb: fix potential kernel log flooding

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On 16.02.2024 10:37:48, Vincent Mailhol wrote:
> > +static void pcan_usb_fd_handle_rx_cache_warn(struct peak_usb_device *dev,
> > +                                            struct pucan_msg *rx_msg)
> > +{
> > +       struct pcan_usb_fd_device *pdev =
> > +                       container_of(dev, struct pcan_usb_fd_device, dev);
> > +
> > +       if (pdev->rx_cache_warn_handled)
> > +               return;
> > +
> > +       netdev_warn(dev->netdev,
> > +                   "Rx cache size warning! Possible loss of frames\n");
> 
> Did you consider using netdev_warn_once?
> 
>   https://elixir.bootlin.com/linux/v6.7/source/include/net/net_debug.h#L46
> 
> This seems to do pretty much what you want.
> 
> FYI, the net_ratelimit() may also be helpful here:
> 
>         if (net_ratelimit())
>                 netdev_warn(...);

There is also dev_warn_ratelimited(), but this does not output the
associated netdev. This could be useful if an error occurs on USB
devices with multiple network devices and the error cannot be linked to
a network device. But AFAICS there is no netdev_warn_ratelimited().

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] can: peak_usb: fix potential "UBSAN: shitf-out-of-bounds" issue

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

On 16.02.2024 14:22:02, Marc Kleine-Budde wrote:
> On 15.02.2024 16:26:54, Stephane Grosjean wrote:
> > Shift exponent 32 is too large for 32-bit type 'int' so u64 values are
> > used instead.
> 
> FYI: Consider migrating this code to timecounter_read/cyclecounter. See

This patch is good as it is, the conversion to
timecounter/cyclecounter is future work.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

AW: [PATCH 1/3] can: peak_usb: fix potential "UBSAN: shitf-out-of-bounds" issue

[Sven Schuchmann]

Von: Stephane Grosjean <s.grosjean@peak-system.com>
Gesendet: Donnerstag, 15. Februar 2024 16:26
An: linux-can Mailing List <linux-can@vger.kernel.org>
Cc: Stephane Grosjean <s.grosjean@peak-system.com>
Betreff: [PATCH 1/3] can: peak_usb: fix potential "UBSAN: shitf-out-of-bounds" issue
 
Probably the subject should be "shift" not "shitf"...

Regards Sven