Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Exporting the trace wrapper via GPL-only symbols inside fundamental static inline refcount primitives breaks the ability of non-GPL modules to load. - [Medium] The `caller` and `ip` fields recorded by the tracepoint yield fundamentally inconsistent addresses depending on whether the calling function is evaluated as inline or out-of-line. --
--- Patch [2]: [PATCH v5 2/5] refcount: add refcount_final_put tracepoint --- commit 89e648b43dd0e3f81f4dd1bfaaa093ab2953cba7 Author: Eugene Mavick <[email protected]> refcount: add refcount_final_put tracepoint Add the refcount_final_put tracepoint to __refcount_sub_and_test() and refcount_dec_if_one() refcount_final_put fires when a reference count reaches zero and the object enters its final release path. > diff --git a/include/linux/refcount.h b/include/linux/refcount.h > index ba7657ced2817..23291a31d1be2 100644 > --- a/include/linux/refcount.h > +++ b/include/linux/refcount.h [ ... ] > @@ -393,6 +394,7 @@ bool __refcount_sub_and_test(int i, refcount_t *r, int > *oldp) > > if (old > 0 && old == i) { > smp_acquire__after_ctrl_dep(); > + do_trace_refcount_final_put(r); [Severity: High] Does this break out-of-tree non-GPL modules? Since __refcount_sub_and_test() is a fundamental inline primitive, embedding the tracepoint macro here forces callers to resolve the GPL-only tracepoint symbols. This will cause non-GPL modules to fail to load when they include <linux/refcount.h> and call this function. [Severity: Medium] Will this record inconsistent instruction pointers depending on whether the function is evaluated inline? Because __refcount_sub_and_test() is static inline, _RET_IP_ will evaluate to the return address of the module invoking the refcount API (the caller's caller), while _THIS_IP_ evaluates to the caller. > return true; > } > > diff --git a/lib/refcount.c b/lib/refcount.c > index a207a8f22b3ca..8b148b5765037 100644 > --- a/lib/refcount.c > +++ b/lib/refcount.c [ ... ] > @@ -56,7 +57,10 @@ bool refcount_dec_if_one(refcount_t *r) > { > int val = 1; > > - return atomic_try_cmpxchg_release(&r->refs, &val, 0); > + bool ret = atomic_try_cmpxchg_release(&r->refs, &val, 0); > + > + do_trace_refcount_final_put_cond(ret, r); [Severity: Medium] In this out-of-line context, _RET_IP_ evaluates to the immediate caller. Does this mean the caller and ip fields recorded by the tracepoint will yield fundamentally inconsistent addresses compared to the inline usage in __refcount_sub_and_test()? > + return ret; > } > EXPORT_SYMBOL(refcount_dec_if_one); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
