netdev - Re: [PATCH bpf] selftests/bpf: enable mptcp before testing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <19a6a29d-85f3-b8d7-c9d9-3c97a625bd13@tessares.net>
Date:   Mon, 13 Feb 2023 17:28:19 +0100
From:   Matthieu Baerts <matthieu.baerts@...sares.net>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Mykola Lysenko <mykolal@...com>,
        Felix Maurer <fmaurer@...hat.com>,
        Nicolas Rybowski <nicolas.rybowski@...sares.net>,
        Davide Caratti <dcaratti@...hat.com>
Subject: Re: [PATCH bpf] selftests/bpf: enable mptcp before testing

Hi Hangbin Liu,

On 13/02/2023 05:10, Hangbin Liu wrote:
> On Fri, Feb 10, 2023 at 05:22:24PM +0100, Matthieu Baerts wrote:
>> Hi Hangbin Liu,
>>
>> On 10/02/2023 10:32, Hangbin Liu wrote:
>>> Some distros may not enable mptcp by default. Enable it before start the
>>> mptcp server. To use the {read/write}_int_sysctl() functions, I moved
>>> them to test_progs.c
>>>
>>> Fixes: 8039d353217c ("selftests/bpf: Add MPTCP test base")
>>> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
>>> ---
>>>  .../testing/selftests/bpf/prog_tests/mptcp.c  | 15 ++++++-
>>
>> Thank you for the patch!
>>
>> The modifications linked to MPTCP look good to me.
>>
>> But I don't think it is needed here: I maybe didn't look properly at
>> 'bpf/test_progs.c' file but I think each program from 'prog_tests'
>> directory is executed in a dedicated netns, no?
>>
>> I don't have an environment ready to validate that but if yes, it means
>> that on a "vanilla" kernel, net.mptcp.enabled sysctl knob should be set
>> to 1. In this case, this modification would be specific to these distros
>> patching MPTCP code to disable it by default. It might then be better to
>> add this patch next to the one disabling MPTCP by default, no? (or
>> revert it to have MPTCP available by default for the applications asking
>> for it :) )
> 
> I think this issue looks like the rp_filter setting. The default rp_filter is 0.
> But many distros set it to 1 for safety reason. Thus there are some fixes for
> the rp_filter setting like this one. e.g.

I think we should not compare net.mptcp.enabled with rp_filter because
the latter is a bit particular: the initial value of rp_filter in a
newly created netns is inherited from the "host" (the initial netns). In
this case, it is difficult to predict the value of rp_filter in a new
netns and it makes sense to force it to the expected value.

Here for net.mptcp.enabled, the value should be 1 in a newly created
netns. If not, it means the kernel has been modified. (It was making
sense to disable it when MPTCP was backported to older kernels and
marked as "Tech Preview". Now, I don't think we should recommend distros
to do that and support such modifications in the upstream kernel :) )

But again, I'm not totally against that, I'm just saying that if these
tests are executed in dedicated netns, this modification is not needed
when using a vanilla kernel ;-)

Except if I misunderstood and these tests are not executed in dedicated
netns?

(Note: to reduce the size of the patch, if these tests are correctly
executed in dedicated netns, then you can simply force net.mptcp.enabled
to be set to 1, no need to reset it to the previous value.)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net