[PATCH net v2] net/sched: serialize qdisc_rtab_list against concurrent get/put

[PATCH net v2] net/sched: serialize qdisc_rtab_list against concurrent get/put

[Aldo Ariel Panzardo]

qdisc_get_rtab() and qdisc_put_rtab() mutate the process-global singly
linked list qdisc_rtab_list and a plain non-atomic 'int refcnt' with no
lock. This was only safe because every caller historically held the RTNL
mutex, which serialized all rate-table lookups, inserts and frees.

That invariant no longer holds. cls_flower sets
TCF_PROTO_OPS_DOIT_UNLOCKED, so tc_new_tfilter() keeps rtnl_held == false
for it and sets TCA_ACT_FLAGS_NO_RTNL. That flag propagates through
tcf_exts_validate_ex() -> tcf_action_init() -> tcf_action_init_1() ->
tcf_police_init(), which calls qdisc_get_rtab()/qdisc_put_rtab() with the
RTNL mutex NOT held. Two RTM_NEWTFILTER requests on different CPUs, each
adding a flower filter with a police action carrying the same rate, then
race on qdisc_rtab_list and on the non-atomic refcnt, leading to a
use-after-free / double-free of the kmalloc-2k struct qdisc_rate_table.
qdisc_rtab_list is a single global (not per-netns), so the corrupted
object is shared system-wide.

  BUG: KASAN: slab-use-after-free in qdisc_put_rtab+0x12f/0x160
   qdisc_put_rtab+0x12f/0x160
   tcf_police_init+0xda9/0x1590
   tcf_action_init_1+0x460/0x6b0
   tcf_action_init+0x439/0xa40
   tcf_exts_validate_ex+0x42d/0x550
   fl_change+0xddd/0x7da0
   tc_new_tfilter+0xaa7/0x2420
   rtnetlink_rcv_msg+0x95e/0xe90
  which belongs to the cache kmalloc-2k of size 2048

Protect qdisc_rtab_list and the refcount with a dedicated spinlock. The
(sleeping, GFP_KERNEL) allocation in qdisc_get_rtab() is performed before
taking the lock; if a concurrent inserter added an identical table in the
meantime the freshly allocated one is freed under the lock, so no
duplicate is leaked. qdisc_put_rtab() now decrements the refcount and
unlinks under the same lock.

Fixes: 470502de5bdb ("net: sched: unlock rules update API")
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
Cc: stable@vger.kernel.org
---

v2:
 - Rework qdisc_get_rtab() to allocate before taking the lock and free the
   surplus table under the lock instead of dropping the lock to allocate
   and re-scanning (Eric Dumazet).
 - Add Fixes: tag and Cc: stable.
 - Build-tested (CONFIG_NET_SCHED=y).

v1 was sent privately to security@kernel.org on 2026-07-11; moving to the
public list now that a fix is ready, as requested.
 net/sched/sch_api.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 8a3236456db4..668bcd60d183 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -415,12 +415,13 @@ static __u8 __detect_linklayer(struct tc_ratespec *r, __u32 *rtab)
 }
 
 static struct qdisc_rate_table *qdisc_rtab_list;
+static DEFINE_SPINLOCK(qdisc_rtab_lock);
 
 struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
 					struct nlattr *tab,
 					struct netlink_ext_ack *extack)
 {
-	struct qdisc_rate_table *rtab;
+	struct qdisc_rate_table *rtab, *new_rtab;
 
 	if (tab == NULL || r->rate == 0 ||
 	    r->cell_log == 0 || r->cell_log >= 32 ||
@@ -429,15 +430,20 @@ struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
 		return NULL;
 	}
 
+	new_rtab = kmalloc_obj(*new_rtab);
+
+	spin_lock(&qdisc_rtab_lock);
 	for (rtab = qdisc_rtab_list; rtab; rtab = rtab->next) {
 		if (!memcmp(&rtab->rate, r, sizeof(struct tc_ratespec)) &&
 		    !memcmp(&rtab->data, nla_data(tab), TC_RTAB_SIZE)) {
 			rtab->refcnt++;
+			spin_unlock(&qdisc_rtab_lock);
+			kfree(new_rtab);
 			return rtab;
 		}
 	}
 
-	rtab = kmalloc_obj(*rtab);
+	rtab = new_rtab;
 	if (rtab) {
 		rtab->rate = *r;
 		rtab->refcnt = 1;
@@ -449,6 +455,7 @@ struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
 	} else {
 		NL_SET_ERR_MSG(extack, "Failed to allocate new qdisc rate table");
 	}
+	spin_unlock(&qdisc_rtab_lock);
 	return rtab;
 }
 EXPORT_SYMBOL(qdisc_get_rtab);
