[PATCH] mctp i2c: check packet length before marking flow active

[PATCH] mctp i2c: check packet length before marking flow active

[William A. Kennington III]

Currently, mctp_i2c_get_tx_flow_state() is called before the packet length
sanity check. This function marks a new flow as active in the MCTP core.

If the sanity check fails, mctp_i2c_xmit() returns early without calling
mctp_i2c_lock_nest(). This results in a mismatched locking state: the
flow is active, but the I2C bus lock was never acquired for it.

When the flow is later released, mctp_i2c_release_flow() will see the
active state and queue an unlock marker. The TX thread will then
decrement midev->i2c_lock_count from 0, causing it to underflow to -1.

This underflow permanently breaks the driver's locking logic, allowing
future transmissions to occur without holding the I2C bus lock, leading
to bus collisions and potential hardware hangs.

Move the mctp_i2c_get_tx_flow_state() call to after the length sanity
check to ensure we only transition the flow state if we are actually
going to proceed with the transmission and locking.

Fixes: f5b8abf9fc3d ("mctp i2c: MCTP I2C binding driver")
Signed-off-by: William A. Kennington III <william@wkennington.com>
---
 drivers/net/mctp/mctp-i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
index 15fe4d1163c1..ee2913758e54 100644
--- a/drivers/net/mctp/mctp-i2c.c
+++ b/drivers/net/mctp/mctp-i2c.c
@@ -496,8 +496,6 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
 	u8 *pecp;
 	int rc;
 
-	fs = mctp_i2c_get_tx_flow_state(midev, skb);
-
 	hdr = (void *)skb_mac_header(skb);
 	/* Sanity check that packet contents matches skb length,
 	 * and can't exceed MCTP_I2C_BUFSZ
@@ -509,6 +507,8 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
 		return;
 	}
 
+	fs = mctp_i2c_get_tx_flow_state(midev, skb);
+
 	if (skb_tailroom(skb) >= 1) {
 		/* Linear case with space, we can just append the PEC */
 		skb_put(skb, 1);
-- 
2.54.0.rc2.533.g4f5dca5207-goog

Re: [PATCH] mctp i2c: check packet length before marking flow active

[Jeremy Kerr]

Hi William,

> Move the mctp_i2c_get_tx_flow_state() call to after the length sanity
> check to ensure we only transition the flow state if we are actually
> going to proceed with the transmission and locking.

Good catch, thanks!

> Subject: [PATCH] mctp i2c: check packet length before marking flow active

You'll want to indicate that this is for the net tree, rather than
net-next, so something like:

   Subject: [PATCH net] net: mctp i2c: check packet length [...]

With that change:

Acked-by: Jeremy Kerr <jk@codeconstruct.com.au>

Out of curiosity though, how did you hit the hdr_byte_count mismatch in
the first place?

Cheers,


Jeremy

Re: [PATCH] mctp i2c: check packet length before marking flow active

[William A. Kennington III]

On 4/22/26 20:47, Jeremy Kerr wrote:

> Hi William,
>
>> Move the mctp_i2c_get_tx_flow_state() call to after the length sanity
>> check to ensure we only transition the flow state if we are actually
>> going to proceed with the transmission and locking.
> Good catch, thanks!
>
>> Subject: [PATCH] mctp i2c: check packet length before marking flow active
> You'll want to indicate that this is for the net tree, rather than
> net-next, so something like:
>
>     Subject: [PATCH net] net: mctp i2c: check packet length [...]

Thanks, forgot the convention...

>
> With that change:
>
> Acked-by: Jeremy Kerr <jk@codeconstruct.com.au>
>
> Out of curiosity though, how did you hit the hdr_byte_count mismatch in
> the first place?

Our current theory is that we have known buggy firmware on our NVME MCTP 
devices and we are seeing some kind of corruption on the bus that we are 
going to fix in on the firmware side. We started also seeing kernel 
crashes along with the bad firmware symptoms, walked through ~110 kdumps 
and found i2c locks that were held by 2 owners (eeprom reading and the 
MCTP TX queue). This ended up causing deadlocks in the i2c stack that 
result in panics. We noticed in many of these cases that we had 10K+ 
BERs and 800+ NACKs in the i2c driver stats struct at the time of crash.

>
> Cheers,
>
>
> Jeremy

[PATCH net v2] net: mctp i2c: check length before marking flow active

[William A. Kennington III]

Currently, mctp_i2c_get_tx_flow_state() is called before the packet length
sanity check. This function marks a new flow as active in the MCTP core.

