net_sched: sch_sfq: use a temporary work area for validating configuration

Many configuration parameters have influence on others (e.g. divisor
-> flows -> limit, depth -> limit) and so it is difficult to correctly
do all of the validation before applying the configuration. And if a
validation error is detected late it is difficult to roll back a
partially applied configuration.

To avoid these issues use a temporary work area to update and validate
the configuration and only then apply the configuration to the
internal state.

Signed-off-by: Octavian Purdila <tavip@google.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 65d5b59da58303718255c4e2bd61fc34728a7d68..7714ae94e05217130b453d5b59925de0d4b1bd91 100644 (file)
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -631,6 +631,15 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
        struct red_parms *p = NULL;
        struct sk_buff *to_free = NULL;
        struct sk_buff *tail = NULL;
+       unsigned int maxflows;
+       unsigned int quantum;
+       unsigned int divisor;
+       int perturb_period;
+       u8 headdrop;
+       u8 maxdepth;
+       int limit;
+       u8 flags;
+
 
        if (opt->nla_len < nla_attr_size(sizeof(*ctl)))
                return -EINVAL;
@@ -656,36 +665,59 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
                NL_SET_ERR_MSG_MOD(extack, "invalid limit");
                return -EINVAL;
        }
+
        sch_tree_lock(sch);
+
+       limit = q->limit;
+       divisor = q->divisor;
+       headdrop = q->headdrop;
+       maxdepth = q->maxdepth;
+       maxflows = q->maxflows;
+       perturb_period = q->perturb_period;
+       quantum = q->quantum;
+       flags = q->flags;
+
+       /* update and validate configuration */
        if (ctl->quantum)
-               q->quantum = ctl->quantum;
-       WRITE_ONCE(q->perturb_period, ctl->perturb_period * HZ);
+               quantum = ctl->quantum;
+       perturb_period = ctl->perturb_period * HZ;
        if (ctl->flows)
-               q->maxflows = min_t(u32, ctl->flows, SFQ_MAX_FLOWS);
+               maxflows = min_t(u32, ctl->flows, SFQ_MAX_FLOWS);
        if (ctl->divisor) {
-               q->divisor = ctl->divisor;
-               q->maxflows = min_t(u32, q->maxflows, q->divisor);
+               divisor = ctl->divisor;
+               maxflows = min_t(u32, maxflows, divisor);
        }
        if (ctl_v1) {
                if (ctl_v1->depth)
-                       q->maxdepth = min_t(u32, ctl_v1->depth, SFQ_MAX_DEPTH);
+                       maxdepth = min_t(u32, ctl_v1->depth, SFQ_MAX_DEPTH);
                if (p) {
-                       swap(q->red_parms, p);
-                       red_set_parms(q->red_parms,
+                       red_set_parms(p,
                                      ctl_v1->qth_min, ctl_v1->qth_max,
                                      ctl_v1->Wlog,
                                      ctl_v1->Plog, ctl_v1->Scell_log,
                                      NULL,
                                      ctl_v1->max_P);
                }
-               q->flags = ctl_v1->flags;
-               q->headdrop = ctl_v1->headdrop;
+               flags = ctl_v1->flags;
+               headdrop = ctl_v1->headdrop;
        }
        if (ctl->limit) {
-               q->limit = min_t(u32, ctl->limit, q->maxdepth * q->maxflows);
-               q->maxflows = min_t(u32, q->maxflows, q->limit);
+               limit = min_t(u32, ctl->limit, maxdepth * maxflows);
+               maxflows = min_t(u32, maxflows, limit);
        }
 
+       /* commit configuration */
+       q->limit = limit;
+       q->divisor = divisor;
+       q->headdrop = headdrop;
+       q->maxdepth = maxdepth;
+       q->maxflows = maxflows;
+       WRITE_ONCE(q->perturb_period, perturb_period);
+       q->quantum = quantum;
+       q->flags = flags;
+       if (p)
+               swap(q->red_parms, p);
+
        qlen = sch->q.qlen;
        while (sch->q.qlen > q->limit) {
                dropped += sfq_drop(sch, &to_free);