Re: [PATCH bpf v2 1/2] bpf: Extract shared reqsk-to-listener upgrade

On 8/3/26 11:44, [email protected] wrote:
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 11bb0d236822..fede810ef37f 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -7079,6 +7079,28 @@ __bpf_skc_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>>  	return sk;
>>  }
>>
>> +static struct sock *
>> +bpf_sk_lookup_full_sk(struct sock *sk)
>> +{
> 
> This isn't a bug, but since this helper only converts a reqsk to its
> listener rather than performing a lookup, would a name outside the
> bpf_sk_lookup_* family - say sk_lookup_full_sk() or
> bpf_reqsk_to_listener() - read more clearly next to
> bpf_sk_lookup_tcp()/bpf_sk_lookup_udp() and struct bpf_sk_lookup?
> 
> The bpf_sk_lookup_ prefix in this file already denotes the BPF helper family
> (bpf_sk_lookup_tcp(), bpf_sk_lookup_udp(), bpf_sk_lookup_assign(),
> bpf_sk_lookup_enabled) and the UAPI context type struct bpf_sk_lookup /
> struct bpf_sk_lookup_kern. Sitting about 20 lines above bpf_sk_lookup()
> itself, the name reads like another lookup entry point rather than a
> reqsk-to-listener conversion.

I've meant it to be a bpf-lookup helper, not a bpf lookup-helper. So yeah,
I'm happy to change it to something more appropriate. sk_lookup_full_sk() then?

>> +	struct sock *sk2 = sk_to_full_sk(sk);
>> +
>> +	/*
>> +	 * sk_to_full_sk() may return sk->rsk_listener, make sure the original
>> +	 * sk sock refcnt is decremented to prevent a request_sock leak.
>> +	 */
>> +	if (sk2 != sk) {
>> +		sock_gen_put(sk);
>> +		/* Ensure there is no need to bump sk2 refcnt. */
>> +		if (unlikely(sk2 && !sock_flag(sk2, SOCK_RCU_FREE))) {
>> +			WARN_ONCE(1, "Found non-RCU, unreferenced socket!");
>> +			return NULL;
>> +		}
>> +		sk = sk2;
>> +	}
>> +
>> +	return sk;
>> +}
> 
> [ ... ]