On 2025-01-24 at 08:23:09 -0800, Dave Hansen wrote:
>On 11/27/24 09:35, Maciej Wieczor-Retman wrote:
>> +static inline int kernel_has_lam(void)
>> +{
>> + unsigned long bits;
>> +
>> + syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);
>> + return !!bits;
>> +}
>
>Generally, I'm less picky about selftest/ code than in-kernel code. But
>people really do take selftest code and use it as a starting point for
>production code.
>
>I'd much rather have overly verbose, obviously correct code:
>
> err = syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);
>
> /* Handle syscall failure, like pre-LAM kernels: */
> if (err)
> return 0
>
> /* Tag bits are empty on non-LAM systems: */
> return !!bits;
>
Sure, more comments is always good :)
>Actually, I was going to argue for that^ just on style and writing good
>code. But then I spotted a bug. What happens if the kernel has
>CONFIG_ADDRESS_MASKING=n, either because it is config'd off or it's old?
>The:
>
> put_user(0, (unsigned long __user *)arg2);
>
>won't ever get run and 'bits' will be uninitialized.
Huh, yeah, you're right. I tested it with both CONFIG_ADDRESS_MASKING=n and =y,
and on systems with it both available and not available but must've been a
coincidence it worked.
I'll fix the checks and init bits for the next version.
>
>So, I think this code was trying to be compact, fast and clever. But it
>really just turns out to be buggy.
>
--
Kind regards
Maciej Wieczór-Retman
