[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