Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Matthieu Baerts <matttbe@kernel.org>]

On 04/09/2024 12:09, Geliang Tang wrote:
> On Tue, 2024-09-03 at 11:11 +0200, Matthieu Baerts wrote:
>> On 03/09/2024 10:04, Geliang Tang wrote:

(...)

>>> +SEC("cgroup/getsockopt")
>>> +int _getsockopt_subflow(struct bpf_sockopt *ctx)
>>> +{
>>> +	struct bpf_sock *sk = ctx->sk;
>>> +
>>> +	if (!sk) {
>>> +		bpf_printk("MPTCP Subflow: unexpected socket, sk
>>> is NULL");
>>> +		ctx->retval = -1;
>>> +		return 1;
>>> +	}
>>> +
>>> +	if (ctx->level == SOL_SOCKET && ctx->optname == SO_MARK)
>>> +		ctx->retval = _check_getsockopt_subflow_mark(sk);
>>> +	else if (ctx->level == SOL_TCP && ctx->optname ==
>>> TCP_CONGESTION)
>>> +		ctx->retval = _check_getsockopt_subflow_cc(sk);
>>
>> As I mentioned in my previous reply: can ctx->retval be < 0 here,
>> before
>> calling _check_getsockopt_subflow_XXX()?
>> If yes or not sure, maybe safer to do:
>>
>>   err = _check_getsockopt_subflow_XXX()
>>   (...)
>>
>>   if (err)
>>       ctx->retval = -1;
>     else
>         ctx->retval = 0;
> 
> Otherwise:
> 
> 86: (63) *(u32 *)(r6 +0) = r1
> dereference of modified ctx ptr R6 off=36 disallowed
> processed 169 insns (limit 1000000) max_states_per_insn 2 total_states
> 15 peak_states 15 mark_read 4
> -- END PROG LOAD LOG --

Sorry, I'm not familiar with these errors: do you mean that 'retval'
always need to be set? But in a previous version you sent, 'retval' was
only set to -1 in case of error, no?

The error message doesn't seem to say that.
Are you sure it is not because 'err' was not initialised, or something
like that?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.