netdev - Re: [PATCH net v2 1/3] net_sched: sch_sfq: use a temporary work area for validating configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGWr4cSM0=fUJLG19J-YB+YzV8dUMHHdd0wJWaKsn0i7DYMZ8w@mail.gmail.com>
Date: Thu, 3 Apr 2025 01:50:37 -0700
From: Octavian Purdila <tavip@...gle.com>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	horms@...nel.org, shuah@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net v2 1/3] net_sched: sch_sfq: use a temporary work area
 for validating configuration

On Wed, Apr 2, 2025 at 1:46 PM Stephen Hemminger
<stephen@...workplumber.org> wrote:
>

Hi Stephen,

Thanks for the review.

> On Wed,  2 Apr 2025 09:27:48 -0700
> Octavian Purdila <tavip@...gle.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.