Re: [PATCH bpf-next 1/3] bpf/bpf_get,set_sockopt: add option to set TCP-BPF sock ops flags

[Alan Maguire <alan.maguire@oracle.com>]

On 06/08/2024 20:54, Martin KaFai Lau wrote:
> On 8/2/24 8:29 AM, Alan Maguire wrote:
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 78a6f746ea0b..570ca3f12175 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -5278,6 +5278,11 @@ static int bpf_sol_tcp_setsockopt(struct sock
>> *sk, int optname,
>>               return -EINVAL;
>>           inet_csk(sk)->icsk_rto_min = timeout;
>>           break;
>> +    case TCP_BPF_SOCK_OPS_CB_FLAGS:
>> +        if (val & ~(BPF_SOCK_OPS_ALL_CB_FLAGS))
>> +            return -EINVAL;
>> +        tp->bpf_sock_ops_cb_flags = val;
>> +        break;
>>       default:
>>           return -EINVAL;
>>       }
>> @@ -5366,6 +5371,17 @@ static int sol_tcp_sockopt(struct sock *sk, int
>> optname,
>>           if (*optlen < 1)
>>               return -EINVAL;
>>           break;
>> +    case TCP_BPF_SOCK_OPS_CB_FLAGS:
>> +        if (*optlen != sizeof(int))
>> +            return -EINVAL;
>> +        if (getopt) {
>> +            struct tcp_sock *tp = tcp_sk(sk);
>> +            int val = READ_ONCE(tp->bpf_sock_ops_cb_flags);
> 
> READ_ONCE() here looks suspicious. There is no existing WRITE_ONCE.
> 
> Is it needed? The read here should have already passed the
> sock_owned_by_me test. The existing write side should also have the
> sock_owned_by_me.
> 
>

Yep, you're right. Will remove for v2. Thanks for taking a look!