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
[off-list ref] wrote:

On Thu, Mar 14, 2024 at 07:47:18PM -0500, Chenyuan Yang wrote:

quoted

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--)

quoted

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

--
With Best Regards,
Andy Shevchenko