Netdev List
 help / color / mirror / Atom feed
* [PATCH net] net: pin protocol module before socket allocation
@ 2026-08-23 17:13 Chengfeng Ye
  2026-08-24 18:22 ` Kuniyuki Iwashima
  2026-08-25 17:23 ` [PATCH net v2] net: pin protocol module before inet " Chengfeng Ye
  0 siblings, 2 replies; 6+ messages in thread
From: Chengfeng Ye @ 2026-08-23 17:13 UTC (permalink / raw)
  To: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
	David S. Miller, Jakub Kicinski, Simon Horman, Frank Filz
  Cc: netdev, linux-kernel, Chengfeng Ye

sk_prot_alloc() reads prot->slab and starts allocating the socket
before it gets a reference to prot->owner. A protocol module can begin
unloading after its protocol was selected but before the reference is
taken, allowing this interleaving:

  CPU 0                                CPU 1
  slab = prot->slab
                                       proto_unregister(prot)
                                         kmem_cache_destroy(prot->slab)
  kmem_cache_alloc(slab, ...)

kmem_cache_alloc() then dereferences a freed struct kmem_cache and can
crash or corrupt memory. The kernel reported:

  Oops: general protection fault, probably for non-canonical address
  KASAN: maybe wild-memory-access in range
  RIP: kmem_cache_alloc_noprof+0x63/0x370
  Call Trace:
   sk_prot_alloc+0x74/0x2c0
   sk_alloc+0x2b/0x6c0
   inet_create+0x2cd/0xd40
   __sock_create+0x1c3/0x430
   __sys_socket+0x116/0x1d0

Take the module reference before reading prot->slab so module removal
cannot destroy the cache during allocation. Drop that reference after
freeing the allocation on either failure path; on success sk_prot_free()
continues to release it as before.

Fixes: a79af59efd20 ("[NET]: Fix module reference counts for loadable protocol modules")
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/core/sock.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 1ad41904db25..59f4b15fd594 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2240,33 +2240,34 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
 	struct sock *sk;
 	struct kmem_cache *slab;
 
+	if (!try_module_get(prot->owner))
+		return NULL;
+
 	slab = prot->slab;
 	if (slab != NULL) {
 		sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
 		if (!sk)
-			return sk;
+			goto out_module_put;
 		if (want_init_on_alloc(priority))
 			sk_prot_clear_nulls(sk, prot->obj_size);
 	} else
 		sk = kmalloc(prot->obj_size, priority);
 
-	if (sk != NULL) {
-		if (security_sk_alloc(sk, family, priority))
-			goto out_free;
-
-		if (!try_module_get(prot->owner))
-			goto out_free_sec;
-	}
+	if (!sk)
+		goto out_module_put;
+
+	if (security_sk_alloc(sk, family, priority))
+		goto out_free;
 
 	return sk;
 
-out_free_sec:
-	security_sk_free(sk);
 out_free:
 	if (slab != NULL)
 		kmem_cache_free(slab, sk);
 	else
 		kfree(sk);
+out_module_put:
+	module_put(prot->owner);
 	return NULL;
 }
 
-- 
2.43.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: pin protocol module before socket allocation
  2026-08-23 17:13 [PATCH net] net: pin protocol module before socket allocation Chengfeng Ye
