| Message ID | 20250402162750.1671155-2-tavip@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net_sched: sch_sfq: reject a derived limit of 1 | expand |
On Wed, 2 Apr 2025 09:27:48 -0700 Octavian Purdila <tavip@google.com> wrote: > diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c > index 65d5b59da583..1af06cd5034a 100644 > --- a/net/sched/sch_sfq.c > +++ b/net/sched/sch_sfq.c > @@ -631,6 +631,16 @@ 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; > + /* work area for validating changes before committing them */ Unnecessary comment. > + int limit; > + unsigned int divisor; > + unsigned int maxflows; > + int perturb_period; > + unsigned int quantum; > + u8 headdrop; > + u8 maxdepth; > + u8 flags; > + Network code prefers reverse christmas tree style declaration order. + /* copy configuration to work area */ + 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; Comment is unneeded. Rather than doing individual fields, why not just use a whole temporary structure?
On Wed, Apr 2, 2025 at 1:46 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > Hi Stephen, Thanks for the review. > On Wed, 2 Apr 2025 09:27:48 -0700 > Octavian Purdila <tavip@google.com> wrote: > > > diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c > > index 65d5b59da583..1af06cd5034a 100644 > > --- a/net/sched/sch_sfq.c > > +++ b/net/sched/sch_sfq.c > > @@ -631,6 +631,16 @@ 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; > > + /* work area for validating changes before committing them */ > Unnecessary comment. Fair, I'll remove the comments (here and below). > > + int limit; > > + unsigned int divisor; > > + unsigned int maxflows; > > + int perturb_period; > > + unsigned int quantum; > > + u8 headdrop; > > + u8 maxdepth; > > + u8 flags; > > + > > Network code prefers reverse christmas tree style declaration order. > Thanks, I'll fix the declaration order. > + /* copy configuration to work area */ > + 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; > > Comment is unneeded. Rather than doing individual fields, why not just use > a whole temporary structure? Do you mean save/restore a full struct sfq_sched_data? I am not sure it is safe to do so since it includes a struct timer_list which, if I am not mistaken, could change under us during save/restore as other timers are added / removed. Beside that, it is quite big (688 bytes on 64bit) so not ideal to put it on stack, would probably need to allocate / free it. And we need to copy a lot more fields than we need. It seems a bit inefficient.
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c index 65d5b59da583..1af06cd5034a 100644 --- a/net/sched/sch_sfq.c +++ b/net/sched/sch_sfq.c @@ -631,6 +631,16 @@ 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; + /* work area for validating changes before committing them */ + int limit; + unsigned int divisor; + unsigned int maxflows; + int perturb_period; + unsigned int quantum; + u8 headdrop; + u8 maxdepth; + u8 flags; + if (opt->nla_len < nla_attr_size(sizeof(*ctl))) return -EINVAL; @@ -656,36 +666,60 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt, NL_SET_ERR_MSG_MOD(extack, "invalid limit"); return -EINVAL; } + sch_tree_lock(sch); + + /* copy configuration to work area */ + 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, no return from this point further */ + 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);
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> --- net/sched/sch_sfq.c | 58 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 12 deletions(-)