| 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: <CAMZ6RqKefpH73tffY-DrXasU-UCm-UvwCbjtPhMJ_9PmfnBUQw@mail.gmail.com>
Date: Tue, 7 Dec 2021 21:12:02 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: linux-can@...r.kernel.org,
Oliver Hartkopp <socketcan@...tkopp.net>,
Jimmy Assarsson <extja@...ser.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-sunxi@...ts.linux.dev,
Nicolas Ferre <nicolas.ferre@...rochip.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Ludovic Desroches <ludovic.desroches@...rochip.com>,
Maxime Ripard <mripard@...nel.org>,
Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Yasushi SHOJI <yashi@...cecubics.com>,
Stephane Grosjean <s.grosjean@...k-system.com>
Subject: Re: [PATCH v4 5/5] can: do not increase tx_bytes statistics for RTR frames
On Mon. 6 Dec 2021 at 23:19, Marc Kleine-Budde <mkl@...gutronix.de> wrote:
> On 03.12.2021 22:18:08, Vincent Mailhol wrote:
> > The actual payload length of the CAN Remote Transmission Request (RTR)
> > frames is always 0, i.e. nothing is transmitted on the wire. However,
> > those RTR frames still use the DLC to indicate the length of the
> > requested frame.
> >
> > As such, net_device_stats:tx_bytes should not be increased when
> > sending RTR frames.
> >
> > The function can_get_echo_skb() already returns the correct length,
> > even for RTR frames (c.f. [1]). However, for historical reasons, the
> > drivers do not use can_get_echo_skb()'s return value and instead, most
> > of them store a temporary length (or dlc) in some local structure or
> > array. Using the return value of can_get_echo_skb() solves the
> > issue. After doing this, such length/dlc fields become unused and so
> > this patch does the adequate cleaning when needed.
> >
> > This patch fixes all the CAN drivers.
> >
> > Finally, can_get_echo_skb() is decorated with the __must_check
> > attribute in order to force future drivers to correctly use its return
> > value (else the compiler would emit a warning).
> >
> > [1] commit ed3320cec279 ("can: dev: __can_get_echo_skb():
> > fix real payload length return value for RTR frames")
> >
> > CC: Nicolas Ferre <nicolas.ferre@...rochip.com>
> > CC: Alexandre Belloni <alexandre.belloni@...tlin.com>
> > CC: Ludovic Desroches <ludovic.desroches@...rochip.com>
> > CC: Maxime Ripard <mripard@...nel.org>
> > CC: Chen-Yu Tsai <wens@...e.org>
> > CC: Jernej Skrabec <jernej.skrabec@...il.com>
> > CC: Yasushi SHOJI <yashi@...cecubics.com>
> > CC: Oliver Hartkopp <socketcan@...tkopp.net>
> > CC: Stephane Grosjean <s.grosjean@...k-system.com>
> > Tested-by: Jimmy Assarsson <extja@...ser.com>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> > ---
> > drivers/net/can/at91_can.c | 8 ++--
> > drivers/net/can/c_can/c_can.h | 1 -
> > drivers/net/can/c_can/c_can_main.c | 7 +---
> > drivers/net/can/cc770/cc770.c | 8 +---
> > drivers/net/can/janz-ican3.c | 3 +-
> > drivers/net/can/mscan/mscan.c | 4 +-
> > drivers/net/can/pch_can.c | 9 ++--
> > drivers/net/can/peak_canfd/peak_canfd.c | 3 +-
> > drivers/net/can/rcar/rcar_can.c | 11 +++--
> > drivers/net/can/rcar/rcar_canfd.c | 6 +--
> > drivers/net/can/sja1000/sja1000.c | 4 +-
> > drivers/net/can/slcan.c | 3 +-
> > drivers/net/can/softing/softing_main.c | 8 ++--
> > drivers/net/can/spi/hi311x.c | 24 +++++------
> > drivers/net/can/spi/mcp251x.c | 25 +++++------
> > drivers/net/can/sun4i_can.c | 5 +--
> > drivers/net/can/usb/ems_usb.c | 7 +---
> > drivers/net/can/usb/esd_usb2.c | 6 +--
> > 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 | 2 +-
> > .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 42 +++++++++----------
> > .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 13 +++---
> > drivers/net/can/usb/mcba_usb.c | 12 ++----
> > 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/ucan.c | 10 ++---
> > drivers/net/can/usb/usb_8dev.c | 6 +--
> > drivers/net/can/vcan.c | 7 ++--
> > drivers/net/can/vxcan.c | 2 +-
> > include/linux/can/skb.h | 5 ++-
> > 31 files changed, 114 insertions(+), 160 deletions(-)
> >
> > diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
> > index 97f1d08b4133..b37d9b4f508e 100644
> > --- a/drivers/net/can/at91_can.c
> > +++ b/drivers/net/can/at91_can.c
> > @@ -448,7 +448,6 @@ static void at91_chip_stop(struct net_device *dev, enum can_state state)
> > static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > {
> > struct at91_priv *priv = netdev_priv(dev);
> > - struct net_device_stats *stats = &dev->stats;
> > struct can_frame *cf = (struct can_frame *)skb->data;
> > unsigned int mb, prio;
> > u32 reg_mid, reg_mcr;
> > @@ -480,8 +479,6 @@ static netdev_tx_t at91_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > /* This triggers transmission */
> > at91_write(priv, AT91_MCR(mb), reg_mcr);
> >
> > - stats->tx_bytes += cf->len;
> > -
> > /* _NOTE_: subtract AT91_MB_TX_FIRST offset from mb! */
> > can_put_echo_skb(skb, dev, mb - get_mb_tx_first(priv), 0);
> >
> > @@ -852,7 +849,10 @@ static void at91_irq_tx(struct net_device *dev, u32 reg_sr)
> > if (likely(reg_msr & AT91_MSR_MRDY &&
> > ~reg_msr & AT91_MSR_MABT)) {
> > /* _NOTE_: subtract AT91_MB_TX_FIRST offset from mb! */
> > - can_get_echo_skb(dev, mb - get_mb_tx_first(priv), NULL);
> > + dev->stats.tx_bytes =
> += ?
Absolutely. I will check that the other assignments are also correct
and send a v5.
Yours sincerely,
Vincent Mailhol
Powered by blists - more mailing lists