Re: [PATCH bpf v3] bpf: fix: Race condition in bpf_trampoline_unlink_cgroup_shim

[Martin KaFai Lau <martin.lau@linux.dev>]

On 2/27/26 7:04 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
>> --- a/kernel/bpf/trampoline.c
>> +++ b/kernel/bpf/trampoline.c
> 
> [ ... ]
> 
>> @@ -1003,12 +1004,13 @@ struct bpf_attach_target_info tgt_info = {};
>>
>>   	shim_link = cgroup_shim_find(tr, bpf_func);
>>   	if (shim_link) {
>> -		/* Reusing existing shim attached by the other program. */
>> -		bpf_link_inc(&shim_link->link.link);
>> -
>> -		mutex_unlock(&tr->mutex);
>> -		bpf_trampoline_put(tr); /* bpf_trampoline_get above */
>> -		return 0;
>> +		link = &shim_link->link.link;
>> +		if (link == bpf_link_inc_not_zero(link)) {
> 
> The other callers of bpf_link_inc_not_zero() check the return value
> using IS_ERR(), for example bpf_link_by_id():
> 
>      link = bpf_link_inc_not_zero(link);
> 
> and bpf_link_get_curr_or_next():
> 
>      link = bpf_link_inc_not_zero(link);
>      if (IS_ERR(link)) {
> 
> Should this use the same IS_ERR() pattern for consistency? Something
> like:
> 
>      if (!IS_ERR(bpf_link_inc_not_zero(link))) {

The same feedback by ai-review. I don't have a strong opinion. However, 
the newly added "struct bpf_link *link" variable is not needed also. I 
would carry the ai's suggestion further and remove the new local 
variable also. Just test:

	if (!IS_ERR(bpf_link_inc_not_zero(&shim_link->link.link))) {

> 
>> +			/* Reusing existing shim attached by the other program. */
>> +			mutex_unlock(&tr->mutex);
>> +			bpf_trampoline_put(tr); /* bpf_trampoline_get above */
>> +			return 0;
>> +		}
>>   	}
> 
> This commit fixes a use-after-free introduced by commit 69fd337a975c
> ("bpf: per-cgroup lsm flavor"), which added the bpf_link_inc() call
> that could resurrect a zero-refcount link. It appears to be missing
> a Fixes: tag. Martin KaFai Lau's review of v2 also expected this tag
> to be present.

Indeed, please ensure to address the earlier reviewer's feedback.

pw-bot: cr

> 
> Suggested:
> 
>      Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
> 
> https://lore.kernel.org/bpf/cd99eba7-ac03-4b53-a79c-a6206abbddb6@linux.dev/
> 
> 
> ---
> 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/22511586078
> 
> AI-authorship-score: low
> AI-authorship-explanation: Human authorship patterns: references maintainer suggestion, informal testing description, iterative v1/v2/v3 development with reviewer feedback, addresses a bug reported by a different researcher.
> issues-found: 2
> issue-severity-score: low
> issue-severity-explanation: Missing Fixes: tag (requested by BPF maintainer) and non-standard bpf_link_inc_not_zero() return value comparison pattern; neither causes runtime problems.