netdev - Re: [PATCH net-next 3/4] net_sched: sch_fq: add 3 bands and WRR scheduling

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]

Date: Mon, 2 Oct 2023 14:23:38 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Willem de Bruijn <willemb@...gle.com>, 
	Soheil Hassas Yeganeh <soheil@...gle.com>, Neal Cardwell <ncardwell@...gle.com>, 
	Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, 
	Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next 3/4] net_sched: sch_fq: add 3 bands and WRR scheduling

On Mon, Oct 2, 2023 at 1:46 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Eric Dumazet <edumazet@...gle.com> writes:
>
> > Before Google adopted FQ for its production servers,
> > we had to ensure AF4 packets would get a higher share
> > than BE1 ones.
> >
> > As discussed this week in Netconf 2023 in Paris, it is time
> > to upstream this for public use.
>
> IIRC, when you mentioned this at Netconf you said the new behaviour
> would probably need to be behind a flag, but I don't see that in this
> series. What was the reason you decided to drop that?


Not a flag, this would add runtime costs.
"struct fq_sched_data" is very big and I try not adding fields unless
really necessary.

I mentioned at Netconf that we had been using this WRR mode for ~5 years
and without a flag.


>
> [..]
> > +static int fq_load_priomap(struct fq_sched_data *q,
> > +                        const struct nlattr *attr,
> > +                        struct netlink_ext_ack *extack)
> > +{
> > +     const struct tc_prio_qopt *map = nla_data(attr);
> > +     int i;
> > +
> > +     if (map->bands != FQ_BANDS) {
> > +             NL_SET_ERR_MSG_MOD(extack, "FQ only supports 3 bands");
> > +             return -EINVAL;
> > +     }
> > +     for (i = 0; i < TC_PRIO_MAX + 1; i++) {
> > +             if (map->priomap[i] >= FQ_BANDS) {
> > +                     NL_SET_ERR_MSG_MOD(extack, "Incorrect field in FQ priomap");
>
> Can we be a bit more specific than just "incorrect" here? Something like
> "FQ priomap field %d maps to a too high band %d"?

Maybe, but note sch_prio does not even set extack for this case.

This is mostly something that only fuzzers like syzbot could possibly
hit, iproute2 will not feed the kernel with such invalid values.