Re: [PATCH net 2/2] selftests: ptp: add a regression test for the frequency adjustment overflow

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260712040922.6403-3-deepshah146@gmail.com?part=2
---
--- Patch [2]: [PATCH net 2/2] selftests: ptp: add a regression test for the frequency adjustment overflow ---
commit be5629a2612f961645c213deea88db61fa06f083
Author: Deep Shah <deepshah146@gmail.com>

selftests: ptp: add a regression test for the frequency adjustment overflow
    
This commit adds a regression test that crafts a struct timex.freq directly
to verify if an overflowing frequency adjustment is correctly rejected by
ptp_clock_adjtime() with -ERANGE.

> diff --git a/tools/testing/selftests/ptp/ptp_freq_overflow.c b/tools/testing/selftests/ptp/ptp_freq_overflow.c
> new file mode 100644
> index 0000000000000..342b22689f679
> --- /dev/null
> +++ b/tools/testing/selftests/ptp/ptp_freq_overflow.c
[ ... ]
> +
> +#define FD_TO_CLOCKID(fd)	((~(clockid_t)(fd) << 3) | 3)

[Severity: Medium]
Does this macro left-shift a negative signed integer?

Casting fd (a positive integer) to clockid_t (a signed 32-bit int), applying
bitwise NOT, and left-shifting it by 3 yields a negative integer. Left-shifting
a negative signed integer invokes undefined behavior in ISO C, which will
trigger a runtime error if tests are run with Undefined Behavior Sanitizer
(UBSan) enabled.

Could the inverted value be cast to an unsigned integer prior to the shift,
similar to how it is handled in testptp.c?

[ ... ]
> +	/*
> +	 * (1 + 147573952589676412) * 125 == 2^64 + 9, which overflows s64 in
> +	 * scaled_ppm_to_ppb() and wraps the result to a ppb of 0.  A kernel
> +	 * that does not detect the overflow lets this absurd frequency past
> +	 * the max_adj check; a fixed kernel rejects it with -ERANGE.
> +	 */
> +	tx.modes = ADJ_FREQUENCY;
> +	tx.freq = 147573952589676412LL;

[Severity: Low]
Will this assignment cause a -Woverflow compiler warning on 32-bit
architectures?

On 32-bit platforms where tx.freq is a 32-bit long (without _TIME_BITS=64),
assigning this 64-bit literal exceeds the maximum value of the signed integer
type. Although a runtime check prevents execution, the compiler still parses
the assignment and emits a -Woverflow warning during the build.

> +
> +	ret = clock_adjtime(clkid, &tx);

[Severity: Medium]
Does this test permanently disrupt the hardware clock frequency on vulnerable
kernels without restoring it?

If the kernel does not have the overflow fix, clock_adjtime() will bypass
the max_adj check and push the corrupted frequency downstream to the driver
via ops->adjfine(). Because the test exits immediately after verifying the
error code, the original frequency is never restored, leaving time
synchronization corrupted until a reboot or manual reset.

Should the test read the initial dialed_frequency and restore it at the
end of the test?

> +	ksft_test_result(ret < 0 && errno == ERANGE,
> +			 "overflowing frequency adjustment is rejected (ret=%d errno=%d)\n",
> +			 ret, ret < 0 ? errno : 0);

[Severity: Medium]
Will this test incorrectly fail if the PTP clock is in free-running mode?

If virtual clocks are active on the selected PTP device, ptp_clock_freerun()
returns true and the kernel explicitly returns -EBUSY before evaluating the
frequency overflow logic. Since the test expects -ERANGE, receiving -EBUSY
will cause the KSFT assertion to fail.

Should -EBUSY be accounted for as a valid condition to gracefully skip
the test?

> +
> +	close(fd);
> +	ksft_finished();
> +}