Re: [PATCH net 1/2] netfilter: conntrack: avoid secs-to-jiffies overflow in SIP helper

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The Fixes: tag gives the wrong origin for the failure the commit
  message mainly describes, which matters for backporting.
- [Low] This is a bug fix under net/, which MAINTAINERS 'NETWORKING
  [GENERAL]' covers with 'P: Documentation/process/maintainer-…

--

netfilter: conntrack: avoid secs-to-jiffies overflow in SIP helper

This adds nf_ct_secs_to_jiffies(), which multiplies in u64 and saturates
at INT_MAX. refresh_signalling_expectation() now uses it, so a large
expiry in a SIP REGISTER 2xx response can no longer wrap the u32
exp->timeout or make the expectation read as already expired.

> Expectations are reaped with "(__s32)(exp->timeout - nfct_time_stamp)
> <= 0".  At HZ=1000 an expiry of 2147484 seconds (24.9 days) crosses
> 2^31 and the expectation is born expired, while 4294968 seconds wraps
> to 704 jiffies.  Registration refreshes break, or a signalling
> expectation stays pinned for days.

[ ... ]

> Fixes: 0f32a40fc91a ("[NETFILTER]: nf_conntrack_sip: create signalling expectations")
> Cc: stable@vger.kernel.org

[Severity: Low]
Should this also have a Fixes: tag for b8b09dc2bf35 ("netfilter:
nf_conntrack_expect: use conntrack GC to reap expectations")?

The commit message quotes a signed expiry test. That test is in
nf_ct_exp_is_expired():

    return (__s32)(READ_ONCE(exp->timeout) - nfct_time_stamp) <= 0;

b8b09dc2bf35 added that test. It also made exp->timeout a u32 and added
the line this patch changes in refresh_signalling_expectation():

    WRITE_ONCE(exp->timeout, nfct_time_stamp + (expires * HZ));

0f32a40fc91a used a timer instead:

    exp->timeout.expires = jiffies + expires * HZ;
    add_timer(&exp->timeout);

Later trees changed this to mod_timer_pending(), but still added
jiffies + expires * HZ.

Trees from before b8b09dc2bf35 only have the 2^32 wrap (4294968 seconds
turning into 704 jiffies). On 64-bit, a product between 2^31 and 2^32 is a
positive unsigned long delta, and the timer wheel clamps it. So the
24.9 day "born expired" case does not seem to exist in those trees.

Those older trees also do not have the WRITE_ONCE(exp->timeout, ...)
context, so the stable backport will not apply cleanly there.

Could the tags or the message say which failure mode applies to which
trees?

The h323 patch in this series ("netfilter: h323: avoid secs-to-jiffies
overflow in RAS timeout") has the same pattern. It cites only
f587de0e2feb, but it relies on the same signed expiry test.

> Sweeping all 2^32-1 inputs at HZ=1000, the helper never wraps, never
> yields a value that reads as expired, and matches the old expression
> up to INT_MAX / HZ.

[Severity: Low]
Could the commit message say how this issue was found, and how the fix
was tested beyond the arithmetic sweep of nf_ct_secs_to_jiffies()?

net/netfilter/ is covered by the NETWORKING [GENERAL] entry, which points
to Documentation/process/maintainer-netdev.rst. Its bug fixes section
says:

    All bug fixes must state how the issue was discovered and validated,
    and how the fix was tested.

It also asks whether the fix was tested against the reproducer and the
kernel selftests.

The message says "Registration refreshes break, or a signalling
expectation stays pinned for days". It does not say whether this was
seen with real REGISTER 2xx traffic through the SIP helper, or found by
reading the code. There is also no Reported-by: tag.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104085925.773471-1-l1138897701%40163.com