@ 2026-08-24 18:22 ` Kuniyuki Iwashima
  2026-08-25 17:26   ` Chengfeng Ye
  2026-08-25 17:23 ` [PATCH net v2] net: pin protocol module before inet " Chengfeng Ye
  1 sibling, 1 reply; 6+ messages in thread
From: Kuniyuki Iwashima @ 2026-08-24 18:22 UTC (permalink / raw)
  To: Chengfeng Ye
  Cc: Eric Dumazet, Paolo Abeni, Willem de Bruijn, David S. Miller,
	Jakub Kicinski, Simon Horman, Frank Filz, netdev, linux-kernel

On Sun, Aug 23, 2026 at 10:13 AM Chengfeng Ye <nicoyip.dev@gmail.com> wrote:
>
> sk_prot_alloc() reads prot->slab and starts allocating the socket
> before it gets a reference to prot->owner. A protocol module can begin
> unloading after its protocol was selected but before the reference is
> taken, allowing this interleaving:
>
>   CPU 0                                CPU 1
>   slab = prot->slab
>                                        proto_unregister(prot)
>                                          kmem_cache_destroy(prot->slab)
>   kmem_cache_alloc(slab, ...)
>
> kmem_cache_alloc() then dereferences a freed struct kmem_cache and can
> crash or corrupt memory. The kernel reported:
>
>   Oops: general protection fault, probably for non-canonical address
>   KASAN: maybe wild-memory-access in range
>   RIP: kmem_cache_alloc_noprof+0x63/0x370
>   Call Trace:
>    sk_prot_alloc+0x74/0x2c0
>    sk_alloc+0x2b/0x6c0
>    inet_create+0x2cd/0xd40

inet_create() rather looks broken.

inet_unregister_protosw() calls synchronize_net(), but the following
proto_unregister() might be done after rcu_read_unlock() and before
WARN_ON(!answer_prot->slab) in inet_create().

Which module did you use to trigger the bug ?


>    __sock_create+0x1c3/0x430
>    __sys_socket+0x116/0x1d0
>
> Take the module reference before reading prot->slab so module removal
> cannot destroy the cache during allocation. Drop that reference after
> freeing the allocation on either failure path; on success sk_prot_free()
> continues to release it as before.
>
> Fixes: a79af59efd20 ("[NET]: Fix module reference counts for loadable protocol modules")
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
>  net/core/sock.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 1ad41904db25..59f4b15fd594 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2240,33 +2240,34 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
>         struct sock *sk;
>         struct kmem_cache *slab;
>
> +       if (!try_module_get(prot->owner))
> +               return NULL;
> +
>         slab = prot->slab;
>         if (slab != NULL) {
>                 sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
>                 if (!sk)
> -                       return sk;
> +                       goto out_module_put;
>                 if (want_init_on_alloc(priority))
>                         sk_prot_clear_nulls(sk, prot->obj_size);
>         } else
>                 sk = kmalloc(prot->obj_size, priority);
>
> -       if (sk != NULL) {
> -               if (security_sk_alloc(sk, family, priority))
> -                       goto out_free;
> -
> -               if (!try_module_get(prot->owner))
> -                       goto out_free_sec;
> -       }
> +       if (!sk)
> +               goto out_module_put;
> +
> +       if (security_sk_alloc(sk, family, priority))
> +               goto out_free;
>
>         return sk;
>
> -out_free_sec:
> -       security_sk_free(sk);
>  out_free:
>         if (slab != NULL)
>                 kmem_cache_free(slab, sk);
>         else
>                 kfree(sk);
> +out_module_put:
> +       module_put(prot->owner);
>         return NULL;
>  }
>
> --
> 2.43.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net v2] net: pin protocol module before inet socket allocation
  2026-08-23 17:13 [PATCH net] net: pin protocol module before socket allocation Chengfeng Ye
  2026-08-24 18:22 ` Kuniyuki Iwashima
@ 2026-08-25 17:23 ` Chengfeng Ye
  2026-08-30 23:57   ` Jakub Kicinski
  1 sibling, 1 reply; 6+ messages in thread
From: Chengfeng Ye @ 2026-08-25 17:23 UTC (permalink / raw)
  To: Eric Dumazet, Kuniyuki Iwashima, Paolo Abeni, Willem de Bruijn,
	David S. Miller, Jakub Kicinski, Simon Horman
  Cc: netdev, linux-kernel, Chengfeng Ye, stable

inet_create() and inet6_create() look up the protocol under
rcu_read_lock(), then drop RCU before using the resulting proto.
sk_alloc() uses GFP_KERNEL, so RCU cannot be held across it.

A loadable protocol can be unregistered in that window.
inet_unregister_protosw() waits with synchronize_net() only for
readers still in the RCU section. After rcu_read_unlock(), module
exit can run proto_unregister(), destroy prot->slab, and free the
module. inet_create() then uses a dangling proto pointer:

  CPU 0 inet_create                     CPU 1 l2tp_ip_exit
  rcu_read_lock()
  answer_prot = answer->prot
  rcu_read_unlock()                     inet_unregister_protosw()
                                          synchronize_net()
                                        proto_unregister()
                                          kmem_cache_destroy(slab)
  WARN_ON(!answer_prot->slab)
  sk_alloc() -> kmem_cache_alloc(stale)

This was reproduced with socket(AF_INET, SOCK_DGRAM, IPPROTO_L2TP)
racing delete_module("l2tp_ip"):

  Oops: general protection fault, probably for non-canonical address
  KASAN: maybe wild-memory-access in range
  RIP: kmem_cache_alloc_noprof+0x63/0x370
  Call Trace:
   sk_prot_alloc+0x74/0x2c0
   sk_alloc+0x2b/0x6c0
   inet_create+0x2cd/0xd40
   __sock_create+0x1c3/0x430
   __sys_socket+0x116/0x1d0

__sock_create() already converts family lookup into a module
reference before dropping RCU, but that pins inet, not the protocol
module. Pin answer_prot->owner while still under RCU, then allocate.
sk_prot_alloc() still takes the socket-lifetime reference; drop the
temporary one on every path after sk_alloc(). inet6_create() has the
same hole.

