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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pin protocol module before inet socket allocation

inet_create() and inet6_create() look up the inet_protosw under RCU and then
drop RCU before using answer_prot across the sleeping sk_alloc().  The patch
takes try_module_get(answer_prot->owner) while still under RCU and drops the
reference on every path after sk_alloc().

> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 32d006c1a8eed..88a18ac0e6e42 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -325,6 +325,10 @@ static int inet_create(struct net *net, struct socket *sock, int protocol,
>  	sock->ops = answer->ops;
>  	answer_prot = answer->prot;
>  	answer_flags = answer->flags;
> +	if (!try_module_get(answer_prot->owner)) {
> +		err = -EPROTONOSUPPORT;
> +		goto out_rcu_unlock;
> +	}
>  	rcu_read_unlock();
>  
>  	WARN_ON(!answer_prot->slab);

[Severity: High]
Does a plain try_module_get() here cover the case where the protocol module
is still inside its own init and then rolls back?

try_module_get() only gates on module_is_live():

kernel/module/main.c:try_module_get() {
	...
		if (likely(module_is_live(module) &&
			   atomic_inc_not_zero(&module->refcnt) != 0))
	...
}

include/linux/module.h:
/* FIXME: It'd be nice to isolate modules during init, too, so they
   aren't used before they (may) fail. */
static inline bool module_is_live(struct module *mod)
{
	return mod->state != MODULE_STATE_GOING;
}

So the pin succeeds for a module in MODULE_STATE_COMING, i.e. while
do_one_initcall(mod->init) is still running with module_mutex dropped.

Some protocol modules publish the protosw early in init and then destroy the
slab from their init-failure unwind, which does not consult the module
refcount:

net/sctp/protocol.c:sctp_v4_protosw_exit() {
	inet_unregister_protosw(&sctp_stream_protosw);
	inet_unregister_protosw(&sctp_seqpacket_protosw);
	proto_unregister(&sctp_prot);
}

reached from the err_protosw_init label in sctp_init() after
sctp_v4_protosw_init() has already registered the protosw.  net/smc has the
same shape:

net/smc/smc_inet.c:smc_inet_init() {
	...
out_inet6_protosw:
	proto_unregister(&smc_inet6_prot);
out_inet6_prot:
	inet_unregister_protosw(&smc_inet_protosw);
	proto_unregister(&smc_inet_prot);
	...
}

Can the sequence the commit message describes still happen with that?

CPU 0 inet_create                  CPU 1 modprobe sctp (mod->init)
rcu_read_lock()                    sctp_v4_protosw_init()  /* published */
answer_prot = answer->prot
try_module_get()  /* COMING -> ok */
rcu_read_unlock()                  /* later init step fails */
                                   sctp_v4_protosw_exit()
                                     inet_unregister_protosw()
                                       synchronize_net() /* returns at once */
                                     proto_unregister()
                                       kmem_cache_destroy(slab)
WARN_ON(!answer_prot->slab)
sk_alloc() -> kmem_cache_alloc(stale)

The kernel's own helper rejects exactly this state, which suggests a plain
reference is not a lifetime guarantee during init:

kernel/module/main.c:
/*
 * We require a truly strong try_module_get(): 0 means success.
 * Otherwise an error is returned due to ongoing or failed
 * initialization etc.
 */
static inline int strong_try_module_get(struct module *mod)
{
	BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED);
	if (mod && mod->state == MODULE_STATE_COMING)
		return -EBUSY;

Would rejecting owners in MODULE_STATE_COMING (or taking a reference tied to
the protosw/proto lifetime instead of the module refcount) close the residual
window?

> @@ -398,6 +402,8 @@ static int inet_create(struct net *net, struct socket *sock, int protocol,
>  		if (err)
>  			goto out_sk_release;
>  	}
> +out_module_put:
> +	module_put(answer_prot->owner);
>  out:
>  	return err;
>  out_rcu_unlock:

[Severity: High]
On the same init-failure path, can module_put(answer_prot->owner) here read
freed memory?

answer_prot lives in the module's .data, and do_init_module() frees the module
image without waiting for foreign references:

kernel/module/main.c:do_init_module() {
	...
fail:
	/* Try to protect us from buggy refcounters. */
	mod->state = MODULE_STATE_GOING;
	synchronize_rcu();
	module_put(mod);
	...
	free_module(mod);
}

That is unlike the delete_module() path, which does the refcount handshake in
try_stop_module()/try_release_module_ref().  So after free_module() runs,
both answer_prot->owner and the sock->ops pointer already stored above refer
to freed module memory.

[ ... ]

> diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
> index 282912a119999..82b390ce3e0cd 100644
> --- a/net/ipv6/af_inet6.c
> +++ b/net/ipv6/af_inet6.c
> @@ -170,6 +170,10 @@ static int inet6_create(struct net *net, struct socket *sock, int protocol,
>  	sock->ops = answer->ops;
>  	answer_prot = answer->prot;
>  	answer_flags = answer->flags;
> +	if (!try_module_get(answer_prot->owner)) {
> +		err = -EPROTONOSUPPORT;
> +		goto out_rcu_unlock;
> +	}
>  	rcu_read_unlock();
>  
>  	WARN_ON(!answer_prot->slab);

[Severity: High]
inet6_create() has the same exclusion, and it also dereferences the protosw
after RCU is dropped:

	sk->sk_backlog_rcv	= answer->prot->backlog_rcv;

Does the COMING-module case above apply here too, given that sctp and smc
register their IPv6 protosw during init and unwind through
inet_unregister_protosw() + proto_unregister() on failure?

[ ... ]
-- 
pw-bot: cr