If the sanity check fails, mctp_i2c_xmit() returns early without calling
mctp_i2c_lock_nest(). This results in a mismatched locking state: the
flow is active, but the I2C bus lock was never acquired for it.

When the flow is later released, mctp_i2c_release_flow() will see the
active state and queue an unlock marker. The TX thread will then
decrement midev->i2c_lock_count from 0, causing it to underflow to -1.

This underflow permanently breaks the driver's locking logic, allowing
future transmissions to occur without holding the I2C bus lock, leading
to bus collisions and potential hardware hangs.

Move the mctp_i2c_get_tx_flow_state() call to after the length sanity
check to ensure we only transition the flow state if we are actually
going to proceed with the transmission and locking.

Fixes: f5b8abf9fc3d ("mctp i2c: MCTP I2C binding driver")
Signed-off-by: William A. Kennington III <william@wkennington.com>
---
 drivers/net/mctp/mctp-i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
index 15fe4d1163c1..ee2913758e54 100644
--- a/drivers/net/mctp/mctp-i2c.c
+++ b/drivers/net/mctp/mctp-i2c.c
@@ -496,8 +496,6 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
 	u8 *pecp;
 	int rc;
 
-	fs = mctp_i2c_get_tx_flow_state(midev, skb);
-
 	hdr = (void *)skb_mac_header(skb);
 	/* Sanity check that packet contents matches skb length,
 	 * and can't exceed MCTP_I2C_BUFSZ
@@ -509,6 +507,8 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
 		return;
 	}
 
+	fs = mctp_i2c_get_tx_flow_state(midev, skb);
+
 	if (skb_tailroom(skb) >= 1) {
 		/* Linear case with space, we can just append the PEC */
 		skb_put(skb, 1);
-- 
2.54.0.545.g6539524ca2-goog

Re: [PATCH] mctp i2c: check packet length before marking flow active

[Jeremy Kerr]

Hi William,

> > Out of curiosity though, how did you hit the hdr_byte_count mismatch in
> > the first place?
> 
> Our current theory is that we have known buggy firmware on our NVME MCTP 
> devices and we are seeing some kind of corruption on the bus that we are 
> going to fix in on the firmware side.

OK, sounds good for the overall fix, but I don't think that would be
causing the path that you're addressing here. The fix is definitely
valid, but can't be hit through any RX data corruption (we're in the
TX path).

The header byte count is populated during header construction, so a
mismatch here would indicate modification of the skb between that point
at the actual xmit. Do you see the "Bad TX len" warning in these cases?

> We started also seeing kernel 
> crashes along with the bad firmware symptoms, walked through ~110 kdumps 
> and found i2c locks that were held by 2 owners (eeprom reading and the 
> MCTP TX queue).

Just to clarify my understanding of the state: "being held by two
owners" would indicate a violation of the lock itself. Or is it that
there are two threads blocked waiting to acquire the mutex?

For NVMe-MI, you're likely using manual tag allocation, where the tag
allocation (and hence flow state) is entirely controlled by userspace.
It may be that the NVMe protocol-level errors are causing that tags to
be held for long durations, perhaps?

Cheers,


Jeremy

Re: [PATCH net v2] net: mctp i2c: check length before marking flow active

[Paolo Abeni]

On 4/23/26 9:46 AM, William A. Kennington III wrote:
> Currently, mctp_i2c_get_tx_flow_state() is called before the packet length
> sanity check. This function marks a new flow as active in the MCTP core.
> 
> If the sanity check fails, mctp_i2c_xmit() returns early without calling
> mctp_i2c_lock_nest(). This results in a mismatched locking state: the
> flow is active, but the I2C bus lock was never acquired for it.
> 
> When the flow is later released, mctp_i2c_release_flow() will see the
> active state and queue an unlock marker. The TX thread will then
> decrement midev->i2c_lock_count from 0, causing it to underflow to -1.
> 
> This underflow permanently breaks the driver's locking logic, allowing
> future transmissions to occur without holding the I2C bus lock, leading
> to bus collisions and potential hardware hangs.
> 
> Move the mctp_i2c_get_tx_flow_state() call to after the length sanity
> check to ensure we only transition the flow state if we are actually
> going to proceed with the transmission and locking.
> 
> Fixes: f5b8abf9fc3d ("mctp i2c: MCTP I2C binding driver")
> Signed-off-by: William A. Kennington III <william@wkennington.com>

Note that you should have included Jeremy's ack, and you should have
avoided reposting before the 24h grace period. In this specific case,
you could have avoided a repost entirely

/P