Fixes: a79af59efd20 ("[NET]: Fix module reference counts for loadable protocol modules")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
v2: Pin answer_prot->owner in inet_create()/inet6_create() under
    RCU instead of reordering try_module_get() in sk_prot_alloc().
    sk_alloc() can sleep, so the lookup must be converted into a
    module reference before rcu_read_unlock(). Suggested by
    Kuniyuki Iwashima.

 net/ipv4/af_inet.c  | 10 ++++++++--
 net/ipv6/af_inet6.c | 10 ++++++++--
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 32d006c1a8ee..88a18ac0e6e4 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);
@@ -332,7 +336,7 @@ static int inet_create(struct net *net, struct socket *sock, int protocol,
 	err = -ENOMEM;
 	sk = sk_alloc(net, PF_INET, GFP_KERNEL, answer_prot, kern);
 	if (!sk)
-		goto out;
+		goto out_module_put;
 
 	err = 0;
 	if (INET_PROTOSW_REUSE & answer_flags)
@@ -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:
@@ -406,7 +412,7 @@ static int inet_create(struct net *net, struct socket *sock, int protocol,
 out_sk_release:
 	sk_common_release(sk);
 	sock->sk = NULL;
-	goto out;
+	goto out_module_put;
 }
 
 
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 282912a11999..82b390ce3e0c 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);
@@ -177,7 +181,7 @@ static int inet6_create(struct net *net, struct socket *sock, int protocol,
 	err = -ENOBUFS;
 	sk = sk_alloc(net, PF_INET6, GFP_KERNEL, answer_prot, kern);
 	if (!sk)
-		goto out;
+		goto out_module_put;
 
 	sock_init_data(sock, sk);
 
@@ -251,6 +255,8 @@ static int inet6_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:
@@ -259,7 +265,7 @@ static int inet6_create(struct net *net, struct socket *sock, int protocol,
 out_sk_release:
 	sk_common_release(sk);
 	sock->sk = NULL;
-	goto out;
+	goto out_module_put;
 }
 
 int __inet6_bind(struct sock *sk, struct sockaddr_unsized *uaddr, int addr_len,
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: pin protocol module before socket allocation
  2026-08-24 18:22 ` Kuniyuki Iwashima
@ 2026-08-25 17:26   ` Chengfeng Ye
  0 siblings, 0 replies; 6+ messages in thread
From: Chengfeng Ye @ 2026-08-25 17:26 UTC (permalink / raw)
  To: Kuniyuki Iwashima
  Cc: Eric Dumazet, Paolo Abeni, Willem de Bruijn, David S. Miller,
	Jakub Kicinski, Simon Horman, Frank Filz, netdev, linux-kernel

On Tue, Aug 25, 2026 at 2:22 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> inet_create() rather looks broken.
>
> inet_unregister_protosw() calls synchronize_net(), but the following
> proto_unregister() might be done after rcu_read_unlock() and before
> WARN_ON(!answer_prot->slab) in inet_create().

Right, that is the root cause. As sk_alloc() uses GFP_KERNEL, it
cannot run under rcu_read_lock(). I just sent a v2 by moving the extra
module reference get/put to inet_create().

> Which module did you use to trigger the bug ?

It is l2tp_ip, via socket(AF_INET, SOCK_DGRAM, IPPROTO_L2TP) racing
delete_module("l2tp_ip"). I think inet6_create() also suffers from
this bug as it has a similar pattern, so v2 also fixes that path as
well.

Best regards,
Chengfeng

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net v2] net: pin protocol module before inet socket allocation
  2026-08-25 17:23 ` [PATCH net v2] net: pin protocol module before inet " Chengfeng Ye
@ 2026-08-30 23:57   ` Jakub Kicinski
  2026-09-04 11:16     ` Chengfeng Ye
  0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2026-08-30 23:57 UTC (permalink / raw)
  To: nicoyip.dev
  Cc: Jakub Kicinski, edumazet, kuniyu, pabeni, willemb, davem, horms,
	netdev, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net v2] net: pin protocol module before inet socket allocation
  2026-08-30 23:57   ` Jakub Kicinski
@ 2026-09-04 11:16     ` Chengfeng Ye
  0 siblings, 0 replies; 6+ messages in thread
From: Chengfeng Ye @ 2026-09-04 11:16 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: edumazet, kuniyu, pabeni, willemb, davem, horms, netdev,
	linux-kernel, stable

On Mon, Aug 31, 2026 at 7:57 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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


Thanks, a v3 is sent to address both the problems.

Best regards,
Chengfeng

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-04 11:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-23 17:13 [PATCH net] net: pin protocol module before socket allocation Chengfeng Ye
2026-08-24 18:22 ` Kuniyuki Iwashima
2026-08-25 17:26   ` Chengfeng Ye
2026-08-25 17:23 ` [PATCH net v2] net: pin protocol module before inet " Chengfeng Ye
2026-08-30 23:57   ` Jakub Kicinski
2026-09-04 11:16     ` Chengfeng Ye

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox