linux-kernel - Re: [v2] net_sched: act_police: add 2 new attributes to support police 64bit rate and peakrate

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] [day] [month] [year] [list]

Date:   Fri, 30 Aug 2019 19:30:47 -0500
From:   "David Z. Dai" <zdai@...ux.vnet.ibm.com>
To:     David Miller <davem@...emloft.net>
Cc:     xiyou.wangcong@...il.com, jhs@...atatu.com, jiri@...nulli.us,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        zdai@...ibm.com
Subject: Re: [v2] net_sched: act_police: add 2 new attributes to support
 police 64bit rate and peakrate

On Fri, 2019-08-30 at 13:33 -0700, David Miller wrote:
> From: "David Z. Dai" <zdai@...ux.vnet.ibm.com>
> Date: Fri, 30 Aug 2019 15:03:52 -0500
> 
> > I have the impression that last parameter num value should be larger
> > than the attribute num value in 2nd parameter (TC_POLICE_RATE64 in this
> > case).
> 
> The argument in question is explicitly the "padding" value.
> 
> Please explain in detail where you got the impression that the
> argument has to be larger?
In include/uapi/linux/pkt_sched.h header:
For HTB:
enum {
        TCA_HTB_UNSPEC,
        TCA_HTB_PARMS,
        TCA_HTB_INIT,
        TCA_HTB_CTAB,
        TCA_HTB_RTAB,
        TCA_HTB_DIRECT_QLEN,
        TCA_HTB_RATE64,    /* <--- */
        TCA_HTB_CEIL64,    /* <--- */
        TCA_HTB_PAD,       /* <--- */
        __TCA_HTB_MAX,
};
/* TCA_HTB_RATE64,TCA_HTB_CEIL64 are declared *before* TCA_HTB_PAD */

For TBF:
enum {
        TCA_TBF_UNSPEC,
        TCA_TBF_PARMS,
        TCA_TBF_RTAB,
        TCA_TBF_PTAB,
        TCA_TBF_RATE64,   /* <--- */
        TCA_TBF_PRATE64,  /* <--- */
        TCA_TBF_BURST,
        TCA_TBF_PBURST,
        TCA_TBF_PAD,      /* <--- */
        __TCA_TBF_MAX,
};
/* TCA_TBF_RATE64, TCA_TBF_PRATE64 are declared *before* TCA_TBF_PAD */

For HTB, in net/sched/sch_htb.c file, htb_dump_class() routine:
        if ((cl->rate.rate_bytes_ps >= (1ULL << 32)) &&
            nla_put_u64_64bit(skb, TCA_HTB_RATE64,
cl->rate.rate_bytes_ps,
                              TCA_HTB_PAD))
                goto nla_put_failure;
        if ((cl->ceil.rate_bytes_ps >= (1ULL << 32)) &&
            nla_put_u64_64bit(skb, TCA_HTB_CEIL64,
cl->ceil.rate_bytes_ps,
                              TCA_HTB_PAD))
                goto nla_put_failure;

For TBF, in net/sched/sch_tbf.c file, tbf_dump() routine:
       if (q->rate.rate_bytes_ps >= (1ULL << 32) &&
            nla_put_u64_64bit(skb, TCA_TBF_RATE64,
q->rate.rate_bytes_ps,
                              TCA_TBF_PAD))
                goto nla_put_failure;
        if (tbf_peak_present(q) &&
            q->peak.rate_bytes_ps >= (1ULL << 32) &&
            nla_put_u64_64bit(skb, TCA_TBF_PRATE64,
q->peak.rate_bytes_ps,
                              TCA_TBF_PAD))
                goto nla_put_failure;

The last parameter used TCA_TBF_PAD, TCA_TBF_PAD are all declared
*after* those attributes.

I am trying to keep it consistent in police part. That's where my
impression is coming from.

Now for suggestion/comment, do you think is it better to add a new PAD
attribute in include/uapi/pkt_cls.h like this:
enum {
        TCA_POLICE_UNSPEC,
        TCA_POLICE_TBF,
        TCA_POLICE_RATE,
        TCA_POLICE_PEAKRATE,
        TCA_POLICE_AVRATE,
        TCA_POLICE_RESULT,
        TCA_POLICE_TM,
        TCA_POLICE_PAD,
        TCA_POLICE_RATE64,      /* <--- */
        TCA_POLICE_PEAKRATE64,  /* <--- */
        TCA_POLICE_PAD2,        /* <--- new PAD */
        __TCA_POLICE_MAX
#define TCA_POLICE_RESULT TCA_POLICE_RESULT
#};
Then use this TCA_POLICE_PAD2 as the last parameter in
nla_put_u64_64bit()? 

Thanks!