Re: [PATCH 09/16 net-next v2] ipv4: fib: split common nexthop logic to fib_core.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 commit message undersells the patch.
- [Low] In include/net/ip_fib.h the patch changes the section comment "/*
  Exported by fib_frontend.c */" to "/* Exported by net/core/fib_core.c…
- [Low] fib_core.o is added to the unconditional obj-y list in
  net/core/Makefile (line 15).
- [Low] The patch adds !CONFIG_IPV4 stubs for fib_nh_init(),
  fib_nh_release(), fib_flush(), fib_info_notify_update() and…

--

ipv4: fib: split common nexthop logic to fib_core.c

This patch creates net/core/fib_core.c. It moves shared FIB and nexthop
helpers there from net/ipv4/fib_semantics.c and net/ipv4/fib_frontend.c, so
IPv6 can still use them when the IPv4 routing stack is not built. It also
adds !CONFIG_IPV4 stubs and guards in ip_fib.h, route.h, nexthop.c and
sysctl_net_ipv4.c.

> This patch creates fib_core.c and move all the functions for common
> nexthop initialization and release from fib_semantics.c to fib_core.c.
> Note that fib_core.c is under net/core.

[Severity: Low]
This isn't a bug, but the description covers only part of what the patch
does. Besides the init and release helpers, the patch also:

- moves rtm_ipv4_policy[] and the exported ip_valid_fib_dump_req() out of
  net/ipv4/fib_frontend.c
- moves fib_nexthop_info() and fib_add_nexthop(), which are netlink
  encoders and not init or release code
- adds !CONFIG_IPV4 stubs: fib_nh_init() returns -EAFNOSUPPORT, and
  fib_nh_release(), fib_flush(), fib_info_notify_update() and
  rt_cache_flush() become no-ops
- makes nexthop_is_good_nh() return false for AF_INET, and makes
  nh_netdev_event() skip nexthop_sync_mtu(), when CONFIG_IPV4 is disabled
- makes the net.ipv4.fib_sync_mem sysctl depend on CONFIG_IPV4, although
  a later patch in the series ("ipv4: disable IPv4-only sysctls when
  CONFIG_IPV4=n") exists for exactly that purpose

Once "ipv4: make CONFIG_IPV4 boolean" later in the series makes IPv4
selectable, users will see these changes. Creating an AF_INET nexthop
fails with -EAFNOSUPPORT, IPv4 nexthop MTU sync is skipped, and
/proc/sys/net/ipv4/fib_sync_mem goes away.

Could the commit message describe these extra changes, or could some of
them move into their own patches?

> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
> index 0a35355fb0f33..6058e53fa3dec 100644
> --- a/include/net/ip_fib.h
> +++ b/include/net/ip_fib.h

[ ... ]

> @@ -442,7 +448,7 @@ static inline bool fib_dscp_masked_match(dscp_t dscp, const struct flowi4 *fl4)
>  	return dscp == (fl4->flowi4_dscp & INET_DSCP_LEGACY_TOS_MASK);
>  }
>  
> -/* Exported by fib_frontend.c */
> +/* Exported by net/core/fib_core.c */
>  extern const struct nla_policy rtm_ipv4_policy[];
>  void ip_fib_init(void);
>  int fib_gw_from_via(struct fib_config *cfg, struct nlattr *nla,

[Severity: Low]
Is this comment accurate? Of the declarations under it, only
rtm_ipv4_policy moved to fib_core.c. The rest are still defined in
net/ipv4/fib_frontend.c, which is only built with CONFIG_IPV4:

    void ip_fib_init(void);
    int fib_gw_from_via(struct fib_config *cfg, struct nlattr *nla, ...);
    __be32 fib_compute_spec_dst(struct sk_buff *skb);
    bool fib_info_nh_uses_dev(struct fib_info *fi, ...);
    int fib_validate_source(struct sk_buff *skb, __be32 src, ...);

fib_unmerge() is in the same position.

The mismatch also goes the other way. fib_nh_common_init() and
fib_nh_common_release() are now defined only in net/core/fib_core.c, but
their declarations are still under the "Exported by fib_semantics.c"
section. The declarations of ip_valid_fib_dump_req(), fib_nexthop_info()
and fib_add_nexthop(), which also moved, have no attribution at all.

This series separates IPv4-only symbols from always-built ones. Could the
new comment suggest that fib_validate_source() or fib_gw_from_via() can be
called from code built with CONFIG_IPV4=n? Such a call would fail to link.

No later patch in the series touches include/net/ip_fib.h, so the comment
is still wrong at the end of the series.

[ ... ]

> @@ -578,10 +584,23 @@ void fib_select_multipath(struct fib_result *res, int hash,
>  void fib_select_path(struct net *net, struct fib_result *res,
>  		     struct flowi4 *fl4, const struct sk_buff *skb);
>  
> +#if IS_ENABLED(CONFIG_IPV4)
>  int fib_nh_init(struct net *net, struct fib_nh *fib_nh,
>  		struct fib_config *cfg, int nh_weight,
>  		struct netlink_ext_ack *extack);
>  void fib_nh_release(struct net *net, struct fib_nh *fib_nh);
> +#else
> +static inline int fib_nh_init(struct net *net, struct fib_nh *fib_nh,
> +			      struct fib_config *cfg, int nh_weight,
> +			      struct netlink_ext_ack *extack)
> +{
> +	return -EAFNOSUPPORT;
> +}
> +
> +static inline void fib_nh_release(struct net *net, struct fib_nh *fib_nh)
> +{
> +}
> +#endif

[Severity: Low]
In net/ipv4/Makefile, nexthop.o is obj-y and fib_semantics.o is
obj-$(CONFIG_IPV4). nexthop.c also calls three other fib_semantics.c
functions that get no stub here: fib_check_nh(),
fib_info_update_nhc_saddr() and fib_nhc_update_mtu().

For nh_create_ipv4(), does a CONFIG_IPV4=n build only link because the
compiler inlines this stub, folds the constant -EAFNOSUPPORT, and drops
the later calls?

net/ipv4/nexthop.c:nh_create_ipv4() {
    ...
	err = fib_nh_init(net, fib_nh, &fib_cfg, 1, extack);
	if (err) {
		fib_nh_release(net, fib_nh);
		goto out;
	}
    ...
	err = fib_check_nh(net, fib_nh, tb_id, 0, extack);
	if (!err) {
		nh->nh_flags = fib_nh->fib_nh_flags;
		fib_info_update_nhc_saddr(net, &fib_nh->nh_common,
    ...
}

The fib_nhc_update_mtu() call in nexthop_sync_mtu() is covered by the
explicit IS_ENABLED(CONFIG_IPV4) check in nh_netdev_event().
fib_check_nh() and fib_info_update_nhc_saddr() have no such guard.

If err here ever stops being a constant, for example because some work is
added before the stubbed call, wouldn't the link fail with undefined
references to these two functions?

There is also a user-facing gap. With IPv4 disabled, an AF_INET nexthop
request passes netlink parsing and then fails inside nh_create_ipv4()
with a bare -EAFNOSUPPORT and no extack message. Could this be rejected
earlier, with an extack message?

No later patch in the series changes nexthop.c or ip_fib.h.

[ ... ]

> diff --git a/net/core/Makefile b/net/core/Makefile
> index c20e520358b8e..315e437a49d8c 100644
> --- a/net/core/Makefile
> +++ b/net/core/Makefile
> @@ -12,7 +12,7 @@ obj-$(CONFIG_SYSCTL) += sysctl_net_core.o
>  obj-y		     += dev.o dev_api.o dev_addr_lists.o dst.o netevent.o \
>  			neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
>  			sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
> -			fib_notifier.o xdp.o flow_offload.o gro.o \
> +			fib_notifier.o fib_core.o xdp.o flow_offload.o gro.o \
>  			netdev-genl.o netdev-genl-gen.o netdev_work.o gso.o

[Severity: Low]
net/Makefile builds core/ for every CONFIG_NET kernel, and enters ipv4/
only when CONFIG_INET is set. With fib_core.o in the unconditional obj-y
list, won't CONFIG_NET=y, CONFIG_INET=n kernels now include this code even
though nothing in them calls it? That covers the rtable/fnhe freeing
helpers, fib_nh_common_init() and fib_nh_common_release(),
fib_nexthop_info() and fib_add_nexthop(), the rtm_ipv4_policy table, and
the EXPORT_SYMBOL_GPL'd ip_valid_fib_dump_req().

All users need INET: nexthop.o, net/ipv6/route.c, ipmr/ip6mr and MPLS.

Could fib_core.o go in obj-y in net/ipv4/Makefile instead? Alternatively,
it could be gated on CONFIG_INET here, as this Makefile already does for
bpf_ksock.o with "ifdef CONFIG_INET".

The commit message only says "Note that fib_core.c is under net/core."
and doesn't explain the placement. No later patch in the series changes
net/core/Makefile.

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