netdev - Re: [PATCH net] mctp i3c: fix MCTP I3C driver multi-thread issue

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] [day] [month] [year] [list]

Message-ID: <CAAfUjZFJRXDOfDjzdiaSgaDKpu3sJCD=2zFQ4APhK_aLXSX5cg@mail.gmail.com>
Date: Tue, 7 Jan 2025 09:29:32 +0800
From: Leo Yang <leo.yang.sy0@...il.com>
To: Jeremy Kerr <jk@...econstruct.com.au>
Cc: matt@...econstruct.com.au, andrew+netdev@...n.ch, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Leo Yang <Leo-Yang@...ntatw.com>
Subject: Re: [PATCH net] mctp i3c: fix MCTP I3C driver multi-thread issue

On Fri, Jan 3, 2025 at 10:28 AM Jeremy Kerr <jk@...econstruct.com.au> wrote:

Hi Jeremy,

>
> Mostly out of curiosity, could you share a little detail about what you
> were observing with that read behaviour? Were the IBIs being handed by
> different CPUs in that case?
>
> I assume that you were seeing the netif_rx() out of sequence with the
> skbs populated from i3c_device_do_priv_xfers(), is that right?
>

Yes, in our test environment, I can observe this issue by making a
request via BMC -> BIC.
and the BIC replies with multiple-packet messages.

Then there is a chance that we can observe the following situation
(trimmed out to avoid long messages in the mail)
i3c from within i3c_device_do_priv_xfers() and
mctp-i3c from messages sent by netif_rx()

[  120.179246] i3c i3c-1: nresp:1, Rx:01:08:50:80:
[  120.282348] i3c i3c-1: nresp:1, Rx:01:08:50:10:
[  120.326819] mctp-i3c 1-7ec80010023: NET_RX_SUCCESS: 01:08:50:80:
[  120.433935] i3c i3c-1: nresp:1, Rx:01:08:50:20:
[  120.478631] mctp-i3c 1-7ec80010023: NET_RX_SUCCESS: 01:08:50:20:
[  120.526682] mctp-i3c 1-7ec80010023: NET_RX_SUCCESS: 01:08:50:10:
[  120.633453] i3c i3c-1: nresp:1, Rx:01:08:50:30:
[  120.736494] i3c i3c-1: nresp:1, Rx:01:08:50:40:
[  120.779371] mctp-i3c 1-7ec80010023: NET_RX_SUCCESS: 01:08:50:40:
[  120.826232] mctp-i3c 1-7ec80010023: NET_RX_SUCCESS: 01:08:50:30:

We can observe that the read order of i3c is: 80 -> 10 -> 20 -> 30 -> 40
But the final sequence of netif_rx() is: 80 -> 20 -> 10 -> 40 -> 30

> Just to clarify the intent here, and if I'm correct with the assumption
> above, it would be good to a comment on what this lock is serialising.
> If you're re-rolling with Jakub's Fixes request, can you add a comment
> too? Something like:
>
>        /* ensure that we netif_rx() in the same order as the i3c reads */
>        mutex_lock(&mi->lock);

Yes, thank you for the suggestion.


Best Regards,

Leo Yang