| 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
| ||
|
Message-ID: <b5cf7fac361752d925f663d9a9b0b8415084f7d3.camel@redhat.com>
Date: Tue, 24 May 2022 10:38:22 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Yuwei Wang <wangyuweihx@...il.com>, davem@...emloft.net,
kuba@...nel.org, edumazet@...gle.com
Cc: daniel@...earbox.net, roopa@...dia.com, dsahern@...nel.org,
qindi@...ff.weibo.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time
for periodic probe
On Sun, 2022-05-22 at 03:17 +0000, Yuwei Wang wrote:
> commit 7482e3841d52 ("net, neigh: Add NTF_MANAGED flag for managed neighbor entries")
> neighbor entries which with NTF_EXT_MANAGED flags will periodically call neigh_event_send()
> for performing the resolution. and the interval was set to DELAY_PROBE_TIME
>
> DELAY_PROBE_TIME was configured as the first probe time delay, and it makes sense to set it to `0`.
>
> when DELAY_PROBE_TIME is `0`, the queue_delayed_work of neighbor entries with NTF_EXT_MANAGED will
> be called recursively with no interval, and then threads of `system_power_efficient_wq` will consume 100% cpu.
>
> as commit messages mentioned in the above commit, we should introduce a new option which means resolution interval.
>
> Signed-off-by: Yuwei Wang <wangyuweihx@...il.com>
> ---
> v2:
> - move `NDTPA_INTERVAL_PROBE_TIME` to the tail of uAPI enum
> - add `NDTPA_INTERVAL_PROBE_TIME` to `nl_ntbl_parm_policy`
> - add detail explain for the behevior when `DELAY_PROBE_TIME` is `0` in
> commit messaage
>
> meanwhile, we should replace `DELAY_PROBE_TIME` with `INTERVAL_PROBE_TIME`
> in `drivers/net/ethernet/mellanox` after this patch was merged
>
> and should we remove `include/uapi/linux/sysctl.h` seems it is no
> longer be used.
>
> include/net/neighbour.h | 3 ++-
> include/net/netevent.h | 1 +
> include/uapi/linux/neighbour.h | 1 +
> include/uapi/linux/sysctl.h | 37 +++++++++++++++++-----------------
> net/core/neighbour.c | 15 ++++++++++++--
> net/decnet/dn_neigh.c | 1 +
> net/ipv4/arp.c | 1 +
> net/ipv6/ndisc.c | 1 +
> 8 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 87419f7f5421..75786903f1d4 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -48,6 +48,7 @@ enum {
> NEIGH_VAR_RETRANS_TIME,
> NEIGH_VAR_BASE_REACHABLE_TIME,
> NEIGH_VAR_DELAY_PROBE_TIME,
> + NEIGH_VAR_INTERVAL_PROBE_TIME,
> NEIGH_VAR_GC_STALETIME,
> NEIGH_VAR_QUEUE_LEN_BYTES,
> NEIGH_VAR_PROXY_QLEN,
> @@ -64,7 +65,7 @@ enum {
> NEIGH_VAR_GC_THRESH1,
> NEIGH_VAR_GC_THRESH2,
> NEIGH_VAR_GC_THRESH3,
> - NEIGH_VAR_MAX
> + NEIGH_VAR_MAX,
You should avoid style-only changes in area not touched otherwise by
this
> };
>
> struct neigh_parms {
> diff --git a/include/net/netevent.h b/include/net/netevent.h
> index 4107016c3bb4..121df77d653e 100644
> --- a/include/net/netevent.h
> +++ b/include/net/netevent.h
> @@ -26,6 +26,7 @@ enum netevent_notif_type {
> NETEVENT_NEIGH_UPDATE = 1, /* arg is struct neighbour ptr */
> NETEVENT_REDIRECT, /* arg is struct netevent_redirect ptr */
> NETEVENT_DELAY_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
> + NETEVENT_INTERVAL_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
Are you sure we need to notify the drivers about this parameter change?
The host will periodically resolve the neighbours, and that should work
regardless of the NIC offload. I think we don't need additional
notifications.
> NETEVENT_IPV4_MPATH_HASH_UPDATE, /* arg is struct net ptr */
> NETEVENT_IPV6_MPATH_HASH_UPDATE, /* arg is struct net ptr */
> NETEVENT_IPV4_FWD_UPDATE_PRIORITY_UPDATE, /* arg is struct net ptr */
> 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)
> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
> index 6a3b194c50fe..53f06bfd2a37 100644
> --- a/include/uapi/linux/sysctl.h
> +++ b/include/uapi/linux/sysctl.h
> @@ -584,24 +584,25 @@ enum {
>
> /* /proc/sys/net/<protocol>/neigh/<dev> */
> enum {
> - NET_NEIGH_MCAST_SOLICIT=1,
> - NET_NEIGH_UCAST_SOLICIT=2,
> - NET_NEIGH_APP_SOLICIT=3,
> - NET_NEIGH_RETRANS_TIME=4,
> - NET_NEIGH_REACHABLE_TIME=5,
> - NET_NEIGH_DELAY_PROBE_TIME=6,
> - NET_NEIGH_GC_STALE_TIME=7,
> - NET_NEIGH_UNRES_QLEN=8,
> - NET_NEIGH_PROXY_QLEN=9,
> - NET_NEIGH_ANYCAST_DELAY=10,
> - NET_NEIGH_PROXY_DELAY=11,
> - NET_NEIGH_LOCKTIME=12,
> - NET_NEIGH_GC_INTERVAL=13,
> - NET_NEIGH_GC_THRESH1=14,
> - NET_NEIGH_GC_THRESH2=15,
> - NET_NEIGH_GC_THRESH3=16,
> - NET_NEIGH_RETRANS_TIME_MS=17,
> - NET_NEIGH_REACHABLE_TIME_MS=18,
> + NET_NEIGH_MCAST_SOLICIT = 1,
> + NET_NEIGH_UCAST_SOLICIT = 2,
> + NET_NEIGH_APP_SOLICIT = 3,
> + NET_NEIGH_RETRANS_TIME = 4,
> + NET_NEIGH_REACHABLE_TIME = 5,
> + NET_NEIGH_DELAY_PROBE_TIME = 6,
> + NET_NEIGH_GC_STALE_TIME = 7,
> + NET_NEIGH_UNRES_QLEN = 8,
> + NET_NEIGH_PROXY_QLEN = 9,
> + NET_NEIGH_ANYCAST_DELAY = 10,
> + NET_NEIGH_PROXY_DELAY = 11,
> + NET_NEIGH_LOCKTIME = 12,
> + NET_NEIGH_GC_INTERVAL = 13,
> + NET_NEIGH_GC_THRESH1 = 14,
> + NET_NEIGH_GC_THRESH2 = 15,
> + NET_NEIGH_GC_THRESH3 = 16,
> + NET_NEIGH_RETRANS_TIME_MS = 17,
> + NET_NEIGH_REACHABLE_TIME_MS = 18,
> + NET_NEIGH_INTERVAL_PROBE_TIME = 19,
> };
>
> /* /proc/sys/net/dccp */
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 47b6c1f0fdbb..92447f04cf07 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,
> - NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME));
> + 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 },
since a 0 value would not make any sense here and will cause problems,
what about adding '.min = 1' ?
Thanks!
Paolo
Powered by blists - more mailing lists