netdev - Re: [PATCH net-next v3 2/2] net, neigh: introduce interval_probe

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:   Wed, 15 Jun 2022 18:39:53 +0300
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Yuwei Wang <wangyuweihx@...il.com>
CC:     davem@...emloft.net, Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>, roopa@...dia.com,
        dsahern@...nel.org, 秦迪 <qindi@...ff.weibo.com>,
        netdev@...r.kernel.org, wangyuweihx <wangyuweihx@...il.com>
Subject: Re: [PATCH net-next v3 2/2] net, neigh: introduce interval_probe_time for periodic probe

On 15 June 2022 18:28:27 EEST, Yuwei Wang <wangyuweihx@...il.com> wrote:
>On Tue, 14 Jun 2022 at 17:10, Nikolay Aleksandrov <razor@...ckwall.org> wrote:
>> > @@ -2255,6 +2257,7 @@ static const struct nla_policy nl_ntbl_parm_policy[NDTPA_MAX+1] = {
>> >       [NDTPA_ANYCAST_DELAY]           = { .type = NLA_U64 },
>> >       [NDTPA_PROXY_DELAY]             = { .type = NLA_U64 },
>> >       [NDTPA_LOCKTIME]                = { .type = NLA_U64 },
>> > +     [NDTPA_INTERVAL_PROBE_TIME]     = { .type = NLA_U64, .min = 1 },
>>
>> shouldn't the min be MSEC_PER_SEC (1 sec minimum) ?
>
>thanks, I will make it match the option ;)
>
>> >
>> > +static int neigh_proc_dointvec_jiffies_positive(struct ctl_table *ctl, int write,
>> > +                                             void *buffer, size_t *lenp,
>> > +                                             loff_t *ppos)
>>
>> Do we need the proc entry to be in jiffies when the netlink option is in ms?
>> Why not make it directly in ms (with _ms similar to other neigh _ms time options) ?
>>
>> IMO, it would be better to be consistent with the netlink option which sets it in ms.
>>
>> It seems the _ms options were added later and usually people want a more understandable
>> value, I haven't seen anyone wanting a jiffies version of a ms interval variable. :)
>>
>
>It was in jiffies because this entry was separated from `DELAY_PROBE_TIME`,
>it keeps nearly all the things the same as `DELAY_PROBE_TIME`,
>they are both configured by seconds and read to jiffies, was `ms` in
>netlink attribute,
>I think it's ok to keep this consistency, and is there a demand
>required to configure it by ms?
>If there is that demand, we can make it configured as ms.
>

no, no demand, just out of user-friendliness :) but 
I get it keeping it as jiffies is also fine 

>> > +{
>> > +     struct ctl_table tmp = *ctl;
>> > +     int ret;
>> > +
>> > +     int min = HZ;
>> > +     int max = INT_MAX;
>> > +
>> > +     tmp.extra1 = &min;
>> > +     tmp.extra2 = &max;
>>
>> hmm, I don't think these min/max match the netlink attribute's min/max.
>
>thanks, I will make it match the attribute ;)
>
>>
>> > +
>> > +     ret = proc_dointvec_jiffies_minmax(&tmp, write, buffer, lenp, ppos);
>> > +     neigh_proc_update(ctl, write);
>> > +     return ret;
>> > +}
>> > +
>> >  int neigh_proc_dointvec(struct ctl_table *ctl, int write, void *buffer,
>> >                       size_t *lenp, loff_t *ppos)
>> >  {
>> > @@ -3658,6 +3683,9 @@ static int neigh_proc_base_reachable_time(struct ctl_table *ctl, int write,
>> >  #define NEIGH_SYSCTL_USERHZ_JIFFIES_ENTRY(attr, name) \
>> >       NEIGH_SYSCTL_ENTRY(attr, attr, name, 0644, neigh_proc_dointvec_userhz_jiffies)
>> >
>> [snip]
>> Cheers,
>>  Nik
>
>Thanks,
>Yuwei Wang