> On Sep 12, 2025, at 03:32, Kees Cook <k...@kernel.org> wrote:
>
> On Thu, Sep 11, 2025 at 03:04:01PM +0000, Qing Zhao wrote:
>>
>>
>>> On Sep 10, 2025, at 23:05, Kees Cook <k...@kernel.org> wrote:
>>>
>>> On Tue, Sep 09, 2025 at 06:49:22PM +0000, Qing Zhao wrote:
>>>>
>>>>> On Sep 4, 2025, at 20:24, Kees Cook <k...@kernel.org> wrote:
>>>>> +For indirect call sites:
>>>>> +
>>>>> +- Keeping indirect calls from being merged (see above) by adding a
>>>>> + wrapping type so that equality was tested based on type-id.
>>>>
>>>> I still think that the additional new created wrapping type and the new
>>>> assignment stmt
>>>>
>>>> wrapper_tmp = (wrapper_ptr_type) fn
>>>> is not necessary.
>>>>
>>>> All the information can be get from function type + type-id which is
>>>> attached as an attribute
>>>> to the original_function_type of “fn”.
>>>> Could you explain why the wrapper type and the new temporary, new
>>>> assignment is
>>>> necessary?
>>>
>>> I couldn't find a way to stop merging just using the attributes. I need
>>> a way to directly associated indirect call sites with the typeid.
>>>
>> When determining whether two callsites should be merged, is it feasible to
>> adding the different type_id from the
>> attributes into consideration?
>
> This is basically what was happening in the RFC, but I kept finding new
> corner cases in various passes, so it felt like whack-a-mole. Using the
> wrapper appeared to solve it across the board with no special casing.
Okay, if this is the case, I think that it’s better to explain the reason in
the design doc on why you finally
decide to add the wrapper function instead of directly using the type_id from
the attribute for comparison.
What’s the issues when directly using the type_id from the attribute, why only
the new wrapper type and
function works?
>
>>>> Why the type-id attached as the attribute is not enough?
>>>
>>> Doing the wrapping avoided needing to update multiple optimization passes
>>> to check for the attribute.
Do you remember which optimization passes need to be updated for these purpose?
>>> And it still needed a way to distinguish
>>> between direct and indirect calls, so I need to wrap only the indirect
>>> calls, where as the typeid attribute is for all functions for all typeid
>>> needs, like preamble generation, etc.
>>
>> Okay, this sounds like a reasonable justification for these additional
>> temporaries
>> and assignment stmts.
>> One more question, are these additional temporaries and assignment stmts are
>> finally eliminated by later optimizations? Any runtime overhead due to them?
>
> Yeah, they totally vanish as far as I've been able to determine.
That’s good. Then you might add this too in the design doc as a justification
of the
New wrapper type, temporaries and new assignment stmt.
thanks.
Qing
>
>>>>> +/* Check if a function needs KCFI preamble generation.
>>>>> + ALL functions get preambles when -fsanitize=kcfi is enabled,
>>>>> regardless
>>>>> + of no_sanitize("kcfi") attribute. */
>>>>
>>>> Why no_sanitize(“kcfi”) is not considered here?
>>>
>>> no_sanitize(“kcfi”) is strictly about whether call-site checking
>>> is performed within the function. It is not used to mark a function as
>>> not being the target of a KCFI call.
>>
>> Okay, is this documented somewhere?
>
> Ah, whoops, no. I have added a note to the "no_sanitize" function attribute
> docs for v3.
>
>>> What is the right tool for me to run to check for these kinds of code
>>> style glitches? contrib/check_GNU_style.py doesn't report anything. Oh!
>>> It takes _patches_ not _files_. The .sh version specifies "patch" in the
>>> help usage. Okay, I will get this all passing cleanly.
>>
>> Yeah, I usually use contrib/check_GNU_style.py to cleanup the code before
>> submitting the patch.
>
> Thanks!
>
>>>>> +/* Wrap indirect calls with KCFI type for anti-merging. */
>>>>> +static unsigned int
>>>>> +kcfi_instrument (void)
>>>>> +{
>>>>> + /* Process current function for call instrumentation only.
>>>>> + Type ID setting is handled by the separate IPA pass. */
>>>>> +
>>>>> + basic_block bb;
>>>>> +
>>>>> + FOR_EACH_BB_FN (bb, cfun)
>>>>> + {
>>>>> + gimple_stmt_iterator gsi;
>>>>> + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>>>> + {
>>>>> + gimple *stmt = gsi_stmt (gsi);
>>>>> +
>>>>> + if (!is_gimple_call (stmt))
>>>>> + continue;
>>>>> +
>>>>> + gcall *call_stmt = as_a <gcall *> (stmt);
>>>>> +
>>>>> + // Skip internal calls - we only instrument indirect calls
>>>>> + if (gimple_call_internal_p (call_stmt))
>>>>> + continue;
>>>>> +
>>>>> + tree fndecl = gimple_call_fndecl (call_stmt);
>>>>> +
>>>>> + // Only process indirect calls (no fndecl)
>>>>> + if (fndecl)
>>>>> + continue;
>>>>> +
>>>>> + tree fn = gimple_call_fn (call_stmt);
>>>>> + if (!is_kcfi_indirect_call (fn))
>>>>> + continue;
>>>>> +
>>>>> + // Get the function type to compute KCFI type ID
>>>>> + tree fn_type = gimple_call_fntype (call_stmt);
>>>>> + gcc_assert (fn_type);
>>>>> + if (TREE_CODE (fn_type) != FUNCTION_TYPE)
>>>>> + continue;
>>>>> +
>>>>> + uint32_t type_id = compute_kcfi_type_id (fn_type);
>>>>> +
>>>>> + // Create KCFI wrapper type for this call
>>>>> + tree wrapper_type = create_kcfi_wrapper_type (fn_type, type_id);
>>>> Again, the new “type_id” has been attached as an attribute of “fn_type”
>>>> here,
>>>
>>> The attribute is attached during IPA. This is run before that, but as I
>>> mentioned, this is the call-site handling, and the IPA pass is for
>>> globally associating a type-id to the function for all other uses
>>> (preambles, weak symbols, etc).
>> During IPA, the typeid is attached to the function type through
>> “set_function_kcfi_type_id” for
>> each function in the callgraph.
>>
>> For each indirect callsite in the above, the routine
>> “create_kcfi_wrapper_type” also attaches
>> the typeid to the original_fn_type, and at the same time, create a new
>> wrapper_type with a type_name
>> embedding the typeid.
>>
>> So, I feel the type_id information is carried redundantly here.
>
> Ah! Yes, sorry, I see what you mean: the tail portion of
> create_kcfi_wrapper_type! Yeah, this is kind of a oversight from
> switching to the IPA pass. I was attaching the typeid to DECLs in IPA
> and TYPEs in GIMPLE (create_kcfi_wrapper_type). The DECLs were used for
> preambles, and the TYPEs were used for RTL expansion. I will attempt to
> merge these; they should all be on the TYPE.
>
>>> I'm open to whatever alternative is needed here. I tried to capture the
>>> merging issue with gcc/testsuite/gcc.dg/kcfi/kcfi-call-sharing.c
>>
>> Might need to study a little bit here to see whether better solution is
>> possible without
>> These additional temporizes and stmts.
>
> Thanks!
>
> --
> Kees Cook