@@ -457,18 +464,25 @@ void qdisc_put_rtab(struct qdisc_rate_table *tab)
 {
 	struct qdisc_rate_table *rtab, **rtabp;
 
-	if (!tab || --tab->refcnt)
+	if (!tab)
 		return;
 
+	spin_lock(&qdisc_rtab_lock);
+	if (--tab->refcnt) {
+		spin_unlock(&qdisc_rtab_lock);
+		return;
+	}
+
 	for (rtabp = &qdisc_rtab_list;
 	     (rtab = *rtabp) != NULL;
 	     rtabp = &rtab->next) {
 		if (rtab == tab) {
 			*rtabp = rtab->next;
-			kfree(rtab);
-			return;
+			break;
 		}
 	}
+	spin_unlock(&qdisc_rtab_lock);
+	kfree(tab);
 }
 EXPORT_SYMBOL(qdisc_put_rtab);
 
-- 
2.43.0

Re: [PATCH net v2] net/sched: serialize qdisc_rtab_list against concurrent get/put

[Jamal Hadi Salim]

On Wed, Jul 15, 2026 at 7:41 AM Aldo Ariel Panzardo <qwe.aldo@gmail.com> wrote:
>
> qdisc_get_rtab() and qdisc_put_rtab() mutate the process-global singly
> linked list qdisc_rtab_list and a plain non-atomic 'int refcnt' with no
> lock. This was only safe because every caller historically held the RTNL
> mutex, which serialized all rate-table lookups, inserts and frees.
>
> That invariant no longer holds. cls_flower sets
> TCF_PROTO_OPS_DOIT_UNLOCKED, so tc_new_tfilter() keeps rtnl_held == false
> for it and sets TCA_ACT_FLAGS_NO_RTNL. That flag propagates through
> tcf_exts_validate_ex() -> tcf_action_init() -> tcf_action_init_1() ->
> tcf_police_init(), which calls qdisc_get_rtab()/qdisc_put_rtab() with the
> RTNL mutex NOT held. Two RTM_NEWTFILTER requests on different CPUs, each
> adding a flower filter with a police action carrying the same rate, then
> race on qdisc_rtab_list and on the non-atomic refcnt, leading to a
> use-after-free / double-free of the kmalloc-2k struct qdisc_rate_table.
> qdisc_rtab_list is a single global (not per-netns), so the corrupted
> object is shared system-wide.
>
>   BUG: KASAN: slab-use-after-free in qdisc_put_rtab+0x12f/0x160
>    qdisc_put_rtab+0x12f/0x160
>    tcf_police_init+0xda9/0x1590
>    tcf_action_init_1+0x460/0x6b0
>    tcf_action_init+0x439/0xa40
>    tcf_exts_validate_ex+0x42d/0x550
>    fl_change+0xddd/0x7da0
>    tc_new_tfilter+0xaa7/0x2420
>    rtnetlink_rcv_msg+0x95e/0xe90
>   which belongs to the cache kmalloc-2k of size 2048
>
> Protect qdisc_rtab_list and the refcount with a dedicated spinlock. The
> (sleeping, GFP_KERNEL) allocation in qdisc_get_rtab() is performed before
> taking the lock; if a concurrent inserter added an identical table in the
> meantime the freshly allocated one is freed under the lock, so no
> duplicate is leaked. qdisc_put_rtab() now decrements the refcount and
> unlinks under the same lock.
>
> Fixes: 470502de5bdb ("net: sched: unlock rules update API")
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

The fix looks reasonable to me. I also tested the poc
 So:
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal


> Cc: stable@vger.kernel.org
> ---
>
> v2:
>  - Rework qdisc_get_rtab() to allocate before taking the lock and free the
>    surplus table under the lock instead of dropping the lock to allocate
>    and re-scanning (Eric Dumazet).
>  - Add Fixes: tag and Cc: stable.
>  - Build-tested (CONFIG_NET_SCHED=y).
>
> v1 was sent privately to security@kernel.org on 2026-07-11; moving to the
> public list now that a fix is ready, as requested.
>  net/sched/sch_api.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 8a3236456db4..668bcd60d183 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -415,12 +415,13 @@ static __u8 __detect_linklayer(struct tc_ratespec *r, __u32 *rtab)
>  }
>
>  static struct qdisc_rate_table *qdisc_rtab_list;
> +static DEFINE_SPINLOCK(qdisc_rtab_lock);
>
>  struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
>                                         struct nlattr *tab,
>                                         struct netlink_ext_ack *extack)
>  {
> -       struct qdisc_rate_table *rtab;
> +       struct qdisc_rate_table *rtab, *new_rtab;
>
>         if (tab == NULL || r->rate == 0 ||
>             r->cell_log == 0 || r->cell_log >= 32 ||
> @@ -429,15 +430,20 @@ struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
>                 return NULL;
>         }
>
> +       new_rtab = kmalloc_obj(*new_rtab);
> +
> +       spin_lock(&qdisc_rtab_lock);
>         for (rtab = qdisc_rtab_list; rtab; rtab = rtab->next) {
>                 if (!memcmp(&rtab->rate, r, sizeof(struct tc_ratespec)) &&
>                     !memcmp(&rtab->data, nla_data(tab), TC_RTAB_SIZE)) {
>                         rtab->refcnt++;
> +                       spin_unlock(&qdisc_rtab_lock);
> +                       kfree(new_rtab);
>                         return rtab;
>                 }
>         }
>
> -       rtab = kmalloc_obj(*rtab);
> +       rtab = new_rtab;
>         if (rtab) {
>                 rtab->rate = *r;
>                 rtab->refcnt = 1;
> @@ -449,6 +455,7 @@ struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
>         } else {
>                 NL_SET_ERR_MSG(extack, "Failed to allocate new qdisc rate table");
>         }
> +       spin_unlock(&qdisc_rtab_lock);
>         return rtab;
>  }
>  EXPORT_SYMBOL(qdisc_get_rtab);
> @@ -457,18 +464,25 @@ void qdisc_put_rtab(struct qdisc_rate_table *tab)
>  {
>         struct qdisc_rate_table *rtab, **rtabp;
>
> -       if (!tab || --tab->refcnt)
> +       if (!tab)
>                 return;
>
> +       spin_lock(&qdisc_rtab_lock);
> +       if (--tab->refcnt) {
> +               spin_unlock(&qdisc_rtab_lock);
> +               return;
> +       }
> +
>         for (rtabp = &qdisc_rtab_list;
>              (rtab = *rtabp) != NULL;
>              rtabp = &rtab->next) {
>                 if (rtab == tab) {
>                         *rtabp = rtab->next;
> -                       kfree(rtab);
> -                       return;
> +                       break;
>                 }
>         }
> +       spin_unlock(&qdisc_rtab_lock);
> +       kfree(tab);
>  }
>  EXPORT_SYMBOL(qdisc_put_rtab);
>
> --
> 2.43.0
>

Re: [PATCH net v2] net/sched: serialize qdisc_rtab_list against concurrent get/put

[Eric Dumazet]

On Wed, Jul 22, 2026 at 1:47 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Wed, Jul 15, 2026 at 7:41 AM Aldo Ariel Panzardo <qwe.aldo@gmail.com> wrote:
> >
> > qdisc_get_rtab() and qdisc_put_rtab() mutate the process-global singly
> > linked list qdisc_rtab_list and a plain non-atomic 'int refcnt' with no
> > lock. This was only safe because every caller historically held the RTNL
> > mutex, which serialized all rate-table lookups, inserts and frees.
> >
> > That invariant no longer holds. cls_flower sets
> > TCF_PROTO_OPS_DOIT_UNLOCKED, so tc_new_tfilter() keeps rtnl_held == false
> > for it and sets TCA_ACT_FLAGS_NO_RTNL. That flag propagates through
> > tcf_exts_validate_ex() -> tcf_action_init() -> tcf_action_init_1() ->
> > tcf_police_init(), which calls qdisc_get_rtab()/qdisc_put_rtab() with the
> > RTNL mutex NOT held. Two RTM_NEWTFILTER requests on different CPUs, each
> > adding a flower filter with a police action carrying the same rate, then
> > race on qdisc_rtab_list and on the non-atomic refcnt, leading to a
> > use-after-free / double-free of the kmalloc-2k struct qdisc_rate_table.
> > qdisc_rtab_list is a single global (not per-netns), so the corrupted
> > object is shared system-wide.
> >
> >   BUG: KASAN: slab-use-after-free in qdisc_put_rtab+0x12f/0x160
> >    qdisc_put_rtab+0x12f/0x160
> >    tcf_police_init+0xda9/0x1590
> >    tcf_action_init_1+0x460/0x6b0
> >    tcf_action_init+0x439/0xa40
> >    tcf_exts_validate_ex+0x42d/0x550
> >    fl_change+0xddd/0x7da0
> >    tc_new_tfilter+0xaa7/0x2420
> >    rtnetlink_rcv_msg+0x95e/0xe90
> >   which belongs to the cache kmalloc-2k of size 2048
> >
> > Protect qdisc_rtab_list and the refcount with a dedicated spinlock. The
> > (sleeping, GFP_KERNEL) allocation in qdisc_get_rtab() is performed before
> > taking the lock; if a concurrent inserter added an identical table in the
> > meantime the freshly allocated one is freed under the lock, so no
> > duplicate is leaked. qdisc_put_rtab() now decrements the refcount and
> > unlinks under the same lock.
> >
> > Fixes: 470502de5bdb ("net: sched: unlock rules update API")
> > Suggested-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
>
> The fix looks reasonable to me. I also tested the poc
>  So:
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Same here, thanks!

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net v2] net/sched: serialize qdisc_rtab_list against concurrent get/put

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 15 Jul 2026 08:41:14 -0300 you wrote:
> qdisc_get_rtab() and qdisc_put_rtab() mutate the process-global singly
> linked list qdisc_rtab_list and a plain non-atomic 'int refcnt' with no
> lock. This was only safe because every caller historically held the RTNL
> mutex, which serialized all rate-table lookups, inserts and frees.
> 
> That invariant no longer holds. cls_flower sets
> TCF_PROTO_OPS_DOIT_UNLOCKED, so tc_new_tfilter() keeps rtnl_held == false
> for it and sets TCA_ACT_FLAGS_NO_RTNL. That flag propagates through
> tcf_exts_validate_ex() -> tcf_action_init() -> tcf_action_init_1() ->
> tcf_police_init(), which calls qdisc_get_rtab()/qdisc_put_rtab() with the
> RTNL mutex NOT held. Two RTM_NEWTFILTER requests on different CPUs, each
> adding a flower filter with a police action carrying the same rate, then
> race on qdisc_rtab_list and on the non-atomic refcnt, leading to a
> use-after-free / double-free of the kmalloc-2k struct qdisc_rate_table.
> qdisc_rtab_list is a single global (not per-netns), so the corrupted
> object is shared system-wide.
> 
> [...]

Here is the summary with links:
  - [net,v2] net/sched: serialize qdisc_rtab_list against concurrent get/put
    https://git.kernel.org/netdev/net/c/f43ee0c0730d

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