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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANmJ_FNXSxPtBbESV4Y4Zme6vabgTJFSw0hjZNndfstSvxAeLw@mail.gmail.com>
Date:   Wed, 15 Jun 2022 23:28:27 +0800
From:   Yuwei Wang <wangyuweihx@...il.com>
To:     Nikolay Aleksandrov <razor@...ckwall.org>
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 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.

> > +{
> > +     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