Re: [PATCH 04/65] x86/hypercall: Annotate fnptr targets

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2021 15:28, Andrew Cooper wrote:
> On 26/11/2021 14:21, Jan Beulich wrote:
>> On 26.11.2021 13:33, Andrew Cooper wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> I understand there's not much to say here, but the title saying just
>> "annotate" without any context as to the purpose of the annotation
>> is too little information imo. I guess this then goes for many more
>> titles in this series.
> 
> I really couldn't think of anything useful to say.  Lots of these
> patches are entirely mechanical.

Them being mechanical is imo unrelated to having a subject which is
halfway meaningful even if one looks at a shortlog in a couple of
years time. All it would take to disambiguate the titles would look
to be "...: Annotate fnptr targets for CET-IBT". Arguably this arch-
specific feature may be a little odd to encounter in common code
logs, but that's still better than being entirely unspecific about
the purpose of the annotations.

>>> --- a/xen/include/xen/hypercall.h
>>> +++ b/xen/include/xen/hypercall.h
>>> @@ -18,12 +18,12 @@
>>>  #include <asm/hypercall.h>
>>>  #include <xsm/xsm.h>
>>>  
>>> -extern long
>>> +extern long cf_check
>>>  do_sched_op(
>>>      int cmd,
>>>      XEN_GUEST_HANDLE_PARAM(void) arg);
>> What purpose does the attribute serve on a declaration? On the surface
>> I would consider it meaningful only on definitions, like e.g. __init.
> 
> Because GCC treats cf_check (and nocf_check) as part of the function
> type.  Simply getting it wrong will yield a "definition doesn't match
> prototype" error.
> 
> Furthermore, it needs to be visible across translation units so one TU
> can spot (and complain at) creating a function pointer to a non-local
> non-endbr'd function.

Hmm, it might well be that way, but that's not what the doc (for 11.2)
says. While suggesting things are along the lines of what you say for
"nocf_check", for "cf_check" it really only talks about code generation:
"The cf_check attribute on a function is used to inform the compiler
that ENDBR instruction should be placed at the function entry when
‘-fcf-protection=branch’ is enabled." And even for "nocf_check" it talks
about extra compile time checks only when the attribute is applied to a
function pointer variable/field, not when applied to a function.

Jan