netdev - Re: [PATCH v4 net-next 6/9] net/sched: mqprio: allow per-TC user input of FP adminStatus

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: <20230407193056.3rklegrgmn2yecuu@skbuf>
Date:   Fri, 7 Apr 2023 22:30:56 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Gerhard Engleder <gerhard@...leder-embedded.com>,
        Amritha Nambiar <amritha.nambiar@...el.com>,
        Ferenc Fejes <ferenc.fejes@...csson.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Roger Quadros <rogerq@...nel.org>,
        Pranavi Somisetty <pranavi.somisetty@....com>,
        Harini Katakam <harini.katakam@....com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Michael Sit Wei Hong <michael.wei.hong.sit@...el.com>,
        Mohammad Athari Bin Ismail <mohammad.athari.ismail@...el.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Jacob Keller <jacob.e.keller@...el.com>,
        linux-kernel@...r.kernel.org, Ferenc Fejes <fejes@....elte.hu>,
        Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH v4 net-next 6/9] net/sched: mqprio: allow per-TC user
 input of FP adminStatus

On Fri, Apr 07, 2023 at 02:49:01PM -0400, Jamal Hadi Salim wrote:
> On Fri, Apr 7, 2023 at 12:41 PM Vladimir Oltean <vladimir.oltean@....com> wrote:
> > On Fri, Apr 07, 2023 at 12:22:26PM -0400, Jamal Hadi Salim wrote:
> > > > +enum {
> > > > +       TC_FP_EXPRESS = 1,
> > > > +       TC_FP_PREEMPTIBLE = 2,
> > > > +};
> > >
> > > Suggestion: Add a MAX to this enum (as is traditionally done)..
> >
> > Max what? This doesn't count anything, it just expresses whether the
> > quality of one traffic class, from the Frame Preemption standard's
> > perspective, is express or preemptible...
> >
> > > > @@ -145,13 +149,94 @@ static int mqprio_parse_opt(struct net_device *dev, struct tc_mqprio_qopt *qopt,
> > > >         return 0;
> > > >  }
> > > >
> > > > +static const struct
> > > > +nla_policy mqprio_tc_entry_policy[TCA_MQPRIO_TC_ENTRY_MAX + 1] = {
> > > > +       [TCA_MQPRIO_TC_ENTRY_INDEX]     = NLA_POLICY_MAX(NLA_U32,
> > > > +                                                        TC_QOPT_MAX_QUEUE),
> > >
> > > And use it here...
> >
> > Where? Above or below the comment? I think you mean below (for the
> > policy of TCA_MQPRIO_TC_ENTRY_FP)?
> 
> That was what I meant. I misread that code thinking it was a nested
> TLV range check. If it is only going to be those two specific values,
> I understand - but then wondering why you need a u32; wouldnt a u8 be
> sufficient?

I believe netlink isn't exactly optimized for passing small values; the
netlink attributes are going to be aligned to NLA_ALIGNTO (4) anyway,
so it's not like this is going to save space or something. Also, there's
a policy restricting the maximum, so arbitrarily large values cannot be
passed now, but could be passed later if needed. I did not see any good
enough reason to close that door.

> The only reason you would need a MAX is if it is possible that new
> values greater than TC_FP_PREEMPTIBLE showing up in the future.

Even if someone wants to add TC_FP_KINDA_PREEMPTIBLE = 3 and
TC_FP_PREEMPTIBLE_WITH_STRIPES = 4 in the future, I'm still not sure how
a MAX definition exported by the kernel is going to help them?

I mean, about the only thing that it would avoid is that I wouldn't be
changing the policy definition, but that's rather minor and doesn't
justify exporting something to UAPI? The changed MAX value is only a
property of the kernel headers that the application is compiled with -
it doesn't give the capability of the running kernel.

To see whether TC_FP_PREEMPTIBLE_WITH_STRIPES is supported, the
application would have to try it and see if it fails. Which is also the
case right now with TC_FP_PREEMPTIBLE.

> > > Lead up question: if the max is 16 then can preemptible_tcs for example be u32?
> >
> > I don't understand this question, sorry. preemptible_tcs is declared as
> > "unsigned long", which IIUC is at least 32-bit.
> 
> I meant: if you only had 16 possible values, meaning 16 bits are
> sufficient, (although i may be misunderstanding the goal of those
> bits) why not be explicit and use the proper type/size?

If you think it's valuable to change the type of preemptible_tcs from
unsigned long to u16 and that's a more "proper" type, I can do so.