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]
Message-ID: <101855d8-878b-2334-fd5a-85684fd78e12@blackwall.org>
Date:   Tue, 14 Jun 2022 12:10:33 +0300
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Yuwei Wang <wangyuweihx@...il.com>, davem@...emloft.net,
        kuba@...nel.org, edumazet@...gle.com, pabeni@...hat.com
Cc:     daniel@...earbox.net, roopa@...dia.com, dsahern@...nel.org,
        qindi@...ff.weibo.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/2] net, neigh: introduce interval_probe_time
 for periodic probe

On 09/06/2022 13:57, Yuwei Wang wrote:
> commit ed6cd6a17896 ("net, neigh: Set lower cap for neigh_managed_work rearming")
> fixed a case when DELAY_PROBE_TIME is configured to 0, the processing of the
> system work queue hog CPU to 100%, and further more we should introduce
> a new option used by periodic probe
> 
> Signed-off-by: Yuwei Wang <wangyuweihx@...il.com>
> ---
> v3:
> - add limitation to prevent `INTERVAL_PROBE_TIME` to 0
> - remove `NETEVENT_INTERVAL_PROBE_TIME_UPDATE`
> - add .min to NDTPA_INTERVAL_PROBE_TIME
> 
>  include/net/neighbour.h        |  1 +
>  include/uapi/linux/neighbour.h |  1 +
>  include/uapi/linux/sysctl.h    | 37 +++++++++++++++++-----------------
>  net/core/neighbour.c           | 33 ++++++++++++++++++++++++++++--
>  net/decnet/dn_neigh.c          |  1 +
>  net/ipv4/arp.c                 |  1 +
>  net/ipv6/ndisc.c               |  1 +
>  7 files changed, 55 insertions(+), 20 deletions(-)
> 
[snip]
> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index 39c565e460c7..8713c3ea81b2 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -154,6 +154,7 @@ enum {
>  	NDTPA_QUEUE_LENBYTES,		/* u32 */
>  	NDTPA_MCAST_REPROBES,		/* u32 */
>  	NDTPA_PAD,
> +	NDTPA_INTERVAL_PROBE_TIME,	/* u64, msecs */
>  	__NDTPA_MAX
>  };
>  #define NDTPA_MAX (__NDTPA_MAX - 1)
[snip]
>  /* /proc/sys/net/dccp */
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 54625287ee5b..845fad952ce2 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1579,7 +1579,7 @@ static void neigh_managed_work(struct work_struct *work)
>  	list_for_each_entry(neigh, &tbl->managed_list, managed_list)
>  		neigh_event_send_probe(neigh, NULL, false);
>  	queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
> -			   max(NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME), HZ));
> +			   NEIGH_VAR(&tbl->parms, INTERVAL_PROBE_TIME));
>  	write_unlock_bh(&tbl->lock);
>  }
>  
> @@ -2100,7 +2100,9 @@ static int neightbl_fill_parms(struct sk_buff *skb, struct neigh_parms *parms)
>  	    nla_put_msecs(skb, NDTPA_PROXY_DELAY,
>  			  NEIGH_VAR(parms, PROXY_DELAY), NDTPA_PAD) ||
>  	    nla_put_msecs(skb, NDTPA_LOCKTIME,
> -			  NEIGH_VAR(parms, LOCKTIME), NDTPA_PAD))
> +			  NEIGH_VAR(parms, LOCKTIME), NDTPA_PAD) ||
> +	    nla_put_msecs(skb, NDTPA_INTERVAL_PROBE_TIME,
> +			  NEIGH_VAR(parms, INTERVAL_PROBE_TIME), NDTPA_PAD))
>  		goto nla_put_failure;
>  	return nla_nest_end(skb, nest);
>  
> @@ -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) ?

>  };
>  
>  static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,
> @@ -2373,6 +2376,10 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,
>  					      nla_get_msecs(tbp[i]));
>  				call_netevent_notifiers(NETEVENT_DELAY_PROBE_TIME_UPDATE, p);
>  				break;
> +			case NDTPA_INTERVAL_PROBE_TIME:
> +				NEIGH_VAR_SET(p, INTERVAL_PROBE_TIME,
> +					      nla_get_msecs(tbp[i]));
> +				break;
>  			case NDTPA_RETRANS_TIME:
>  				NEIGH_VAR_SET(p, RETRANS_TIME,
>  					      nla_get_msecs(tbp[i]));
> @@ -3562,6 +3569,24 @@ static int neigh_proc_dointvec_zero_intmax(struct ctl_table *ctl, int write,
>  	return ret;
>  }
>  
> +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. :)

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

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