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

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

[Leo Yang]

We found a timeout problem with the pldm command on our system.  The
reason is that the MCTP-I3C driver has a race condition when receiving
multiple-packet messages in multi-thread, resulting in a wrong packet
order problem.

We identified this problem by adding a debug message to the
mctp_i3c_read function.

According to the MCTP spec, a multiple-packet message must be composed
in sequence, and if there is a wrong sequence, the whole message will be
discarded and wait for the next SOM.
For example, SOM → Pkt Seq #2 → Pkt Seq #1 → Pkt Seq #3 → EOM.

Therefore, we try to solve this problem by adding a mutex to the
mctp_i3c_read function.  Before the modification, when a command
requesting a multiple-packet message response is sent consecutively, an
error usually occurs within 100 loops.  After the mutex, it can go
through 40000 loops without any error, and it seems to run well.

But I'm a little worried about the performance of mutex in high load
situation (as spec seems to allow different endpoints to respond at the
same time), do you think this is a feasible solution?

Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
---
 drivers/net/mctp/mctp-i3c.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/mctp/mctp-i3c.c b/drivers/net/mctp/mctp-i3c.c
index 9adad59b8676..0d625b351ebd 100644
--- a/drivers/net/mctp/mctp-i3c.c
+++ b/drivers/net/mctp/mctp-i3c.c
@@ -125,6 +125,7 @@ static int mctp_i3c_read(struct mctp_i3c_device *mi)
 
 	xfer.data.in = skb_put(skb, mi->mrl);
 
+	mutex_lock(&mi->lock);
 	rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
 	if (rc < 0)
 		goto err;
@@ -166,8 +167,10 @@ static int mctp_i3c_read(struct mctp_i3c_device *mi)
 		stats->rx_dropped++;
 	}
 
+	mutex_unlock(&mi->lock);
 	return 0;
 err:
+	mutex_unlock(&mi->lock);
 	kfree_skb(skb);
 	return rc;
 }
-- 
2.39.2

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

[Jakub Kicinski]

On Thu, 26 Dec 2024 10:53:19 +0800 Leo Yang wrote:
> We found a timeout problem with the pldm command on our system.  The
> reason is that the MCTP-I3C driver has a race condition when receiving
> multiple-packet messages in multi-thread, resulting in a wrong packet
> order problem.
> 
> We identified this problem by adding a debug message to the
> mctp_i3c_read function.
> 
> According to the MCTP spec, a multiple-packet message must be composed
> in sequence, and if there is a wrong sequence, the whole message will be
> discarded and wait for the next SOM.
> For example, SOM → Pkt Seq #2 → Pkt Seq #1 → Pkt Seq #3 → EOM.
> 
> Therefore, we try to solve this problem by adding a mutex to the
> mctp_i3c_read function.  Before the modification, when a command
> requesting a multiple-packet message response is sent consecutively, an
> error usually occurs within 100 loops.  After the mutex, it can go
> through 40000 loops without any error, and it seems to run well.
> 
> But I'm a little worried about the performance of mutex in high load
> situation (as spec seems to allow different endpoints to respond at the
> same time), do you think this is a feasible solution?

I don't see any obvious problem, Tx seems to hold this lock already.
Could you repost with a Fixes tag added?
-- 
pw-bot: cr

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

[Jeremy Kerr]

Hi Leo,

> We found a timeout problem with the pldm command on our system.  The
> reason is that the MCTP-I3C driver has a race condition when receiving
> multiple-packet messages in multi-thread, resulting in a wrong packet
> order problem.
> 
> We identified this problem by adding a debug message to the
> mctp_i3c_read function.

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?

> Therefore, we try to solve this problem by adding a mutex to the
> mctp_i3c_read function.

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);

Otherwise, all looks good. Thanks for the contribution!

Cheers,


Jeremy

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

[Leo Yang]

On Fri, Jan 3, 2025 at 10:28 AM Jeremy Kerr <jk@codeconstruct.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