On Thu, Nov 20, 2025 at 10:23:46PM +0000, [email protected] wrote:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 9cf4cd56d..c571deeff 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -551,6 +551,7 @@ int modify_ftrace_direct(struct ftrace_ops *ops,
> > unsigned long addr);
> > int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long
> > addr);
> >
> > int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash
> > *hash);
> > +int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash
> > *hash);
> >
> > void ftrace_stub_direct_tramp(void);
> >
> > @@ -583,6 +584,11 @@ static inline int update_ftrace_direct_add(struct
> > ftrace_ops *ops, struct ftrace
> > return -ENODEV;
> > }
> >
> > +static inline int update_ftrace_direct_del(struct ftrace_ops *ops, struct
> > ftrace_hash *hash)
> > +{
> > + return -ENODEV;
> > +}
> > +
>
> [ ... ]
>
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 850ff55ff..cc730a8fd 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
>
> [ ... ]
>
> > +/**
> > + * hash_sub - substracts @b from @a and returns the result
> > + * @a: struct ftrace_hash object
> > + * @b: struct ftrace_hash object
> > + *
> > + * Returns struct ftrace_hash object on success, NULL on error.
> > + */
> > +static struct ftrace_hash *hash_sub(struct ftrace_hash *a, struct
> > ftrace_hash *b)
> > +{
> > + struct ftrace_func_entry *entry, *del;
> > + struct ftrace_hash *sub;
> > + int size, i;
> > +
> > + sub = alloc_and_copy_ftrace_hash(a->size_bits, a);
> ^^^^^^^^^^^^
>
> Can hash_sub() handle a NULL hash pointer in parameter a? I notice the
> parallel function hash_add() safely handles NULL via hash_count() and
> alloc_and_copy_ftrace_hash() which checks ftrace_hash_empty().
>
> However, hash_sub() directly dereferences a->size_bits before passing a
> to alloc_and_copy_ftrace_hash(). This will crash if a is NULL.
>
> This matters because update_ftrace_direct_del() calls hash_sub() with
> old_filter_hash which can be NULL:
>
> old_filter_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
> ...
> new_filter_hash = hash_sub(old_filter_hash, hash);
>
> The existing update_ftrace_direct_add() has the same defensive check and
> passes the potentially-NULL old_filter_hash to hash_add(), which handles
> it correctly.
so we never call update_ftrace_direct_del without having
update_ftrace_direct_add called, which ensures the existence of filter_hash,
but it's probably good idea to do explicit check for future
thanks,
jirka
>
> > + if (!sub)
> > + goto error;
> > +
> > + size = 1 << b->size_bits;
> > + for (i = 0; i < size; i++) {
> > + hlist_for_each_entry(entry, &b->buckets[i], hlist) {
> > + del = __ftrace_lookup_ip(sub, entry->ip);
> > + if (WARN_ON_ONCE(!del))
> > + goto error;
> > + remove_hash_entry(sub, del);
> > + kfree(del);
> > + }
> > + }
> > + return sub;
> > +
> > + error:
> > + free_ftrace_hash(sub);
> > + return NULL;
> > +}
> > +
> > +int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash
> > *hash)
> > +{
>
> [ ... ]
>
>
> ---
> 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/19552032346
