On Mon, Jun 17 2024 at 16:06, Kees Cook wrote:
> On Tue, Jun 18, 2024 at 12:13:27AM +0200, Thomas Gleixner wrote:
>> In fact is_valid_bugaddr() should be globally fixed up to return bool to
>> match what the function name suggests.
>>
>> The UD type information is x86 specific and has zero business in a
>> generic architecture agnostic function return value.
>>
>> It's a sad state of affairs that I have to explain this to people who
>> care about code correctness. Readability and consistency are substantial
>> parts of correctness, really.
>
> Well, it's trade-offs. If get_ud_type() is in is_valid_bugaddr(), we
> have to call it _again_ outside of is_valid_bugaddr(). That's suboptimal
> as well. I was trying to find a reasonable way to avoid refactoring all
> architectures and to avoid code code.
There is not much of a trade-off. This is not the context switch hot
path, right?
Aside of that what is wrong with refactoring? If something does not fit
or the name does not make sense anymore then refactoring is the right
thing to do, no? It's not rocket science and just a little bit more work
but benefitial at the end.
> Looking at it all again, I actually think arch/x86/kernel/traps.c
> shouldn't call is_valid_bugaddr() at all. That usage can continue to
> stay in lib/bug.c, which is only ever used by x86 during very early
> boot, according to the comments in early_fixup_exception(). So just a
> direct replacement of is_valid_bugaddr() with the proposed get_ud_type()
> should be fine in arch/x86/kernel/traps.c.
I haven't looked at the details, but if that's the case then there is
even less of a reason to abuse is_valid_bugaddr().
That said it would still be sensible to convert is_valid_bugaddr() to a
boolean return value :)
Thanks,
tglx
