Re: [PATCH v5 2/5] refcount: add refcount_final_put tracepoint

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