Re: [PATCH 07/16 net-next v3] ipv4: icmp: split IPv4 specific logic into icmp_ipv4.c

[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] The boot-only per-CPU ICMP control-socket setup loop used to be
  inside `int __init icmp_init(void)`.

--

ipv4: icmp: split IPv4 specific logic into icmp_ipv4.c

This commit moves the IPv4 specific ICMP code from net/ipv4/icmp.c into
a new file, net/ipv4/icmp_ipv4.c. That code covers packet processing,
error handling and the per-CPU control socket setup. The new file is
built only when CONFIG_IPV4 is enabled, while icmp.c is now always
built.

> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 90c0e22c29bea..5e8dcf71d135f 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c

[ ... ]

> @@ -1757,28 +366,11 @@ static struct pernet_operations __net_initdata icmp_sk_ops = {
>  
>  int __init icmp_init(void)
>  {
> -	int err, i;
> -
> -	for_each_possible_cpu(i) {
> -		struct sock *sk;
> +	if (IS_ENABLED(CONFIG_IPV4)) {
> +		int err = icmp_init_ipv4();
>  
> -		err = inet_ctl_sock_create(&sk, PF_INET,
> -					   SOCK_RAW, IPPROTO_ICMP, &init_net);
> -		if (err < 0)
> +		if (err)
>  			return err;

[ ... ]

> diff --git a/net/ipv4/icmp_ipv4.c b/net/ipv4/icmp_ipv4.c
> new file mode 100644
> index 0000000000000..24a1fb497cf6f
> --- /dev/null
> +++ b/net/ipv4/icmp_ipv4.c

[ ... ]

> +int icmp_init_ipv4(void)
> +{
> +	int err, i;
> +
> +	for_each_possible_cpu(i) {
> +		struct sock *sk;
> +
> +		err = inet_ctl_sock_create(&sk, PF_INET,
> +					   SOCK_RAW, IPPROTO_ICMP, &init_net);
> +		if (err < 0)
> +			return err;
> +
> +		per_cpu(ipv4_icmp_sk, i) = sk;

[Severity: Low]
This loop used to run inside the __init icmp_init(). The new
icmp_init_ipv4() has no __init annotation, but its only caller is still
icmp_init(). So this code is no longer freed after boot.

Should icmp_init_ipv4() be marked __init, both here and on its prototype
in include/net/icmp.h?

The prototype also sits in the public header next to the normal ICMP
entry points, which makes a boot-only helper look like a general API. If
something called it a second time, it would overwrite the per-CPU
ipv4_icmp_sk pointers and leak the old sockets. No such caller exists
today.

The annotation also appears to be missing at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930135334.4739-1-fmancera%40suse.de