Re: [PATCH net 2/2] mptcp: avoid wrapping blackhole timeout

[gang.yan@linux.dev]

> 
> mptcp_active_should_disable() calculates the active-disable interval as
> multiplier * blackhole_timeout * HZ. Since all operands are 32-bit, a
> large blackhole_timeout can wrap the delay even on 64-bit kernels and
> make active MPTCP available again while the blackhole is still present.
> 
> For example, with HZ=1000, a timeout of 6912213 seconds and the maximum
> multiplier of 64 should produce 442381632000 jiffies, but the current
> calculation truncates it to 512 jiffies.
> 
> This issue was found using a static analysis tool. A loopback reproducer
> induced 12 MPTCP blackholes and waited 1.2 seconds before opening another
> connection. The unpatched kernel retried MP_CAPABLE, while the patched
> kernel kept MPTCP disabled and used TCP fallback directly. The reproducer
> required neither kernel modifications nor fault injection.
> 
> Compute the interval in u64 and cap it at MAX_JIFFY_OFFSET before adding
> it to the saved timestamp. This preserves the existing exponential
> backoff for representable intervals and prevents large values from
> wrapping into short timeouts.
> 
> Fixes: 27069e7cb3d1 ("mptcp: disable active MPTCP in case of blackhole")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yingjie Wang <x-professor@outlook.com>
> ---
>  net/mptcp/ctrl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index 63c5747f0f63..5224cfb3ed1f 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
> @@ -487,6 +487,7 @@ bool mptcp_active_should_disable(struct sock *ssk)
>  unsigned long timeout;
>  int disable_times;
>  int multiplier;
> + u64 delay;
>  
>  pernet = mptcp_get_pernet(net);
>  blackhole_timeout = READ_ONCE(pernet->blackhole_timeout);
> @@ -503,10 +504,11 @@ bool mptcp_active_should_disable(struct sock *ssk)
>  
>  /* Limit timeout to max: 2^6 * initial timeout */
>  multiplier = 1 << min(disable_times - 1, 6);
> + delay = (u64)multiplier * blackhole_timeout * HZ;
>  

Hi Yingjie,

Thanks for the patch. Basically LGTM, 


Just two small questions:

>  /* Paired with the WRITE_ONCE() in mptcp_active_disable(). */
>  timeout = READ_ONCE(pernet->active_disable_stamp) +
> - multiplier * blackhole_timeout * HZ;
> + min(delay, MAX_JIFFY_OFFSET);

1. AFAICS, on 64bit machine, the largest possible delay is 64 * INT_MAX * HZ, far
less than MAX_JIFFY_OFFSET ((LONG_MAX >> 1) - 1 = 2^62 - 2). So is the min() intended for the
32-bit machine? 

2. If so, should the comment be updated? Because there is a second timeout on 32-bit machine.


Thanks
Gang

Reviewed-by: Gang Yan <yangang@kylinos.cn>

>  
>  return time_before(jiffies, timeout);
>  }
> -- 
> 2.43.0
>