Re: [PATCH 07/16 net-next v2] 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 patch moves the per-CPU ICMP control socket setup (a
  for_each_possible_cpu() loop calling inet_ctl_sock_create()) out of…

--

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

This commit moves the IPv4 specific ICMP code (packet processing, error
handling, the icmp_pointers[] table and the per-CPU control socket setup)
out of net/ipv4/icmp.c into a new file, net/ipv4/icmp_ipv4.c. The new file
is built only when CONFIG_IPV4 is enabled, and icmp.c is moved to obj-y.

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

[Severity: Low]
Should icmp_init_ipv4() be marked __init?

The per-CPU control socket loop used to live directly in icmp_init(),
which is __init, so it was freed with init memory after boot. It now sits
in icmp_init_ipv4(), which has no annotation.

The only caller is still icmp_init(), which runs once at boot, so this
code can never run again. It now stays in resident .text instead of being
discarded. The commit message describes this as a plain move and doesn't
mention the change.

Would it make sense to use int __init icmp_init_ipv4(void) here, and
maybe put __init on the prototype in include/net/icmp.h as well?

> +		if (err < 0)
> +			return err;
> +
> +		per_cpu(ipv4_icmp_sk, i) = sk;

[ ... ]

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