Re: [PATCH net v5] net: pin protocol module before inet socket allocation

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch closes the failed-module-init lifetime race only for
  userspace sockets.
- [Medium] A userspace socket() call can now fail with -EPROTONOSUPPORT
  even though the protocol module is loading successfully.

--

net: pin protocol module before inet socket allocation

This patch takes a temporary module reference on the protocol owner in
inet_create() and inet6_create() before dropping RCU, so a concurrent
module unload cannot destroy the proto slab before sk_alloc() uses it. It
also rejects userspace socket creation while the owner module is still
MODULE_STATE_COMING, and exempts kernel sockets from that check.

> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 14ce01092fda6..5c02022c6c182 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c

[ ... ]

> @@ -322,9 +323,15 @@ static int inet_create(struct net *net, struct socket *sock, int protocol,
>  	    !ns_capable(net->user_ns, CAP_NET_RAW))
>  		goto out_rcu_unlock;
>  
> -	sock->ops = answer->ops;
>  	answer_prot = answer->prot;
> +	answer_owner = answer_prot->owner;
>  	answer_flags = answer->flags;
> +	if ((!kern && answer_owner && module_is_coming(answer_owner)) ||
> +	    !try_module_get(answer_owner)) {
> +		err = -EPROTONOSUPPORT;
> +		goto out_rcu_unlock;
> +	}

[Severity: Medium]
Can a userspace socket() call now fail with -EPROTONOSUPPORT while the
protocol module is loading and will finish successfully?

do_init_module() sets MODULE_STATE_LIVE only after mod->init() returns.
Anything published from inside init is therefore seen as COMING until
then.

Before this patch, the only check was try_module_get(prot->owner) in
sk_prot_alloc(). module_is_live() only rejects MODULE_STATE_GOING, so that
check succeeds for COMING modules and the socket was created.

For SCTP, sctp_init() publishes the protosw early and does more work after
that:

net/sctp/protocol.c:sctp_init() {
    ...
	status = sctp_v4_protosw_init();
    ...
	status = sctp_v6_protosw_init();
    ...
	status = register_pernet_subsys(&sctp_ctrlsock_ops);
    ...
}

register_pernet_subsys() creates a control socket in every netns.
sctp_v4_add_protocol(), sctp_v6_add_protocol() and sctp_offload_init()
come after it. So the window gets longer as the number of network
namespaces grows.

Suppose a daemon calls socket(AF_INET, SOCK_STREAM, IPPROTO_SCTP) while
modules-load.d, or another socket() autoload, is loading sctp. It would
now get EPROTONOSUPPORT, and applications usually read that as "protocol
not supported" and do not retry.

A second concurrent autoloader can hit this too. If its request_module()
returns while the module is still COMING, the retried lookup finds the
protosw and this check then fails the call.

Would it work to drop RCU, wait on module_wq until the owner leaves
MODULE_STATE_COMING, and then retry the lookup, instead of returning an
error straight away? inet6_create() in net/ipv6/af_inet6.c has the same
check.

[Severity: High]
Does the !kern short-circuit leave the failed-init case open for kernel
sockets created by other subsystems?

The commit message justifies the exemption with SCTP's own per-netns
control sockets. Those are destroyed by sctp_init()'s unwind through
unregister_pernet_subsys(&sctp_ctrlsock_ops). However, every kern caller
skips the COMING check, and try_module_get() succeeds on a COMING module.

DLM, for example, creates SCTP sockets with kern set:

fs/dlm/lowcomms.c:dlm_connect() {
    ...
	result = sock_create_kern(&init_net, dlm_local_addr[0].ss_family,
				  SOCK_STREAM, dlm_proto_ops->proto, &sock);
    ...
}

The DLM listen path does the same.

Consider this sequence:

1. sctp is COMING, and sctp_v4_protosw_init() has already published
   sctp_stream_protosw.

2. DLM's sock_create_kern() reaches inet_create() with kern=1 and skips
   the COMING check. try_module_get() succeeds, and sk_alloc() allocates
   from sctp_prot.slab.

3. A later step in sctp_init() fails, for example ENOMEM in
   register_pernet_subsys(&sctp_ctrlsock_ops), or a failure in
   sctp_v4_add_protocol() or sctp_v6_add_protocol(). The unwind runs:

	err_register_ctrlsock:
		sctp_v6_protosw_exit();
	err_v6_protosw_init:
		sctp_v4_protosw_exit();

   This reaches proto_unregister(&sctp_prot), which calls
   kmem_cache_destroy(prot->slab) while the DLM socket is still allocated
   in that cache.

4. The failure path in do_init_module() sets MODULE_STATE_GOING and calls
   synchronize_rcu() and module_put(). It then calls free_module()
   regardless of the remaining refcount.

After that, does the DLM socket's sk->sk_prot point into freed module
memory? If so, the next sendmsg, close or release on it would be a
use-after-free.

Neither the temporary reference added here nor the one taken in
sk_prot_alloc() stops free_module() on that failure path.

The underlying race is older than this patch. But the commit message
lists failed module initialization as a case this patch handles, and the
kern exemption keeps it reachable. Should the COMING rejection also apply
to kern sockets that the owning module did not create itself? The same
question applies to inet6_create().

> +	sock->ops = answer->ops;
>  	rcu_read_unlock();
>  
>  	WARN_ON(!answer_prot->slab);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013075604.868273-1-nicoyip.dev%40gmail.com