Re: [PATCH net] net: mctp: free reassembly head skb when key_add fails on SOM

[Jeremy Kerr]

Hi Joas & Guikin,

[merging two threads here; Guikin reported the same issue, around 12
hours earlier]

> When a start-of-message fragment creates a new reassembly key on the
> broadcast/bind path, mctp_frag_queue() stores the skb as
> key->reasm_head before the key is made observable with mctp_key_add().
> 
> If mctp_key_add() fails (a race with another SOM packet for the same
> source/dest/tag, or the receiving socket going away), the error path
> only drops the key reference via mctp_key_unref().  The final key
> destructor frees the key itself but not key->reasm_head, and since the
> key was never added to any list, neither the expiry worker nor socket
> teardown can reach it to release the queued fragment.  The skb is
> leaked.

OK, good catch.

> diff --git a/net/mctp/route.c b/net/mctp/route.c
> index e7c95eeac..3d79c2031 100644
> --- a/net/mctp/route.c
> +++ b/net/mctp/route.c
> @@ -552,8 +552,16 @@ static int mctp_dst_input(struct mctp_dst *dst, struct sk_buff *skb)
>  			 * can do is drop.
>  			 */
>  			rc = mctp_key_add(key, msk);
> -			if (!rc)
> +			if (!rc) {
>  				trace_mctp_key_acquire(key);
> +			} else {
> +				/* key_add failed: the key is not observable by
> +				 * any cleanup path, so free the fragment we
> +				 * just queued on it to avoid leaking the skb.
> +				 */
> +				kfree_skb(key->reasm_head);
> +				key->reasm_head = NULL;
> +			}

We could just defer setting skb to NULL until the mctp_key_add() has
succeeded; there's already the common error path that cleans up any
remaining skb.

Alternatively, we could change the semantics mctp_key_unref() to also
free any pending reassembly, but I'm okay with the current approach of
being explicit about the state handling.

Cheers,


Jeremy