Re: [PATCHv5 bpf-next 02/28] ftrace: Add ftrace_hash_remove function

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]

> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -416,6 +416,7 @@ void free_ftrace_hash(struct ftrace_hash *hash);
>  struct ftrace_func_entry *add_ftrace_hash_entry_direct(struct ftrace_hash *hash,
>  						       unsigned long ip, unsigned long direct);
>  unsigned long ftrace_hash_count(struct ftrace_hash *hash);
> +void ftrace_hash_remove(struct ftrace_hash *hash);
>
>  /* The hash used to know what functions callbacks trace */
>  struct ftrace_ops_hash {
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1249,6 +1249,25 @@ remove_hash_entry(struct ftrace_hash *hash,
>  	hash->count--;
>  }
>
> +void ftrace_hash_remove(struct ftrace_hash *hash)
> +{
> +	struct hlist_head *hhd;
> +	struct hlist_node *tn;
> +	struct ftrace_func_entry *entry;
> +	int size = 1 << hash->size_bits;
                          ^^^^^^^^^^^^^^^^

Can this dereference a NULL pointer? The code computes the size using
hash->size_bits before checking if hash is NULL:

    int size = 1 << hash->size_bits;
    ...
    if (!hash || !hash->count)
        return;

If a caller passes NULL, the function will crash on the size computation.

Looking at how this is used in the later commit e77bc5ff90dd
('bpf: Add bpf_trampoline_multi_attach/detach functions'),
init_tracing_multi_data() in kernel/bpf/trampoline.c allocates three hashes:

    data->reg    = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
    data->unreg  = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
    data->modify = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
    if (!data->reg || !data->unreg || !data->modify) {
        clear_tracing_multi_data(data);
        return -ENOMEM;
    }

On allocation failure, clear_tracing_multi_data() calls
remove_tracing_multi_data(), which calls ftrace_hash_remove() on data->reg,
data->unreg, and data->modify. Any of these might be NULL if the
corresponding alloc_ftrace_hash() failed.

Would it be safer to move the size calculation below the NULL check?

> +	int i;
> +
> +	if (!hash || !hash->count)
> +		return;
> +
> +	for (i = 0; i < size; i++) {
> +		hhd = &hash->buckets[i];
> +		hlist_for_each_entry_safe(entry, tn, hhd, hlist)
> +			remove_hash_entry(hash, entry);
> +	}
> +	FTRACE_WARN_ON(hash->count);
> +}
> +
>  static void ftrace_hash_clear(struct ftrace_hash *hash)
>  {
>  	struct hlist_head *hhd;


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24598000047