[PATCH v1 net] af_unix: Add error handling in af_unix_init().

[PATCH v1 net] af_unix: Add error handling in af_unix_init().

[Kuniyuki Iwashima]

sock_register() and register_pernet_subsys() in af_unix_init() could
fail.

For example, after loading another PF_UNIX module (it's improbable
though), loading unix.ko fails at sock_register() and leaks slab
memory.  Also, register_pernet_subsys() fails when running out of
memory.

Let's handle errors appropriately.

Fixes: 097e66c57845 ("[NET]: Make AF_UNIX per network namespace safe [v2]")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index ede2b2a140a4..5352f30850f1 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -3720,7 +3720,7 @@ static void __init bpf_iter_register(void)
 
 static int __init af_unix_init(void)
 {
-	int i, rc = -1;
+	int i, rc;
 
 	BUILD_BUG_ON(sizeof(struct unix_skb_parms) > sizeof_field(struct sk_buff, cb));
 
@@ -3730,20 +3730,25 @@ static int __init af_unix_init(void)
 	}
 
 	rc = proto_register(&unix_dgram_proto, 1);
-	if (rc != 0) {
+	if (rc) {
 		pr_crit("%s: Cannot create unix_sock SLAB cache!\n", __func__);
 		goto out;
 	}
 
 	rc = proto_register(&unix_stream_proto, 1);
-	if (rc != 0) {
+	if (rc) {
 		pr_crit("%s: Cannot create unix_sock SLAB cache!\n", __func__);
-		proto_unregister(&unix_dgram_proto);
-		goto out;
+		goto err_proto_register;
 	}
 
-	sock_register(&unix_family_ops);
-	register_pernet_subsys(&unix_net_ops);
+	rc = sock_register(&unix_family_ops);
+	if (rc)
+		goto err_sock_register;
+
+	rc = register_pernet_subsys(&unix_net_ops);
+	if (rc)
+		goto err_register_pernet_subsys;
+
 	unix_bpf_build_proto();
 
 #if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
@@ -3752,6 +3757,15 @@ static int __init af_unix_init(void)
 
 out:
 	return rc;
+
+err_register_pernet_subsys:
+	sock_unregister(PF_UNIX);
+err_sock_register:
+	proto_unregister(&unix_stream_proto);
+err_proto_register:
+	proto_unregister(&unix_dgram_proto);
+
+	goto out;
 }
 
 static void __exit af_unix_exit(void)
-- 
2.30.2

Re: [PATCH v1 net] af_unix: Add error handling in af_unix_init().

[Alexander H Duyck]

On Wed, 2022-12-14 at 18:20 +0900, Kuniyuki Iwashima wrote:
> sock_register() and register_pernet_subsys() in af_unix_init() could
> fail.
> 
> For example, after loading another PF_UNIX module (it's improbable
> though), loading unix.ko fails at sock_register() and leaks slab
> memory.  Also, register_pernet_subsys() fails when running out of
> memory.
> 
> Let's handle errors appropriately.
> 
> Fixes: 097e66c57845 ("[NET]: Make AF_UNIX per network namespace safe [v2]")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

So the "Fixes" tags for this are no good. The 2nd one is from the start
of using git for the kernel. As such I am suspecting that this isn't
fixing a patch that introduced an issue.

Since it doesn't really seem like this is fixing an issue other than
adding some additional exception handling. I would suggest getting rid
of the "Fixes" tags and just submitting this for net-next once the
window opens.

The code itself looks fine, but I don't think it is really fixing much
either. If you want to wait to submit this for net-next you can include
this:

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Re: [PATCH v1 net] af_unix: Add error handling in af_unix_init().

[Jakub Kicinski]

On Wed, 14 Dec 2022 18:20:08 +0900 Kuniyuki Iwashima wrote:
>  static int __init af_unix_init(void)
>  {
> -	int i, rc = -1;
> +	int i, rc;

This is just a cleanup, right? Let's skip it, it will conflict.

>  	BUILD_BUG_ON(sizeof(struct unix_skb_parms) > sizeof_field(struct sk_buff, cb));
>  
> @@ -3730,20 +3730,25 @@ static int __init af_unix_init(void)
>  	}
>  
>  	rc = proto_register(&unix_dgram_proto, 1);
> -	if (rc != 0) {
> +	if (rc) {
>  		pr_crit("%s: Cannot create unix_sock SLAB cache!\n", __func__);

Should this message be updated?

>  		goto out;
>  	}
>  
>  	rc = proto_register(&unix_stream_proto, 1);
> -	if (rc != 0) {
> +	if (rc) {
>  		pr_crit("%s: Cannot create unix_sock SLAB cache!\n", __func__);

And this?

> -		proto_unregister(&unix_dgram_proto);
> -		goto out;
> +		goto err_proto_register;
>  	}

Re: [PATCH v1 net] af_unix: Add error handling in af_unix_init().

[Alexander Duyck]

On Wed, Dec 14, 2022 at 1:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 14 Dec 2022 11:18:05 -0800 Alexander H Duyck wrote:
> > So the "Fixes" tags for this are no good. The 2nd one is from the start
> > of using git for the kernel. As such I am suspecting that this isn't
> > fixing a patch that introduced an issue.
>
> We ask people to add the "start of history" tag when the issue goes all
> the way back.

The point I was getting at is that this issue doesn't really go all
the way back. Essentially sock_register could only fail if you were
registering a proto over 32 or one that was already registered. So
with 2.6.12-rc2 we should never see a failure without modifications to
the kernel as we only register PF_UNIX once and the other functions at
that time were void. This makes all the fixes tags suspect since the
patch doesn't resolve any issue with that code.

More likely candidates would have been:
Fixes: 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap")
Fixes: 097e66c57845 ("[NET]: Make AF_UNIX per network namespace safe [v2]")

> > Since it doesn't really seem like this is fixing an issue other than
> > adding some additional exception handling. I would suggest getting rid
> > of the "Fixes" tags and just submitting this for net-next once the
> > window opens.
> >
> > The code itself looks fine, but I don't think it is really fixing much
> > either.
>
> We don't do gradation of fixes in netdev, if a function can fail not
> handling the exception is a bug. I'm not saying that you're wrong
> calling this out as highly theoretical, all I'm saying is that I for
> one do not have the mental stamina to try to establish and use more
> complex heuristics. We haven't had material reasons to introduce any.

My concern was that this is more of a refactor/cleanup posing as a
fix. I know I have been guilty of that once or twice myself trying to
squeeze a patch in during the merge window.

Re: [PATCH v1 net] af_unix: Add error handling in af_unix_init().

[Jakub Kicinski]

On Wed, 14 Dec 2022 14:27:23 -0800 Alexander Duyck wrote:
> > We ask people to add the "start of history" tag when the issue goes all
> > the way back.  
> 
> The point I was getting at is that this issue doesn't really go all
> the way back. Essentially sock_register could only fail if you were
> registering a proto over 32 or one that was already registered. So
> with 2.6.12-rc2 we should never see a failure without modifications to
> the kernel as we only register PF_UNIX once and the other functions at
> that time were void. This makes all the fixes tags suspect since the
> patch doesn't resolve any issue with that code.
> 
> More likely candidates would have been:
> Fixes: 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap")
> Fixes: 097e66c57845 ("[NET]: Make AF_UNIX per network namespace safe [v2]")

I see! Makes sense.