Re: [PATCH mptcp-next v2 1/5] mptcp: sched: change scheduler sysctl atomically

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free and module reference leak due to missing reference counting when setting the default MPTCP scheduler
- [Medium] Redundant scheduler list traversal during socket initialization defeats pointer caching
--

--- Patch [1]: [PATCH mptcp-next v2 1/5] mptcp: sched: change scheduler sysctl atomically ---
commit d5b1780c033b9909a1b30fbd814812af5c25ed2f
Author: Gang Yan <[email protected]>

mptcp: sched: change scheduler sysctl atomically

This commit changes the per-netns scheduler storage from an inline string
to an RCU-protected pointer to an mptcp_sched_ops struct. It updates the
sysctl read/write paths and socket initialization to use this pointer,
aiming to prevent concurrent readers from observing half-written names.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/626

> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index 63c5747f0f63c..479b31eb30079 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
[ ... ]
> @@ -126,7 +133,7 @@ static int mptcp_set_scheduler(char *scheduler, const char *name)
>  	rcu_read_lock();
>  	sched = mptcp_sched_find(name);
>  	if (sched)
> -		strscpy(scheduler, name, MPTCP_SCHED_NAME_MAX);
> +		xchg(&pernet->scheduler, sched);

[Severity: High]
When replacing the scheduler pointer here with xchg(), does this leak the
module reference for the previous scheduler returned by xchg()?

Also, do we need to acquire a module reference for the newly assigned sched
via bpf_try_module_get() before storing it? Without a reference, could the
module be unloaded, leaving pernet->scheduler pointing to freed memory that
would trigger a use-after-free when dereferenced by a concurrent reader in
proc_scheduler()?

[ ... ]
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index c2762d74f29d7..82df0400d0f94 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -3271,8 +3272,8 @@ static int mptcp_init_sock(struct sock *sk)
>  		return -ENOMEM;
>  
>  	rcu_read_lock();
> -	ret = mptcp_init_sched(mptcp_sk(sk),
> -			       mptcp_sched_find(mptcp_get_scheduler(net)));
> +	mptcp_get_scheduler(net, sched_name);
> +	ret = mptcp_init_sched(mptcp_sk(sk), mptcp_sched_find(sched_name));

[Severity: Medium]
Since pernet->scheduler now stores the mptcp_sched_ops pointer directly,
could we retrieve and use that pointer here directly? Extracting the name
via mptcp_get_scheduler() just to perform a redundant list traversal with
mptcp_sched_find() seems to defeat the purpose of caching the operations
pointer.

>  	rcu_read_unlock();
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1