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

On Thu, Sep 10, 2026 at 6:15 PM [off-list ref] wrote:

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] The sysctl half of this patch is inert in this tree:
  `neigh_proc_dointvec_ms_jiffies_positive()`…

--

neighbour: Enforce min/max to NDTPA_INTERVAL_PROBE_TIME_MS.

The patch replaces the ineffective .min-only policy entry for
NDTPA_INTERVAL_PROBE_TIME_MS with NLA_POLICY_FULL_RANGE(), bounding it to
1..86400000 ms, and adds the same max to the sysctl handler plus the
netlink spec and ip-sysctl documentation.

One question about the sysctl half of the change below.

quoted

diff --git a/Documentation/netlink/specs/rt-neigh.yaml b/Documentation/netlink/specs/rt-neigh.yaml
index 0f46ef3135905..c8e55c98d5649 100644
--- a/Documentation/netlink/specs/rt-neigh.yaml
+++ b/Documentation/netlink/specs/rt-neigh.yaml

@@ -341,6 +341,9 @@ attribute-sets:
       -
         name: interval-probe-time-ms
         type: u64
+        checks:
+          min: 1
+          max: 86400000

 operations:
   enum-model: directional

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 208f46967ee59..b05829e44d8fc 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst

@@ -248,7 +248,7 @@ neigh/default/unres_qlen - INTEGER

 neigh/default/interval_probe_time_ms - INTEGER
      The probe interval for neighbor entries with NTF_MANAGED flag,
-     the min value is 1.
+     the min value is 1, and the max value is 86400000 (1 day).

      Default: 5000

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 49dd7df149ef3..0db78a0dfb516 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,
+};
+

[ ... ]

quoted

@@ -3672,12 +3680,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 proc_dointvec_ms_jiffies_minmax() actually look at extra1/extra2 in
this tree?  The call chain from neigh_proc_dointvec_ms_jiffies_positive()
seems to drop the range check:

kernel/time/jiffies.c:

  static int do_proc_int_conv_ms_jiffies_minmax(bool *negp, ulong *u_ptr,
                                                int *k_ptr, int dir,
                                                const struct ctl_table *tbl)
  {
        return proc_int_conv(negp, u_ptr, k_ptr, dir, tbl, false,
                             sysctl_u2k_int_conv_ms, sysctl_k2u_int_conv_ms);
  }

kernel/sysctl.c:proc_int_conv() only consults the bounds when
k_ptr_range_check is true:

        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);

Since the converter passes false, aren't both tmp.extra1 = &min and the new
tmp.extra2 = &max dead stores, so a write of 0, of a value well above
86400000, or of a negative value to
/proc/sys/net/ipv{4,6}/neigh/*/interval_probe_time_ms still succeeds?

If so, the failure mode described in the commit message stays reachable via
the sysctl entry point, where neigh_managed_work() requeues itself with the
unvalidated value:

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

with 0 meaning no delay and a negative int sign-extending into the unsigned
long delay argument.

That would also make this sentence in the commit message inaccurate:

    The same max check is applied to sysctl as well.

and likewise the new documentation text:

        the min value is 1, and the max value is 86400000 (1 day).

The netlink half looks fine, so the sysctl path and the range now declared
in Documentation/netlink/specs/rt-neigh.yaml would disagree: an out-of-range
sysctl write can still be reported back by RTM_GETNEIGHTBL as a value
outside the declared 1..86400000 range.

The cover note below the --- line points at a separate, not-yet-merged
sysctl-core fix
(https://lore.kernel.org/linux-fsdevel/20260905233819.1064529-2-kuniyu@google.com/ (local))
as a prerequisite.  Since that note does not end up in git history,

Good to know Sashiko reads comments under --- :)

The fix was already queued for sysctl-fixes, no need to wait.
https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/sysctl-fixes&id=1bb4e0f7a019b6b34228efbcc9c7399e3dbf72a8

would it
make sense to order this patch behind that fix, or to mention the dependency
in the commit message itself?  Note this commit carries a Fixes: tag, so a
stable backport would carry the documented bound without the enforcement.

I checked the end of this series and do_proc_int_conv_ms_jiffies_minmax()
still passes false, so nothing later in the series appears to restore the
check.

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