netdev - Re: [PATCH v3 net-next] net/sched: add skbprio scheduler

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:   Fri, 13 Jul 2018 11:26:28 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     Michel Machado <michel@...irati.com.br>,
        Nishanth Devarajan <ndev2021@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Cody Doucette <doucette@...edu>
Subject: Re: [PATCH v3 net-next] net/sched: add skbprio scheduler

On Fri, Jul 13, 2018 at 6:04 AM Marcelo Ricardo Leitner
<marcelo.leitner@...il.com> wrote:
>
> On Thu, Jul 12, 2018 at 11:05:45PM -0700, Cong Wang wrote:
> > On Wed, Jul 11, 2018 at 12:33 PM Marcelo Ricardo Leitner
> > <marcelo.leitner@...il.com> wrote:
> > >
> > > On Tue, Jul 10, 2018 at 07:25:53PM -0700, Cong Wang wrote:
> > > > On Mon, Jul 9, 2018 at 2:40 PM Marcelo Ricardo Leitner
> > > > <marcelo.leitner@...il.com> wrote:
> > > > >
> > > > > On Mon, Jul 09, 2018 at 05:03:31PM -0400, Michel Machado wrote:
> > > > > >    Changing TC_PRIO_MAX from 15 to 63 risks breaking backward compatibility
> > > > > > with applications.
> > > > >
> > > > > If done, it needs to be done carefully, indeed. I don't know if it's
> > > > > doable, neither I know how hard is your requirement for 64 different
> > > > > priorities.
> > > >
> > > > struct tc_prio_qopt {
> > > >         int     bands;                  /* Number of bands */
> > > >         __u8    priomap[TC_PRIO_MAX+1]; /* Map: logical priority -> PRIO band */
> > > > };
> > > >
> > > > How would you do it carefully?
> > >
> > > quick shot, multiplex v1 and v2 formats based on bands and sizeof():
> > >
> > > #define TCQ_PRIO_BANDS_V1       16
> > > #define TCQ_PRIO_BANDS_V2       64
> > > #define TC_PRIO_MAX_V2          64
> > >
> > > struct tc_prio_qopt_v2 {
> > >         int     bands;                  /* Number of bands */
> > >         __u8    priomap[TC_PRIO_MAX_V2+1]; /* Map: logical priority -> PRIO band */
> > > };
> > >
> >
> > Good try, but:
> >
> > 1. You don't take padding into account, although the difference
> > between 16 and 64 is big here. If it were 16 and 20, almost certainly
> > wouldn't work.
>
> It still would work, no matter how much padding you have, as currently
> you can't use more than 3 bands.

I am lost.

With your proposal above, you have 16 bands for V1 and 64 bands
for V2, where does 3 come from???


>
> >
> > 2. What if I compile a new iproute2 on an old kernel? The iproute2
> > will use V2, while old kernel has no knowledge of V2, so it only
> > copies a part of V2 in the end....
>
> Yes, and that's not a problem:
> - Either bands is > 3 and it will return EINVAL, protecting from
>   reading beyond the buffer.
> - Or 2 <= bands <= 3 and it will handle it as a _v1 struct, and use
>   only the original size.

Again why 3 not 16 or 64 ??

Also, why does an old kernel has the logic in its binary to determine
this?

>
> iproute2 (or other app) may still use _v1 if it wants, btw.

Yes, old iproute2 must still have v1, what's point? Are you
suggesting new iproute2 should still have v1 after you propose
v1 and v2 for kernel?

I must seriously miss something. Please help.

Thanks!