| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5c68ff433c708f517e841885314ecd90b263e888.camel@esd.eu>
Date: Thu, 9 Dec 2021 23:21:49 +0000
From: Stefan Mätje <Stefan.Maetje@....eu>
To: "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
"mailhol.vincent@...adoo.fr" <mailhol.vincent@...adoo.fr>,
"mkl@...gutronix.de" <mkl@...gutronix.de>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-sunxi@...ts.linux.dev" <linux-sunxi@...ts.linux.dev>,
"socketcan@...tkopp.net" <socketcan@...tkopp.net>,
"extja@...ser.com" <extja@...ser.com>
Subject: Re: [PATCH v5 0/5] fix statistics and payload issues for error
Hello Vincent,
I now had some time to test the patches [1/5], [4/5] and [5/5] for esd_usb2.c
and they work as expected. You may add
For esd_usb2.c
Acked-by: Stefan Mätje <stefan.maetje@....eu>
Tested-by: Stefan Mätje <stefan.maetje@....eu>
Thanks for the work changing all the drivers.
Best regards,
Stefan Mätje
Am Dienstag, den 07.12.2021, 21:15 +0900 schrieb Vincent Mailhol:
> Important: this patch series depends on below patch:
> https://lore.kernel.org/linux-can/20211123111654.621610-1-mailhol.vincent@wanadoo.fr/T/#u
>
> There are some common errors which are made when updating the network
> statistics or processing the CAN payload:
>
> 1. Incrementing the "normal" stats when generating or sending a CAN
> error message frame. Error message frames are an abstraction of
> Socket CAN and do not exist on the wire. The first patch of this
> series fixes the RX stats for 22 different drivers, the second one
> fixes the TX stasts for the kvaser driver (N.B. only this driver is
> capable of sending error on the bus).
>
> 2. Copying the payload of RTR frames: RTR frames have no payload and
> the data buffer only contains garbage. The DLC/length should not be
> used to do a memory copy. The third patch of this series address
> this issue for 3 different drivers.
>
> 3. Counting the length of the Remote Transmission Frames (RTR). The
> length of an RTR frame is the length of the requested frame not the
> actual payload. In reality the payload of an RTR frame is always 0
> bytes long. The fourth patch of this series fixes the RX stats for
> 27 different drivers and the fifth one fixes the TX stats for 25
> different ones.
>
>
> * Changelog *
>
> v4 -> v5:
>
> * at91_can: netdev_tx_t at91_start_xmit: replace
> | dev->stats.tx_bytes = ...
> by
> | dev->stats.tx_bytes += ...
>
> v3 -> v4:
>
> * patch 2/5: kvaser: include the suggestion from Jimmy Assarsson so
> that we don't need to get the original CAN frame anymore to
> determine whether or not it was an error frame. patch 5/5 is
> rebased accordingly.
>
> * patch 5/5: kvaser: kvaser_usb_hydra_frame_to_cmd_std: remove
> unrelated change.
>
> * patch 5/5: slcan: better factorize code for the tx RTR frames
> (reorder the line instead of adding a new "if" branch).
>
>
> v2 -> v3:
>
> * Fix an issue in the fourth patch ("do not increase rx_bytes
> statistics for RTR frames"). In ucan_rx_can_msg() of the ucan
> driver, the changes in v2 made no sense. Reverted it to v1.
>
>
> v1 -> v2:
>
> * can_rx_offload_napi_poll: v1 used CAN_ERR_MASK instead of
> CAN_ERR_FLAG. Fixed the issue.
>
> * use correct vocabulary. The correct term to designate the Socket
> CAN specific error skb is "error message frames" not "error
> frames". "error frames" is used in the standard and has a
> different meaning.
>
> * better factorize code for the rx RTR frames. Most of the drivers
> already have a switch to check if the frame is a RTR. Moved the
> instruction to increase net_device_stats:rx_bytes inside the else
> branch of those switches whenever possible (for some drivers with
> some complex logic, putting and additional RTR check was easier).
>
> * add a patch which prevent drivers to copy the payload of RTR
> frames.
>
> * add a patch to cover the tx RTR frames (the fifth patch of
> v2). The tx RTR frames issue was supposedly covered by the
> can_get_echo_skb() function which returns the correct length for
> drivers to increase their stats. However, the reality is that most
> of the drivers do not check this value and instead use a local
> copy of the length/dlc.
>
> Vincent Mailhol (5):
> can: do not increase rx statistics when generating a CAN rx error
> message frame
> can: kvaser_usb: do not increase tx statistics when sending error
> message frames
> can: do not copy the payload of RTR frames
> can: do not increase rx_bytes statistics for RTR frames
> can: do not increase tx_bytes statistics for RTR frames
>
> drivers/net/can/at91_can.c | 18 ++---
> drivers/net/can/c_can/c_can.h | 1 -
> drivers/net/can/c_can/c_can_main.c | 16 ++---
> drivers/net/can/cc770/cc770.c | 16 ++---
> drivers/net/can/dev/dev.c | 4 --
> drivers/net/can/dev/rx-offload.c | 7 +-
> drivers/net/can/grcan.c | 6 +-
> drivers/net/can/ifi_canfd/ifi_canfd.c | 11 +--
> drivers/net/can/janz-ican3.c | 6 +-
> drivers/net/can/kvaser_pciefd.c | 16 ++---
> drivers/net/can/m_can/m_can.c | 13 +---
> drivers/net/can/mscan/mscan.c | 14 ++--
> drivers/net/can/pch_can.c | 33 ++++-----
> drivers/net/can/peak_canfd/peak_canfd.c | 14 ++--
> drivers/net/can/rcar/rcar_can.c | 22 +++---
> drivers/net/can/rcar/rcar_canfd.c | 13 +---
> drivers/net/can/sja1000/sja1000.c | 11 ++-
> drivers/net/can/slcan.c | 7 +-
> drivers/net/can/softing/softing_main.c | 8 +--
> drivers/net/can/spi/hi311x.c | 31 ++++----
> drivers/net/can/spi/mcp251x.c | 31 ++++----
> drivers/net/can/sun4i_can.c | 22 +++---
> drivers/net/can/usb/ems_usb.c | 14 ++--
> drivers/net/can/usb/esd_usb2.c | 13 ++--
> drivers/net/can/usb/etas_es58x/es58x_core.c | 7 --
> drivers/net/can/usb/gs_usb.c | 7 +-
> drivers/net/can/usb/kvaser_usb/kvaser_usb.h | 5 +-
> .../net/can/usb/kvaser_usb/kvaser_usb_core.c | 4 +-
> .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 71 +++++++++----------
> .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 20 ++----
> drivers/net/can/usb/mcba_usb.c | 23 +++---
> drivers/net/can/usb/peak_usb/pcan_usb.c | 9 ++-
> drivers/net/can/usb/peak_usb/pcan_usb_core.c | 20 +++---
> drivers/net/can/usb/peak_usb/pcan_usb_core.h | 1 -
> drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 11 ++-
> drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 12 ++--
> drivers/net/can/usb/ucan.c | 17 +++--
> drivers/net/can/usb/usb_8dev.c | 17 ++---
> drivers/net/can/vcan.c | 7 +-
> drivers/net/can/vxcan.c | 2 +-
> drivers/net/can/xilinx_can.c | 19 +++--
> include/linux/can/skb.h | 5 +-
> 42 files changed, 254 insertions(+), 350 deletions(-)
>
>
> base-commit: 11f2c3c57f37befb1af6ccac0defacb813411d9d
Powered by blists - more mailing lists