Re: [PATCH mptcp-next v2 3/5] mptcp: use READ_ONCE() over sysctls

Hi Gang,

On 18/08/2026 15:02, [email protected] wrote:
> August 18, 2026 at 6:01 PM, [email protected] mailto:[email protected]  wrote:
> 
> 
>>
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>
>> Pre-existing issues:
>> - [High] Concurrent sysctl writes or sysctl updates racing with socket creation can leave the socket's `pm_type` and `ops` mismatched, breaking MPTCP path management.
>> --
>>
>> --- Patch [3]: [PATCH mptcp-next v2 3/5] mptcp: use READ_ONCE() over sysctls ---
>> commit 77c9e4aa4cdd5f6c57cc4cab253d46fd0cf2dfb2
>> Author: Matthieu Baerts (NGI0) <[email protected]>
>>
>> mptcp: use READ_ONCE() over sysctls
>>
>> This commit adds READ_ONCE() and WRITE_ONCE() macros around sysctl
>> variables in the MPTCP networking code to avoid KCSAN issues. It also
>> ensures the pm_type is written with WRITE_ONCE() in proc_path_manager().
>>
>> Closes: https://lore.kernel.org/CANn89iL=os-60kDKqMDdyiXuPF5CG=eejS0vmthwpDGXz_Bp8A@mail.gmail.com
>>
>>>
>>> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
>>>  index c0481b09c1a17..733706f06f1b1 100644
>>>  --- a/net/mptcp/ctrl.c
>>>  +++ b/net/mptcp/ctrl.c
>>>
>> [ ... ]
>>
>>>
>>> @@ -251,7 +251,7 @@ static int proc_path_manager(const struct ctl_table *ctl, int write,
>>>  pm_type = MPTCP_PM_TYPE_KERNEL;
>>>  else if (strncmp(pm_name, "userspace", MPTCP_PM_NAME_MAX) == 0)
>>>  pm_type = MPTCP_PM_TYPE_USERSPACE;
>>>  - pernet->pm_type = pm_type;
>>>  + WRITE_ONCE(pernet->pm_type, pm_type);
>>>
>> [Severity: High]
>> This is a pre-existing issue, but can this sequential update race with socket
>> creation and leave the socket's pm_type and ops mismatched?
>>
>> If an administrator writes to the sysctl while a new socket is being created,
>> could mptcp_pm_data_reset() in net/mptcp/pm.c interleave like this:
>>
>> CPU1 (sysctl write)
>> proc_path_manager()
>>  mptcp_set_path_manager(pernet, pm_name); // Updates pernet->path_manager
>>
>> CPU2 (socket creation)
>> mptcp_pm_data_reset()
>>  pm_type = mptcp_get_pm_type(net); // Reads old pm_type
>>  ...
>>  mptcp_get_path_manager(net, pm_name); // Reads new path_manager
>>
>> CPU1 (sysctl write resumes)
>> proc_path_manager()
>>  WRITE_ONCE(pernet->pm_type, pm_type); // Updates pernet->pm_type
>>
>> Would this leave the new socket corrupted with a contradictory path manager
>> state, breaking MPTCP path management logic?
> 
> Hi,
> 
> I think that may happen. Maybe need anothor patch to fix this.
> 
> When I have time, I'll look this.
Indeed, that's for another patch, probably a fix for -net.

But it feels like you would need a lock to handle that properly. If it
starts to be too complex, we could also say that the net.mptcp.pm_type
sysctl knob is deprecated, and that's a known issue. In this case, it
might be good to add a comment above this WRITE_ONCE(). Then, this could
be done in a dedicated patch I think to explain why. This "mptcp: use
READ_ONCE() over sysctls" would then be only about ... READ_ONCE() :)

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