Re: [PATCH v1 net 2/4] neighbour: Enforce min/max to NDTPA_INTERVAL_PROBE_TIME_MS.

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 2

- [High] The sysctl half of this fix is dead code: in
  neigh_proc_dointvec_ms_jiffies_positive()…
- [Low] Documentation/netlink/specs/rt-neigh.yaml declares interval-
  probe-time-ms as a bare `type: u64` with no `checks:` block (lines…
- [Low] Documentation/networking/ip-sysctl.rst:249-253 documents
  neigh/default/interval_probe_time_ms as having only a lower bound…

Pre-existing issues:
- [Medium] Only NDTPA_INTERVAL_PROBE_TIME_MS gains range validation.

--

neighbour: Enforce min/max to NDTPA_INTERVAL_PROBE_TIME_MS.

This adds NLA_POLICY_FULL_RANGE() validation to
NDTPA_INTERVAL_PROBE_TIME_MS so values outside [1, one day in ms] are
rejected on the RTM_SETNEIGHTBL path, and installs the same maximum on the
interval_probe_time_ms sysctl handler.  A few questions below.

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 4b17c2a155947..454a8b8f3aa01 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2359,6 +2359,13 @@ static const struct nla_policy nl_neightbl_policy[NDTA_MAX+1] = {
>  	[NDTA_PARMS]		= { .type = NLA_NESTED },
>  };
>  
> +#define NTBL_PARM_MS_MAX	(24 * 60 * 60 * MSEC_PER_SEC)
> +
> +static const struct netlink_range_validation nl_ntbl_parm_ms_range = {
> +	.min = 1,
> +	.max = NTBL_PARM_MS_MAX,
> +};
> +

[Severity: Low]
Should Documentation/netlink/specs/rt-neigh.yaml be updated in the same
patch?  In the neighbour-table-parms attribute set the attribute is still
declared with no checks block:

      -
        name: interval-probe-time-ms
        type: u64

With nl_ntbl_parm_ms_range now enforcing [1, 86400000], ynl-generated
bindings and the generated documentation describe an unconstrained u64 for
an attribute the kernel rejects with -EINVAL outside a one-day window.
Would adding "checks: min: 1 / max: 86400000" here keep the spec and the
policy in agreement?

>  static const struct nla_policy nl_ntbl_parm_policy[NDTPA_MAX+1] = {
>  	[NDTPA_IFINDEX]			= { .type = NLA_U32 },
>  	[NDTPA_QUEUE_LEN]		= { .type = NLA_U32 },
> @@ -2375,7 +2382,8 @@ 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 },

[Severity: Medium]
This isn't a bug introduced by this patch, but the sibling millisecond
attributes above have the same shape of problem and keep plain
{ .type = NLA_U64 }: NDTPA_BASE_REACHABLE_TIME, NDTPA_GC_STALETIME,
NDTPA_DELAY_PROBE_TIME, NDTPA_RETRANS_TIME, NDTPA_ANYCAST_DELAY,
NDTPA_PROXY_DELAY and NDTPA_LOCKTIME.

neightbl_set() feeds each of them through nla_get_msecs() into the int
NEIGH_VAR array:

			case NDTPA_BASE_REACHABLE_TIME:
				NEIGH_VAR_SET(p, BASE_REACHABLE_TIME,
					      nla_get_msecs(tbp[i]));

For a u64 above jiffies_to_msecs(MAX_JIFFY_OFFSET), msecs_to_jiffies()
clamps to MAX_JIFFY_OFFSET (0x3FFFFFFFFFFFFFFE on 64-bit), which narrows to
-2 in the int p->data[] slot.

That negative value then reaches the same sinks the commit message
describes, in neigh_periodic_work():

	queue_delayed_work(system_power_efficient_wq, &tbl->gc_work,
			      NEIGH_VAR(&tbl->parms, BASE_REACHABLE_TIME) >> 1);

and for the proxy delay:

	unsigned long proxy_delay = NEIGH_VAR(p, PROXY_DELAY);

	return proxy_delay ?
	       jiffies + get_random_u32_below(proxy_delay) : jiffies;

Since NTBL_PARM_MS_MAX and nl_ntbl_parm_ms_range are named generically
already, could they be reused for these attributes too, or is that
intentionally left for a follow-up?

> -	[NDTPA_INTERVAL_PROBE_TIME_MS]	= { .type = NLA_U64, .min = 1 },
> +	[NDTPA_INTERVAL_PROBE_TIME_MS]	= NLA_POLICY_FULL_RANGE(NLA_U64,
> +								&nl_ntbl_parm_ms_range),
>  };
>  
>  static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,

