[PATCH] mctp: route: hold key->lock in mctp_flow_prepare_output()

[PATCH] mctp: route: hold key->lock in mctp_flow_prepare_output()

[Chengfeng Ye]

mctp_flow_prepare_output() checks key->dev and may call
mctp_dev_set_key(), but it does not hold key->lock while doing so.

mctp_dev_set_key() and mctp_dev_release_key() are annotated with
__must_hold(&key->lock), so key->dev access is intended to be
serialized by key->lock. The mctp_sendmsg() transmit path reaches
mctp_flow_prepare_output() via mctp_local_output() -> mctp_dst_output()
without holding key->lock, so the check-and-set sequence is racy.

Example interleaving:

  CPU0                                  CPU1
  ----                                  ----
  mctp_flow_prepare_output(key, devA)
    if (!key->dev)  // sees NULL
                                        mctp_flow_prepare_output(
                                            key, devB)
                                          if (!key->dev)  // still NULL
                                          mctp_dev_set_key(devB, key)
                                            mctp_dev_hold(devB)
                                            key->dev = devB
    mctp_dev_set_key(devA, key)
      mctp_dev_hold(devA)
      key->dev = devA   // overwrites devB

Now both devA and devB references were acquired, but only the final
key->dev value is tracked for release. One reference can be lost,
causing a resource leak as mctp_dev_release_key() would only decrease
the reference on one dev.

Fix by taking key->lock around the key->dev check and
mctp_dev_set_key() call.

Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
---
 net/mctp/route.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/mctp/route.c b/net/mctp/route.c
index 0381377ab760..4a1ac55ad31e 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -359,6 +359,7 @@ static void mctp_flow_prepare_output(struct sk_buff *skb, struct mctp_dev *dev)
 {
 	struct mctp_sk_key *key;
 	struct mctp_flow *flow;
+	unsigned long flags;
 
 	flow = skb_ext_find(skb, SKB_EXT_MCTP);
 	if (!flow)
@@ -366,12 +367,17 @@ static void mctp_flow_prepare_output(struct sk_buff *skb, struct mctp_dev *dev)
 
 	key = flow->key;
 
+	spin_lock_irqsave(&key->lock, flags);
+
 	if (key->dev) {
 		WARN_ON(key->dev != dev);
-		return;
+		goto out_unlock;
 	}
 
 	mctp_dev_set_key(dev, key);
+
+out_unlock:
+	spin_unlock_irqrestore(&key->lock, flags);
 }
 #else
 static void mctp_skb_set_flow(struct sk_buff *skb, struct mctp_sk_key *key) {}
-- 
2.25.1

Re: [PATCH] mctp: route: hold key->lock in mctp_flow_prepare_output()

[Jakub Kicinski]

On Mon,  2 Mar 2026 17:40:56 +0000 Chengfeng Ye wrote:
> mctp_flow_prepare_output() checks key->dev and may call
> mctp_dev_set_key(), but it does not hold key->lock while doing so.

Please add an appropriate Fixes tag and repost.
-- 
pw-bot: cr

Re: [PATCH] mctp: route: hold key->lock in mctp_flow_prepare_output()

[Jeremy Kerr]

Hi Chengfeng,

For v2 (as requested by Jakub), can you correct my email address? I'm
jk@, not jeremy@.

> mctp_flow_prepare_output() checks key->dev and may call
> mctp_dev_set_key(), but it does not hold key->lock while doing so.
> 
> mctp_dev_set_key() and mctp_dev_release_key() are annotated with
> __must_hold(&key->lock), so key->dev access is intended to be
> serialized by key->lock. The mctp_sendmsg() transmit path reaches
> mctp_flow_prepare_output() via mctp_local_output() -> mctp_dst_output()
> without holding key->lock, so the check-and-set sequence is racy.

Good catch. I don't *think* we can hit this at present, as the key will
be unique over distinct sendmsg() calls, and the re-use of a key is all
sequential (ie., during fragmentation).

However, that may not always be the case, and we're currently violating
the __must_hold(key->lock) on mctp_dev_set_key() through this path. So
the addition of the lock here looks good.

One comment on the implementation:

> diff --git a/net/mctp/route.c b/net/mctp/route.c
> index 0381377ab760..4a1ac55ad31e 100644
> --- a/net/mctp/route.c
> +++ b/net/mctp/route.c
> @@ -359,6 +359,7 @@ static void mctp_flow_prepare_output(struct sk_buff *skb, struct mctp_dev *dev)
>  {
>         struct mctp_sk_key *key;
>         struct mctp_flow *flow;
> +       unsigned long flags;
>  
>         flow = skb_ext_find(skb, SKB_EXT_MCTP);
>         if (!flow)
> @@ -366,12 +367,17 @@ static void mctp_flow_prepare_output(struct sk_buff *skb, struct mctp_dev *dev)
>  
>         key = flow->key;
>  
> +       spin_lock_irqsave(&key->lock, flags);
> +
>         if (key->dev) {
>                 WARN_ON(key->dev != dev);
> -               return;
> +               goto out_unlock;
>         }

You could shift the mctp_dev_set_key() to an else block here, and avoid
the goto. Following that, it may be more readable if you invert the
logic (if (!key->dev) ...), but I will leave that as your call.

Cheers,


Jeremy

Re: [PATCH] mctp: route: hold key->lock in mctp_flow_prepare_output()

[Chengfeng Ye]

Hi Jeremy,

> For v2 (as requested by Jakub), can you correct my email address? I'm
> jk@, not jeremy@.

Sorry indeed for this mistake, I corrected the email address for the
new patches.

> You could shift the mctp_dev_set_key() to an else block here, and avoid
> the goto. Following that, it may be more readable if you invert the
> logic (if (!key->dev) ...), but I will leave that as your call.

No problem. I missed this message while making the v2 patch, so the
change is addressed in v3 (https://lkml.org/lkml/2026/3/6/201).

Best,
Chengfeng