Re: [PATCH net v2] net: mctp i2c: check length before marking flow active

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 23 Apr 2026 00:46:52 -0700 you wrote:
> Currently, mctp_i2c_get_tx_flow_state() is called before the packet length
> sanity check. This function marks a new flow as active in the MCTP core.
> 
> If the sanity check fails, mctp_i2c_xmit() returns early without calling
> mctp_i2c_lock_nest(). This results in a mismatched locking state: the
> flow is active, but the I2C bus lock was never acquired for it.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: mctp i2c: check length before marking flow active
    https://git.kernel.org/netdev/net/c/4ca07b9239bd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v2] net: mctp i2c: check length before marking flow active

[Jeremy Kerr]

Hi Pablo,

> Here is the summary with links:
>   - [net,v2] net: mctp i2c: check length before marking flow active
>     https://git.kernel.org/netdev/net/c/4ca07b9239bd
> 
> You are awesome, thank you!

4ca07b9239bd seems to have acquired an unrelated change:

   $ git show 4ca07b9239bd | diffstat
    drivers/net/mctp/mctp-i2c.c |    4 ++--
    net/sched/cls_flower.c      |    4 +++-
    2 files changed, 5 insertions(+), 3 deletions(-)

While the commit structure is probably not intentional, are the Flower
changes acceptable for the net tree?

Cheers,


Jeremy

Re: [PATCH net v2] net: mctp i2c: check length before marking flow active

[Paolo Abeni]

On 4/29/26 3:23 AM, Jeremy Kerr wrote:
> Hi Pablo,
> 
>> Here is the summary with links:
>>   - [net,v2] net: mctp i2c: check length before marking flow active
>>     https://git.kernel.org/netdev/net/c/4ca07b9239bd
>>
>> You are awesome, thank you!
> 
> 4ca07b9239bd seems to have acquired an unrelated change:
> 
>    $ git show 4ca07b9239bd | diffstat
>     drivers/net/mctp/mctp-i2c.c |    4 ++--
>     net/sched/cls_flower.c      |    4 +++-
>     2 files changed, 5 insertions(+), 3 deletions(-)
> 
> While the commit structure is probably not intentional, are the Flower
> changes acceptable for the net tree?

Thanks for checking!

That is a serious error on my side, I had an unclean working tree when I
applied the patch that ended-up including random staff. Absolutely not
intended for the net tree. I'll send a fixup patch.

Thank you again for the head-up!

Paolo

Re: [PATCH] mctp i2c: check packet length before marking flow active

[William A. Kennington III]

On 4/23/26 21:16, Jeremy Kerr wrote:

> Hi William,
>
>>> Out of curiosity though, how did you hit the hdr_byte_count mismatch in
>>> the first place?
>> Our current theory is that we have known buggy firmware on our NVME MCTP
>> devices and we are seeing some kind of corruption on the bus that we are
>> going to fix in on the firmware side.
> OK, sounds good for the overall fix, but I don't think that would be
> causing the path that you're addressing here. The fix is definitely
> valid, but can't be hit through any RX data corruption (we're in the
> TX path).
Yeah I think you might be right, the hard part is reproducing this is so 
infrequent for us that it takes a long time to iterate on testing these 
changes.
>
> The header byte count is populated during header construction, so a
> mismatch here would indicate modification of the skb between that point
> at the actual xmit. Do you see the "Bad TX len" warning in these cases?

I double checked and so far I can’t find evidence of it. Probably we 
still want to keep this change, but it’s not the root of our problems.

>> We started also seeing kernel
>> crashes along with the bad firmware symptoms, walked through ~110 kdumps
>> and found i2c locks that were held by 2 owners (eeprom reading and the
>> MCTP TX queue).
> Just to clarify my understanding of the state: "being held by two
> owners" would indicate a violation of the lock itself. Or is it that
> there are two threads blocked waiting to acquire the mutex?
I think it’s actually this, 2 threads are waiting on acquiring the lock. 
There was a theory that it was a lock underflow that allowed 2 threads 
to acquire the lock that lead to this patch.
> For NVMe-MI, you're likely using manual tag allocation, where the tag
> allocation (and hence flow state) is entirely controlled by userspace.
> It may be that the NVMe protocol-level errors are causing that tags to
> be held for long durations, perhaps?

Yeah, this is very plausible given the device(s) stop responding 
correctly. I imagine we are getting stuck with manual allocations and 
not releasing locks. Can we reset the state machine back to NEW instead 
of holding the lock?

>
> Cheers,
>
>
> Jeremy