Commit 11b73313 authored by Eric Dumazet's avatar Eric Dumazet Committed by Jakub Kicinski

sch_netem: fix issues in netem_change() vs get_dist_table()

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: 2174a08d ("sch_netem: acquire qdisc lock in netem_change()")
Reported-by: default avatarsyzbot <syzkaller@googlegroups.com>
Signed-off-by: default avatarEric Dumazet <edumazet@google.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: default avatarJamal Hadi Salim <jhs@mojatatu.com>
Reviewed-by: default avatarSimon Horman <simon.horman@corigine.com>
Link: https://lore.kernel.org/r/20230622181503.2327695-1-edumazet@google.comSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent eb441289
...@@ -773,12 +773,10 @@ static void dist_free(struct disttable *d) ...@@ -773,12 +773,10 @@ static void dist_free(struct disttable *d)
* signed 16 bit values. * signed 16 bit values.
*/ */
static int get_dist_table(struct Qdisc *sch, struct disttable **tbl, static int get_dist_table(struct disttable **tbl, const struct nlattr *attr)
const struct nlattr *attr)
{ {
size_t n = nla_len(attr)/sizeof(__s16); size_t n = nla_len(attr)/sizeof(__s16);
const __s16 *data = nla_data(attr); const __s16 *data = nla_data(attr);
spinlock_t *root_lock;
struct disttable *d; struct disttable *d;
int i; int i;
...@@ -793,13 +791,7 @@ static int get_dist_table(struct Qdisc *sch, struct disttable **tbl, ...@@ -793,13 +791,7 @@ static int get_dist_table(struct Qdisc *sch, struct disttable **tbl,
for (i = 0; i < n; i++) for (i = 0; i < n; i++)
d->table[i] = data[i]; d->table[i] = data[i];
root_lock = qdisc_root_sleeping_lock(sch); *tbl = d;
spin_lock_bh(root_lock);
swap(*tbl, d);
spin_unlock_bh(root_lock);
dist_free(d);
return 0; return 0;
} }
...@@ -956,6 +948,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, ...@@ -956,6 +948,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
{ {
struct netem_sched_data *q = qdisc_priv(sch); struct netem_sched_data *q = qdisc_priv(sch);
struct nlattr *tb[TCA_NETEM_MAX + 1]; struct nlattr *tb[TCA_NETEM_MAX + 1];
struct disttable *delay_dist = NULL;
struct disttable *slot_dist = NULL;
struct tc_netem_qopt *qopt; struct tc_netem_qopt *qopt;
struct clgstate old_clg; struct clgstate old_clg;
int old_loss_model = CLG_RANDOM; int old_loss_model = CLG_RANDOM;
...@@ -966,6 +960,18 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, ...@@ -966,6 +960,18 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
if (ret < 0) if (ret < 0)
return ret; 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); sch_tree_lock(sch);
/* backup q->clg and q->loss_model */ /* backup q->clg and q->loss_model */
old_clg = q->clg; old_clg = q->clg;
...@@ -975,26 +981,17 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, ...@@ -975,26 +981,17 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]); ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
if (ret) { if (ret) {
q->loss_model = old_loss_model; q->loss_model = old_loss_model;
q->clg = old_clg;
goto unlock; goto unlock;
} }
} else { } else {
q->loss_model = CLG_RANDOM; q->loss_model = CLG_RANDOM;
} }
if (tb[TCA_NETEM_DELAY_DIST]) { if (delay_dist)
ret = get_dist_table(sch, &q->delay_dist, swap(q->delay_dist, delay_dist);
tb[TCA_NETEM_DELAY_DIST]); if (slot_dist)
if (ret) swap(q->slot_dist, slot_dist);
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;
}
sch->limit = qopt->limit; sch->limit = qopt->limit;
q->latency = PSCHED_TICKS2NS(qopt->latency); q->latency = PSCHED_TICKS2NS(qopt->latency);
...@@ -1044,17 +1041,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, ...@@ -1044,17 +1041,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
unlock: unlock:
sch_tree_unlock(sch); sch_tree_unlock(sch);
return ret;
get_table_failure: table_free:
/* recover clg and loss_model, in case of dist_free(delay_dist);
* q->clg and q->loss_model were modified dist_free(slot_dist);
* in get_loss_clg() return ret;
*/
q->clg = old_clg;
q->loss_model = old_loss_model;
goto unlock;
} }
static int netem_init(struct Qdisc *sch, struct nlattr *opt, static int netem_init(struct Qdisc *sch, struct nlattr *opt,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment