[net/netlink] Question about potential memleak in netlink_proto_init()

[net/netlink] Question about potential memleak in netlink_proto_init()

[Chenyuan Yang]

Dear Netlink Developers,

We are curious whether the function `netlink_proto_init()` might have a memory leak.

The function is https://elixir.bootlin.com/linux/v6.8/source/net/netlink/af_netlink.c#L2908
and the relevant code is
```
static int __init netlink_proto_init(void)
{
	int i;
  ...

	for (i = 0; i < MAX_LINKS; i++) {
		if (rhashtable_init(&nl_table[i].hash,
				    &netlink_rhashtable_params) < 0) {
			while (--i > 0)
				rhashtable_destroy(&nl_table[i].hash);
			kfree(nl_table);
			goto panic;
		}
	}
  ...
}
```

In the for loop, when `rhashtable_init()` fails, the function will free the allocated memory for `nl_table[i].hash` by checking `while (--i > 0)`. However, the first element (`i=1`) of `nl_table` is not freed since `i` is decremented before the check.

Based on our understanding, a possible fix would be
```
-      while (--i > 0)
+      while (--i >= 0)
```

Please kindly correct us if we missed any key information. Looking forward to your response!

Best,
Chenyuan

Re: [net/netlink] Question about potential memleak in netlink_proto_init()

[Florian Westphal]

Chenyuan Yang <chenyuan0y@gmail.com> wrote:
> Dear Netlink Developers,
> 
> We are curious whether the function `netlink_proto_init()` might have a memory leak.

Yes, but

> The function is https://elixir.bootlin.com/linux/v6.8/source/net/netlink/af_netlink.c#L2908
> and the relevant code is
> ```
> static int __init netlink_proto_init(void)
> {
> 	int i;
>   ...
> 
> 	for (i = 0; i < MAX_LINKS; i++) {
> 		if (rhashtable_init(&nl_table[i].hash,
> 				    &netlink_rhashtable_params) < 0) {
> 			while (--i > 0)
> 				rhashtable_destroy(&nl_table[i].hash);
> 			kfree(nl_table);
> 			goto panic;

... this calls panic(), kernel will crash intentionally.

Perhaps best patch would be to remove this error handling
and panic straight away, this is pretty much dead code.

Re: [net/netlink] Question about potential memleak in netlink_proto_init()

[Kuniyuki Iwashima]

From: Chenyuan Yang <chenyuan0y@gmail.com>
Date: Thu, 14 Mar 2024 19:47:18 -0500
> Dear Netlink Developers,
> 
> We are curious whether the function `netlink_proto_init()` might have a memory leak.
> 
> The function is https://elixir.bootlin.com/linux/v6.8/source/net/netlink/af_netlink.c#L2908
> and the relevant code is
> ```
> static int __init netlink_proto_init(void)
> {
> 	int i;
>   ...
> 
> 	for (i = 0; i < MAX_LINKS; i++) {
> 		if (rhashtable_init(&nl_table[i].hash,
> 				    &netlink_rhashtable_params) < 0) {
> 			while (--i > 0)
> 				rhashtable_destroy(&nl_table[i].hash);
> 			kfree(nl_table);
> 			goto panic;

If rhashtable_init() fails, the kernel panic occurs, so there's
no real memleak issue.


> 		}
> 	}
>   ...
> }
> ```
> 
> In the for loop, when `rhashtable_init()` fails, the function will free
> the allocated memory for `nl_table[i].hash` by checking `while (--i > 0)`.
> However, the first element (`i=1`) of `nl_table` is not freed since `i` is
> decremented before the check.
> 
> Based on our understanding, a possible fix would be
> ```
> -      while (--i > 0)
> +      while (--i >= 0)
> ```

Change itself looks good, no need for cleanup in the first place though.

Re: [net/netlink] Question about potential memleak in netlink_proto_init()

[Andy Shevchenko]

On Thu, Mar 14, 2024 at 07:47:18PM -0500, Chenyuan Yang wrote:
> Dear Netlink Developers,
> 
> We are curious whether the function `netlink_proto_init()` might have a
> memory leak.
> 
> The function is
> https://elixir.bootlin.com/linux/v6.8/source/net/netlink/af_netlink.c#L2908
> and the relevant code is
> ```
> static int __init netlink_proto_init(void)
> {
> 	int i;
>   ...
> 
> 	for (i = 0; i < MAX_LINKS; i++) {
> 		if (rhashtable_init(&nl_table[i].hash,
> 				    &netlink_rhashtable_params) < 0) {
> 			while (--i > 0)
> 				rhashtable_destroy(&nl_table[i].hash);
> 			kfree(nl_table);
> 			goto panic;
> 		}
> 	}
>   ...
> }
> ```
> 
> In the for loop, when `rhashtable_init()` fails, the function will free the
> allocated memory for `nl_table[i].hash` by checking `while (--i > 0)`.
> However, the first element (`i=1`) of `nl_table` is not freed since `i` is
> decremented before the check.
> 
> Based on our understanding, a possible fix would be
> ```
> -      while (--i > 0)
> +      while (--i >= 0)
> ```

The better pattern (and widely used in kernel) is

	while (i--)

> Please kindly correct us if we missed any key information. Looking forward to
> your response!

-- 
With Best Regards,
Andy Shevchenko

Re: [net/netlink] Question about potential memleak in netlink_proto_init()

[Chenyuan Yang]

Thank you all for sharing your insights on this issue.

I'm pondering over the best solution: should we follow Kuniyuki's
suggestion to "eliminate the cleanup code," or would it be better to
adopt Florian's approach of "eschewing error handling in favor of
immediate panic"?

Best,
Chenyuan

On Fri, Mar 15, 2024 at 9:36 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 14, 2024 at 07:47:18PM -0500, Chenyuan Yang wrote:
> > Dear Netlink Developers,
> >
> > We are curious whether the function `netlink_proto_init()` might have a
> > memory leak.
> >
> > The function is
> > https://elixir.bootlin.com/linux/v6.8/source/net/netlink/af_netlink.c#L2908
> > and the relevant code is
> > ```
> > static int __init netlink_proto_init(void)
> > {
> >       int i;
> >   ...
> >
> >       for (i = 0; i < MAX_LINKS; i++) {
> >               if (rhashtable_init(&nl_table[i].hash,
> >                                   &netlink_rhashtable_params) < 0) {
> >                       while (--i > 0)
> >                               rhashtable_destroy(&nl_table[i].hash);
> >                       kfree(nl_table);
> >                       goto panic;
> >               }
> >       }
> >   ...
> > }
> > ```
> >
> > In the for loop, when `rhashtable_init()` fails, the function will free the
> > allocated memory for `nl_table[i].hash` by checking `while (--i > 0)`.
> > However, the first element (`i=1`) of `nl_table` is not freed since `i` is
> > decremented before the check.
> >
> > Based on our understanding, a possible fix would be
> > ```
> > -      while (--i > 0)
> > +      while (--i >= 0)
> > ```
>
> The better pattern (and widely used in kernel) is
>
>         while (i--)
>
> > Please kindly correct us if we missed any key information. Looking forward to
> > your response!
>
> --
> With Best Regards,
> Andy Shevchenko
>
>