netdev - Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <da0551556e42fd67c0b743d6d066fb09702571ef.camel@esd.eu>
Date:   Thu, 9 Feb 2023 19:00:54 +0000
From:   Frank Jungclaus <Frank.Jungclaus@....eu>
To:     "mkl@...gutronix.de" <mkl@...gutronix.de>
CC:     Stefan Mätje <Stefan.Maetje@....eu>,
        "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
        "wg@...ndegger.com" <wg@...ndegger.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mailhol.vincent@...adoo.fr" <mailhol.vincent@...adoo.fr>
Subject: Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT
 event (2)

On Thu, 2023-02-02 at 16:22 +0100, Marc Kleine-Budde wrote:
> On 23.01.2023 15:47:22, Frank Jungclaus wrote:
> > On Thu, 2022-12-22 at 11:21 +0900, Vincent MAILHOL wrote:
> > > On Thu. 22 Dec. 2022 at 03:42, Frank Jungclaus <Frank.Jungclaus@....eu> wrote:
> > > > On Tue, 2022-12-20 at 14:49 +0900, Vincent MAILHOL wrote:
> > > > > On Tue. 20 Dec. 2022 at 06:29, Frank Jungclaus <frank.jungclaus@....eu> wrote:
> > > > > > Started a rework initiated by Vincents remarks "You should not report
> > > > > > the greatest of txerr and rxerr but the one which actually increased."
> > > > > > [1]
> > > > > 
> > > > > I do not see this comment being addressed. You are still assigning the
> > > > > flags depending on the highest value, not the one which actually
> > > > > changed.
> > > > 
> > > > 
> > > > Yes, I'm assigning depending on the highest value, but from my point of
> > > > view doing so is analogue to what is done by can_change_state().
> > > 
> > > On the surface, it may look similar. But if you look into details,
> > > can_change_state() is only called when there is a change on enum
> > > can_state. enum can_state is the global state and does not
> > > differentiate the RX and TX.
> > > 
> > > I will give an example. Imagine that:
> > > 
> > >   - txerr is 128 (ERROR_PASSIVE)
> > >   - rxerr is 95 (ERROR_ACTIVE)
> > > 
> > > Imagine that rxerr then increases to 96. If you call
> > > can_change_state() under this condition, the old state:
> > > can_priv->state is still equal to the new one: max(tx_state, rx_state)
> > > and you would get the oops message:
> > > 
> > >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/dev.c#L100
> > > 
> > > So can_change_state() is indeed correct because it excludes the case
> > > when the smallest err counter changed.
> > > 
> > > > And
> > > > it should be fine, because e.g. my "case ESD_BUSSTATE_WARN:" is reached
> > > > exactly once while the transition from ERROR_ACTIVE to
> > > > ERROR_WARN. Than one of rec or tec is responsible for this
> > > > transition.
> > > > There is no second pass for "case ESD_BUSSTATE_WARN:"
> > > > when e.g. rec is already on WARN (or above) and now tec also reaches
> > > > WARN.
> > > > Man, this is even difficult to explain in German language ;)
> > > 
> > > OK. This is new information. I agree that it should work. But I am
> > > still puzzled because the code doesn't make this limitation apparent.
> > > 
> > > Also, as long as you have the rxerr and txerr value, you should still
> > > be able to set the correct flag by comparing the err counters instead
> > > of relying on your device events.
> > > 
> > 
> > I agree, this would be an option. But I dislike the fact that then
> > - beside the USB firmware - there is a second instance which decides on
> > the bus state. I'll send a reworked patch which makes use of
>                       ^^^^^^^^^^^^^^^^^^^^^
> > can_change_state(). Hopefully that will address your concerns ;)
> > This also will fix the imperfection, that our current code e.g. does
> > an error_warning++ when going back in direction of ERROR_ACTIVE ...
> 
> Not taking this series, waiting for the reworked version.
> 
> Marc
> 
Marc, can I just send a reworked patch of [PATCH 2/3], let's say
with subject [PATCH v2 2/3] as a reply to this thread or should I
better resend the complete patch series as [PATCH v2 0/3] up to
[PATCH v2 3/3]?

Regards Frank