Re: [PATCH net v2 1/2] net/sched: act_ct: fix sk_buff leak when the header checks reject a packet

On Thu, Aug 6, 2026 at 6:12 AM Hyunjung Ko <[email protected]> wrote:
>
> tcf_ct_handle_fragments() runs its header sanity checks before handing
> anything to the defragmentation engine:
>
>         if (family == NFPROTO_IPV4)
>                 err = tcf_ct_ipv4_is_fragment(skb, &frag);
>         else
>                 err = tcf_ct_ipv6_is_fragment(skb, &frag);
>         if (err || !frag)
>                 return err;
>
> tcf_ct_ipv4_is_fragment() returns -EINVAL or -ENOMEM;
> tcf_ct_ipv6_is_fragment() adds -EPROTO when ipv6_find_hdr() fails. None of
> them frees or queues the skb, so on that path the caller still owns it.
>
> tcf_ct_act() however funnels every non-zero return into the
> ownership-transfer exit:
>
>         err = tcf_ct_handle_fragments(net, skb, family, p->zone, &defrag);
>         if (err)
>                 goto out_frag;
>         ...
> out_frag:
>         if (err != -EINPROGRESS)
>                 tcf_action_inc_drop_qstats(&c->common);
>         return TC_ACT_CONSUMED;
>
> TC_ACT_CONSUMED means the action took ownership of the skb, so no caller
> frees it - sch_handle_ingress(), sch_handle_egress() and
> tcf_qevent_handle() all deliberately skip the free for that verdict. The
> skb is therefore orphaned: one sk_buff plus its data buffer is leaked per
> malformed packet, unbounded. Note the drop counter is already incremented
> for these errors, so the statistics claim a drop that never happens.
>
> Three different ownership states reach out_frag: today - the skb may be
> queued by the defrag engine (-EINPROGRESS), already freed by
> nf_ct_handle_fragments(), or still owned by us. Tell the caller which of
> those it is, and free the packet ourselves in the last case, which
> restores the TC_ACT_SHOT behaviour that predated the Fixes: commit.
>
> Reproduced on v7.2-rc6 with a 54-byte frame carrying a 40-byte IPv6
> header with nexthdr = 0 (hop-by-hop) and nothing after it, on a
> clsact ingress chain with "action ct". kmemleak reports one leaked
> 232-byte skbuff_head_cache object plus its 704-byte data buffer per
> packet; with this patch it reports none.
>
> Fixes: 3f14b377d01d ("net/sched: act_ct: fix skb leak and crash on ooo frags")
> Cc: [email protected] # v6.8+
> Assisted-by: Anthropic-Claude-Code:Claude-Opus-5
> Signed-off-by: Hyunjung Ko <[email protected]>

I actually have tested and reviewed this. So:

Acked-by: Jamal Hadi Salim <[email protected]>

cheers,
jamal
> ---
>  net/sched/act_ct.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
>
> v2:
>  - add a tdc selftest (patch 2/2), as requested by Jamal
>  - add Assisted-by: tag
>  - no functional change to the fix itself
> v1: https://lore.kernel.org/netdev/[email protected]/
>
> Reproducer needs CONFIG_NET_ACT_CT, plus CONFIG_DEBUG_KMEMLEAK and
> kmemleak=on to observe it:
>
>   ip link add veth0 type veth peer name veth1
>   ip link set veth0 up; ip link set veth1 up
>   tc qdisc add dev veth0 clsact
>   tc filter add dev veth0 ingress matchall action ct
>
> then inject at veth1 a 54-byte frame: ethertype 0x86DD, a 40-byte IPv6
> header with nexthdr = 0 (hop-by-hop) and nothing after it, so
> ipv6_find_hdr() fails with -EBADMSG and tcf_ct_ipv6_is_fragment()
> returns -EPROTO.
>
> Before, one sk_buff plus its data buffer per packet:
>
>   kmemleak: 50 new suspected memory leaks
>   unreferenced object 0xffff888103ed13c0 (size 232):
>     kmem_cache_alloc_node_noprof+0x2f1/0x3e0
>     __alloc_skb+0xe5/0x860
>     alloc_skb_with_frags+0x82/0x750
>     sock_alloc_send_pskb+0x658/0x7e0
>     packet_sendmsg+0x1833/0x4860
>     __x64_sys_sendto+0xe0/0x1c0
>     do_syscall_64+0x102/0x5a0
>
> After: kmemleak reports no unreferenced objects.
>
> Note /proc/slabinfo is not a usable check here on a KASAN build -
> skbuff_head_cache active_objs still grows because the quarantine holds
> the freed objects. kmemleak is the reliable signal.
>
> Patch 2/2 turns the same case into a tdc test, using the clsact drop
> counter as the discriminator: before the fix act_ct returns
> TC_ACT_CONSUMED, so tc_run() never reaches its TC_ACT_SHOT arm and the
> counter stays at zero while the skbs leak.
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index be535a261fa0..e250969c84ac 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -840,8 +840,15 @@ static int tcf_ct_ipv6_is_fragment(struct sk_buff *skb, bool *frag)
>         return 0;
>  }
>
> +/* On error, tells the caller whether it still owns @skb and must free it
> + * itself.  @skb is ours only when the header checks below reject the packet
> + * before it is handed to the defragmentation engine; once nf_ct_handle_
> + * fragments() has been called the skb is either queued (-EINPROGRESS) or has
> + * already been freed by it.
> + */
>  static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
> -                                  u8 family, u16 zone, bool *defrag)
> +                                  u8 family, u16 zone, bool *defrag,
> +                                  bool *skb_is_ours)
>  {
>         enum ip_conntrack_info ctinfo;
>         struct tc_skb_cb cb;
> @@ -859,8 +866,12 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
>                 err = tcf_ct_ipv4_is_fragment(skb, &frag);
>         else
>                 err = tcf_ct_ipv6_is_fragment(skb, &frag);
> -       if (err || !frag)
> +       if (err) {
> +               *skb_is_ours = true;
>                 return err;
> +       }
> +       if (!frag)
> +               return 0;
>
>         cb = *tc_skb_cb(skb);
>         err = nf_ct_handle_fragments(net, skb, zone, family, &proto, &cb.mru);
> @@ -977,6 +988,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>         int nh_ofs, err, retval;
>         struct tcf_ct_params *p;
>         bool add_helper = false;
> +       bool skb_is_ours = false;
>         bool skip_add = false;
>         bool defrag = false;
>         struct nf_conn *ct;
> @@ -1012,9 +1024,18 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>          */
>         nh_ofs = skb_network_offset(skb);
>         skb_pull_rcsum(skb, nh_ofs);
> -       err = tcf_ct_handle_fragments(net, skb, family, p->zone, &defrag);
> -       if (err)
> +       err = tcf_ct_handle_fragments(net, skb, family, p->zone, &defrag,
> +                                     &skb_is_ours);
> +       if (err) {
> +               /* The skb is still ours only when the header checks rejected
> +                * it; returning TC_ACT_CONSUMED for such a packet would leak
> +                * it, since no caller frees an skb it was told it no longer
> +                * owns.
> +                */
> +               if (skb_is_ours)
> +                       goto drop;
>                 goto out_frag;
> +       }
>
>         err = nf_ct_skb_network_trim(skb, family);
>         if (err)
> --
> 2.43.0