All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexey Dobriyan <adobriyan@gmail.com>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Luca Tettamanti <kronos.it@gmail.com>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	David Miller <davem@davemloft.net>
Subject: [PATCH] af_key: fix netns ops ordering on module load/unload
Date: Sat, 30 Jan 2010 14:53:27 +0200	[thread overview]
Message-ID: <20100130125327.GA4258@x200> (raw)
In-Reply-To: <1264782806.3184.42.camel@edumazet-laptop>

On Fri, Jan 29, 2010 at 05:33:26PM +0100, Eric Dumazet wrote:
> @@ -3807,21 +3807,24 @@ static int __init ipsec_pfkey_init(void)
>  	if (err != 0)
>  		goto out;
>  
> -	err = sock_register(&pfkey_family_ops);
> -	if (err != 0)
> -		goto out_unregister_key_proto;
>  	err = xfrm_register_km(&pfkeyv2_mgr);
>  	if (err != 0)
> -		goto out_sock_unregister;
> +		goto out_unregister_key_proto;
> +
>  	err = register_pernet_subsys(&pfkey_net_ops);
>  	if (err != 0)
>  		goto out_xfrm_unregister_km;
> +
> +	err = sock_register(&pfkey_family_ops);
> +	if (err != 0)
> +		goto out_unregister_pernet;
>  out:
>  	return err;
> +
> +out_unregister_pernet:
> +	unregister_pernet_subsys(&pfkey_net_ops);
>  out_xfrm_unregister_km:
>  	xfrm_unregister_km(&pfkeyv2_mgr);
> -out_sock_unregister:
> -	sock_unregister(PF_KEY);
>  out_unregister_key_proto:
>  	proto_unregister(&key_proto);
>  	goto out;

ACK analysis, except this is not enough.

Here is patch which survived netns start/stop/modprobe/rmmod cycles.

	Alexey, who still doesn't get why bug reproduces so easily for bug reporter.

Luca, please confirm.

Audit for the rest of the modules pending.

[PATCH] af_key: fix netns ops ordering on module load/unload

1. After sock_register() returns, it's possible to create sockets,
   even if module still not initialized fully (blame generic module code
   for that!)
2. Consequently, pfkey_create() can be called with pfkey_net_id still not
   initialized which will BUG_ON in net_generic():
	kernel BUG at include/net/netns/generic.h:43!
3. During netns shutdown, netns ops should be unregistered after
   key manager unregistered because key manager calls can be triggered
   from xfrm_user module:

   	general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
	pfkey_broadcast+0x111/0x210 [af_key]
	pfkey_send_notify+0x16a/0x300 [af_key]
	km_state_notify+0x41/0x70
	xfrm_flush_sa+0x75/0x90 [xfrm_user]
4. Unregister netns ops after socket ops just in case and for symmetry.

Reported by Luca Tettamanti.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 net/key/af_key.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -3794,9 +3794,9 @@ static struct pernet_operations pfkey_net_ops = {
 
 static void __exit ipsec_pfkey_exit(void)
 {
-	unregister_pernet_subsys(&pfkey_net_ops);
 	xfrm_unregister_km(&pfkeyv2_mgr);
 	sock_unregister(PF_KEY);
+	unregister_pernet_subsys(&pfkey_net_ops);
 	proto_unregister(&key_proto);
 }
 
@@ -3807,21 +3807,22 @@ static int __init ipsec_pfkey_init(void)
 	if (err != 0)
 		goto out;
 
-	err = sock_register(&pfkey_family_ops);
+	err = register_pernet_subsys(&pfkey_net_ops);
 	if (err != 0)
 		goto out_unregister_key_proto;
+	err = sock_register(&pfkey_family_ops);
+	if (err != 0)
+		goto out_unregister_pernet;
 	err = xfrm_register_km(&pfkeyv2_mgr);
 	if (err != 0)
 		goto out_sock_unregister;
-	err = register_pernet_subsys(&pfkey_net_ops);
-	if (err != 0)
-		goto out_xfrm_unregister_km;
 out:
 	return err;
-out_xfrm_unregister_km:
-	xfrm_unregister_km(&pfkeyv2_mgr);
+
 out_sock_unregister:
 	sock_unregister(PF_KEY);
+out_unregister_pernet:
+	unregister_pernet_subsys(&pfkey_net_ops);
 out_unregister_key_proto:
 	proto_unregister(&key_proto);
 	goto out;

  reply	other threads:[~2010-01-30 12:58 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-01-29  9:48 [2.6.33-rc5] kernel BUG at include/net/netns/generic.h:41! Luca Tettamanti
2010-01-29 10:17 ` Alexey Dobriyan
2010-01-29 15:22   ` Eric Dumazet
2010-01-29 16:33     ` [PATCH] xfrm: Change initializations order in ipsec_pfkey_init() Eric Dumazet
2010-01-30 12:53       ` Alexey Dobriyan [this message]
2010-02-01 13:50         ` [PATCH] af_key: fix netns ops ordering on module load/unload Luca Tettamanti
2010-02-01 13:56           ` Eric Dumazet
2010-02-04  2:11             ` David Miller
2010-01-30 13:23   ` [2.6.33-rc5] kernel BUG at include/net/netns/generic.h:41! Luca Tettamanti

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20100130125327.GA4258@x200 \
    --to=adobriyan@gmail.com \
    --cc=davem@davemloft.net \
    --cc=eric.dumazet@gmail.com \
    --cc=kronos.it@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.