[PATCH net] sch_netem: fix issues in netem_change() vs get_dist_table()

[PATCH net] sch_netem: fix issues in netem_change() vs get_dist_table()

[Eric Dumazet]

In blamed commit, I missed that get_dist_table() was allocating
memory using GFP_KERNEL, and acquiring qdisc lock to perform
the swap of newly allocated table with current one.

In this patch, get_dist_table() is allocating memory and
copy user data before we acquire the qdisc lock.

Then we perform swap operations while being protected by the lock.

Note that after this patch netem_change() no longer can do partial changes.
If an error is returned, qdisc conf is left unchanged.

Fixes: 2174a08db80d ("sch_netem: acquire qdisc lock in netem_change()")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Simon Horman <simon.horman@corigine.com>
---
 net/sched/sch_netem.c | 59 ++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index e79be1b3e74da3c154f7ee23e16cc9e8da8f7106..b93ec2a3454ebceea559299f90533cbf1c0f7c26 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -773,12 +773,10 @@ static void dist_free(struct disttable *d)
  * signed 16 bit values.
  */
 
-static int get_dist_table(struct Qdisc *sch, struct disttable **tbl,
-			  const struct nlattr *attr)
+static int get_dist_table(struct disttable **tbl, const struct nlattr *attr)
 {
 	size_t n = nla_len(attr)/sizeof(__s16);
 	const __s16 *data = nla_data(attr);
-	spinlock_t *root_lock;
 	struct disttable *d;
 	int i;
 
@@ -793,13 +791,7 @@ static int get_dist_table(struct Qdisc *sch, struct disttable **tbl,
 	for (i = 0; i < n; i++)
 		d->table[i] = data[i];
 
-	root_lock = qdisc_root_sleeping_lock(sch);
-
-	spin_lock_bh(root_lock);
-	swap(*tbl, d);
-	spin_unlock_bh(root_lock);
-
-	dist_free(d);
+	*tbl = d;
 	return 0;
 }
 
@@ -956,6 +948,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 {
 	struct netem_sched_data *q = qdisc_priv(sch);
 	struct nlattr *tb[TCA_NETEM_MAX + 1];
+	struct disttable *delay_dist = NULL;
+	struct disttable *slot_dist = NULL;
 	struct tc_netem_qopt *qopt;
 	struct clgstate old_clg;
 	int old_loss_model = CLG_RANDOM;
@@ -966,6 +960,18 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 	if (ret < 0)
 		return ret;
 
+	if (tb[TCA_NETEM_DELAY_DIST]) {
+		ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST]);
+		if (ret)
+			goto table_free;
+	}
+
+	if (tb[TCA_NETEM_SLOT_DIST]) {
+		ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST]);
+		if (ret)
+			goto table_free;
+	}
+
 	sch_tree_lock(sch);
 	/* backup q->clg and q->loss_model */
 	old_clg = q->clg;
@@ -975,26 +981,17 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 		ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
 		if (ret) {
 			q->loss_model = old_loss_model;
+			q->clg = old_clg;
 			goto unlock;
 		}
 	} else {
 		q->loss_model = CLG_RANDOM;
 	}
 
-	if (tb[TCA_NETEM_DELAY_DIST]) {
-		ret = get_dist_table(sch, &q->delay_dist,
-				     tb[TCA_NETEM_DELAY_DIST]);
-		if (ret)
-			goto get_table_failure;
-	}
-
-	if (tb[TCA_NETEM_SLOT_DIST]) {
-		ret = get_dist_table(sch, &q->slot_dist,
-				     tb[TCA_NETEM_SLOT_DIST]);
-		if (ret)
-			goto get_table_failure;
-	}
-
+	if (delay_dist)
+		swap(q->delay_dist, delay_dist);
+	if (slot_dist)
+		swap(q->slot_dist, slot_dist);
 	sch->limit = qopt->limit;
 
 	q->latency = PSCHED_TICKS2NS(qopt->latency);
@@ -1044,17 +1041,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 
 unlock:
 	sch_tree_unlock(sch);
-	return ret;
 
-get_table_failure:
-	/* recover clg and loss_model, in case of
-	 * q->clg and q->loss_model were modified
-	 * in get_loss_clg()
-	 */
-	q->clg = old_clg;
-	q->loss_model = old_loss_model;
-
-	goto unlock;
+table_free:
+	dist_free(delay_dist);
+	dist_free(slot_dist);
+	return ret;
 }
 
 static int netem_init(struct Qdisc *sch, struct nlattr *opt,
-- 
2.41.0.178.g377b9f9a00-goog

Re: [PATCH net] sch_netem: fix issues in netem_change() vs get_dist_table()

[Jamal Hadi Salim]

On Thu, Jun 22, 2023 at 2:15 PM Eric Dumazet <edumazet@google.com> wrote:
>
> In blamed commit, I missed that get_dist_table() was allocating
> memory using GFP_KERNEL, and acquiring qdisc lock to perform
> the swap of newly allocated table with current one.
>
> In this patch, get_dist_table() is allocating memory and
> copy user data before we acquire the qdisc lock.
>
> Then we perform swap operations while being protected by the lock.
>
> Note that after this patch netem_change() no longer can do partial changes.
> If an error is returned, qdisc conf is left unchanged.
>
> Fixes: 2174a08db80d ("sch_netem: acquire qdisc lock in netem_change()")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Simon Horman <simon.horman@corigine.com>

LGTM.
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal
> ---
>  net/sched/sch_netem.c | 59 ++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index e79be1b3e74da3c154f7ee23e16cc9e8da8f7106..b93ec2a3454ebceea559299f90533cbf1c0f7c26 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -773,12 +773,10 @@ static void dist_free(struct disttable *d)
>   * signed 16 bit values.
>   */
>
> -static int get_dist_table(struct Qdisc *sch, struct disttable **tbl,
> -                         const struct nlattr *attr)
> +static int get_dist_table(struct disttable **tbl, const struct nlattr *attr)
>  {
>         size_t n = nla_len(attr)/sizeof(__s16);
>         const __s16 *data = nla_data(attr);
> -       spinlock_t *root_lock;
>         struct disttable *d;
>         int i;
>
> @@ -793,13 +791,7 @@ static int get_dist_table(struct Qdisc *sch, struct disttable **tbl,
>         for (i = 0; i < n; i++)
>                 d->table[i] = data[i];
>
> -       root_lock = qdisc_root_sleeping_lock(sch);
> -
> -       spin_lock_bh(root_lock);
> -       swap(*tbl, d);
> -       spin_unlock_bh(root_lock);
> -
> -       dist_free(d);
> +       *tbl = d;
>         return 0;
>  }
>
> @@ -956,6 +948,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>  {
>         struct netem_sched_data *q = qdisc_priv(sch);
>         struct nlattr *tb[TCA_NETEM_MAX + 1];
> +       struct disttable *delay_dist = NULL;
> +       struct disttable *slot_dist = NULL;
>         struct tc_netem_qopt *qopt;
>         struct clgstate old_clg;
>         int old_loss_model = CLG_RANDOM;
> @@ -966,6 +960,18 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>         if (ret < 0)
>                 return ret;
>
> +       if (tb[TCA_NETEM_DELAY_DIST]) {
> +               ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST]);
> +               if (ret)
> +                       goto table_free;
> +       }
> +
> +       if (tb[TCA_NETEM_SLOT_DIST]) {
> +               ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST]);
> +               if (ret)
> +                       goto table_free;
> +       }
> +
>         sch_tree_lock(sch);
>         /* backup q->clg and q->loss_model */
>         old_clg = q->clg;
> @@ -975,26 +981,17 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>                 ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
>                 if (ret) {
>                         q->loss_model = old_loss_model;
> +                       q->clg = old_clg;
>                         goto unlock;
>                 }
>         } else {
>                 q->loss_model = CLG_RANDOM;
>         }
>
> -       if (tb[TCA_NETEM_DELAY_DIST]) {
> -               ret = get_dist_table(sch, &q->delay_dist,
> -                                    tb[TCA_NETEM_DELAY_DIST]);
> -               if (ret)
> -                       goto get_table_failure;
> -       }
> -
> -       if (tb[TCA_NETEM_SLOT_DIST]) {
> -               ret = get_dist_table(sch, &q->slot_dist,
> -                                    tb[TCA_NETEM_SLOT_DIST]);
> -               if (ret)
> -                       goto get_table_failure;
> -       }
> -
> +       if (delay_dist)
> +               swap(q->delay_dist, delay_dist);
> +       if (slot_dist)
> +               swap(q->slot_dist, slot_dist);
>         sch->limit = qopt->limit;
>
>         q->latency = PSCHED_TICKS2NS(qopt->latency);
> @@ -1044,17 +1041,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>
>  unlock:
>         sch_tree_unlock(sch);
> -       return ret;
>
> -get_table_failure:
> -       /* recover clg and loss_model, in case of
> -        * q->clg and q->loss_model were modified
> -        * in get_loss_clg()
> -        */
> -       q->clg = old_clg;
> -       q->loss_model = old_loss_model;
> -
> -       goto unlock;
> +table_free:
> +       dist_free(delay_dist);
> +       dist_free(slot_dist);
> +       return ret;
>  }
>
>  static int netem_init(struct Qdisc *sch, struct nlattr *opt,
> --
> 2.41.0.178.g377b9f9a00-goog
>

Re: [PATCH net] sch_netem: fix issues in netem_change() vs get_dist_table()

[Simon Horman]

On Thu, Jun 22, 2023 at 06:15:03PM +0000, Eric Dumazet wrote:
> In blamed commit, I missed that get_dist_table() was allocating
> memory using GFP_KERNEL, and acquiring qdisc lock to perform
> the swap of newly allocated table with current one.
> 
> In this patch, get_dist_table() is allocating memory and
> copy user data before we acquire the qdisc lock.
> 
> Then we perform swap operations while being protected by the lock.
> 
> Note that after this patch netem_change() no longer can do partial changes.
> If an error is returned, qdisc conf is left unchanged.
> 
> Fixes: 2174a08db80d ("sch_netem: acquire qdisc lock in netem_change()")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Simon Horman <simon.horman@corigine.com>
> ---
>  net/sched/sch_netem.c | 59 ++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 34 deletions(-)

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net] sch_netem: fix issues in netem_change() vs get_dist_table()

[patchwork-bot+netdevbpf]

Hello:

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

On Thu, 22 Jun 2023 18:15:03 +0000 you wrote:
> In blamed commit, I missed that get_dist_table() was allocating
> memory using GFP_KERNEL, and acquiring qdisc lock to perform
> the swap of newly allocated table with current one.
> 
> In this patch, get_dist_table() is allocating memory and
> copy user data before we acquire the qdisc lock.
> 
> [...]

Here is the summary with links:
  - [net] sch_netem: fix issues in netem_change() vs get_dist_table()
    https://git.kernel.org/netdev/net/c/11b73313c124

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