Re: [PATCH v3 net-next 2/6] ipv4: Inline inet_blackhole_dev_init() to devinet_init().

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In net/ipv4/devinet.c, devinet_init() now panics when
  inetdev_init(blackhole_netdev) returns an ERR_PTR.

Pre-existing issues:
- [Low] If alloc_netdev() fails in blackhole_netdev_init()
  (drivers/net/loopback.c, subsys_initcall_sync), the function returns…

--

ipv4: Inline inet_blackhole_dev_init() to devinet_init().

This commit removes the late_initcall inet_blackhole_dev_init(). The
in_device for blackhole_netdev is now allocated directly from
devinet_init(), because blackhole_netdev is allocated before inet_init().

> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index e84d3cf922474..2ed4c706ca9e3 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -340,18 +340,6 @@ static void inetdev_destroy(struct in_device *in_dev)
>  	in_dev_put(in_dev);
>  }
>  
> -static int __init inet_blackhole_dev_init(void)
> -{
> -	struct in_device *in_dev;
> -
> -	rtnl_lock();
> -	in_dev = inetdev_init(blackhole_netdev);
> -	rtnl_unlock();
> -
> -	return PTR_ERR_OR_ZERO(in_dev);
> -}
> -late_initcall(inet_blackhole_dev_init);
> -

[ ... ]

> @@ -2974,4 +2962,9 @@ void __init devinet_init(void)
>  		panic("Unable to register inet_af_ops\n");
>  
>  	rtnl_register_many(devinet_rtnl_msg_handlers);
> +
> +	rtnl_lock();
> +	if (IS_ERR(inetdev_init(blackhole_netdev)))
> +		panic("IP: failed to allocate in_device for blackhole_netdev\n");

[Severity: Low]
This isn't a bug, but should the commit message say that this failure is
now fatal?

The removed inet_blackhole_dev_init() returned PTR_ERR_OR_ZERO(in_dev).
On an inetdev_init() failure, the initcall core only logged the error,
and boot continued with blackhole_netdev->ip_ptr still NULL. After this
change, the same failure panics.

Panicking here matches how devinet_init() already handles a
rtnl_af_register() failure a few lines above. The only ways this call can
fail are the kzalloc_obj() and neigh_parms_alloc() allocations in
inetdev_init(). Even so, the commit message only says:

    Let's inline inet_blackhole_dev_init() to devinet_init().

Could a sentence be added noting that an allocation failure here now
panics instead of being ignored?

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch, but
what happens if blackhole_netdev is NULL at this point?

drivers/net/loopback.c:blackhole_netdev_init() {
	blackhole_netdev = alloc_netdev(0, "blackhole_dev", NET_NAME_UNKNOWN,
					blackhole_netdev_setup);
	if (!blackhole_netdev)
		return -ENOMEM;
	...
}

The initcall core ignores that -ENOMEM, so blackhole_netdev stays NULL.
inetdev_init() then dereferences it through dev_net(dev) on its first
line. That would oops before the new panic message is printed.

The removed late_initcall had no NULL check either, and addrconf_init()
also calls ipv6_add_dev(blackhole_netdev) without one. This patch just
moves an existing early-boot crash from late_initcall time to fs_initcall
time.

> +	rtnl_unlock();
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001204752.2572265-1-kuniyu%40google.com