[ ... ]

> @@ -3670,12 +3678,13 @@ static int neigh_proc_dointvec_ms_jiffies_positive(const struct ctl_table *ctl,
>  						   void *buffer, size_t *lenp, loff_t *ppos)
>  {
>  	struct ctl_table tmp = *ctl;
> -	int ret;
> +	int ret, min, max;
>  
> -	int min = msecs_to_jiffies(1);
> +	min = msecs_to_jiffies(1);
> +	max = msecs_to_jiffies(NTBL_PARM_MS_MAX);
>  
>  	tmp.extra1 = &min;
> -	tmp.extra2 = NULL;
> +	tmp.extra2 = &max;
>  
>  	ret = proc_dointvec_ms_jiffies_minmax(&tmp, write, buffer, lenp, ppos);

[Severity: High]
Does this half of the fix actually take effect at this commit?  As far as I
can tell proc_dointvec_ms_jiffies_minmax() never looks at extra1/extra2, so
both the new max and the pre-existing min are inert here.

kernel/time/jiffies.c:do_proc_int_conv_ms_jiffies_minmax() passes
k_ptr_range_check = false:

	return proc_int_conv(negp, u_ptr, k_ptr, dir, tbl, false,
			     sysctl_u2k_int_conv_ms, sysctl_k2u_int_conv_ms);

and kernel/sysctl.c:proc_int_conv() only performs the comparison when that
flag is set:

	if (k_ptr_range_check) {
		...
		if ((tbl->extra1 && *(int *)tbl->extra1 > tmp_k) ||
		    (tbl->extra2 && *(int *)tbl->extra2 < tmp_k))
			return -EINVAL;
		WRITE_ONCE(*k_ptr, tmp_k);
	} else
		return user_to_kern(negp, u_ptr, k_ptr);

The only remaining filter is proc_int_u2k_conv_uop(), which rejects
magnitudes above INT_MAX but lets 0 and negative values through:

	if (*negp) {
		if (u > (ulong) INT_MAX + 1)
			return -EINVAL;
		WRITE_ONCE(*k_ptr, -u);

So does

  echo 0 > /proc/sys/net/ipv4/neigh/default/interval_probe_time_ms

still store 0 jiffies, leaving the failure mode described in the commit
message reachable through sysctl in neigh_managed_work()?

	queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
			   NEIGH_VAR(&tbl->parms, INTERVAL_PROBE_TIME_MS));

If that is right, the commit message statement

    The same max check is applied to sysctl as well.

does not hold at this SHA, nor at the end of the series.  The prerequisite
sysctl range-check change is mentioned only below the scissors line:

    Currently, the sysctl range check is not applied to
    interval_probe_time_ms, which needs this fix:
    https://lore.kernel.org/linux-fsdevel/20260905233819.1064529-2-kuniyu@google.com/

That text is stripped when the patch is applied, so the permanent git log
keeps the unqualified claim.  Could the dependency be spelled out above the
scissors line, or the two patches ordered so this one lands after the
sysctl fix?

[Severity: Low]
Also, Documentation/networking/ip-sysctl.rst still documents only a lower
bound for this knob:

  neigh/default/interval_probe_time_ms - INTEGER
	The probe interval for neighbor entries with NTF_MANAGED flag,
	the min value is 1.

	Default: 5000

Should that be updated to mention the new 24 hour maximum?  The bound is
already observable on the RTM_SETNEIGHTBL path, and would become observable
on the sysctl file once the prerequisite range-check fix lands.

>  	neigh_proc_update(ctl, write);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907215853.3709987-1-kuniyu%40google.com