Re: [PATCH 09/13 RFC net-next] net: fib: split common nexthop logic to fib_core.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] IPv6 route dumps silently omit nexthop gateway, interface, and encap information when IPv4 is disabled.
- [High] IPv6 strict route netlink dumps bypass filter parsing and ignore user filters when CONFIG_IPV4 is disabled.
- [High] Missing fallback stub for `ip_valid_fib_dump_req` causes undefined reference linker errors for IPv6 multicast routing and MPLS when compiled without IPv4.
--

--- Patch [9]: [PATCH 09/13 RFC net-next] net: fib: split common nexthop logic to fib_core.c ---
commit 8475b4cb1ca1bfc7fdd4717168ce4e1522e0104d
Author: Fernando Fernandez Mancera <fmancera@suse.de>
Subject: net: fib: split common nexthop logic to fib_core.c

This patch creates fib_core.c and moves common nexthop initialization and
release functions from fib_semantics.c to fib_core.c. This enables compiling
the INET subsystem without IPv4.

> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
> index 0a35355fb0f33..0c56d8690e008 100644
> --- a/include/net/ip_fib.h
> +++ b/include/net/ip_fib.h
[ ... ]
> @@ -658,8 +683,26 @@ int ip_valid_fib_dump_req(struct net *net, const struct nlmsghdr *nlh,
>  			  struct fib_dump_filter *filter,
>  			  struct netlink_callback *cb);
>  

[Severity: High]
Does this missing fallback stub cause undefined reference linker errors for IPv6
multicast routing and MPLS when compiled without IPv4?

ip6mr_rtm_dumproute() and mpls_valid_fib_dump_req() unconditionally call
ip_valid_fib_dump_req(). Could a static inline stub be added for the
!CONFIG_IPV4 case?

> +#if IS_ENABLED(CONFIG_IPV4)
>  int fib_nexthop_info(struct sk_buff *skb, const struct fib_nh_common *nh,
>  		     u8 rt_family, unsigned char *flags, bool skip_oif);
>  int fib_add_nexthop(struct sk_buff *skb, const struct fib_nh_common *nh,
>  		    int nh_weight, u8 rt_family, u32 nh_tclassid);
> +#else
> +static inline int fib_nexthop_info(struct sk_buff *skb,
> +				   const struct fib_nh_common *nh,
> +				   u8 rt_family, unsigned char *flags,
> +				   bool skip_oif)
> +{
> +	return 0;
> +}

[Severity: High]
Since the stub returns 0, will IPv6 route dumps silently omit the nexthop
gateway, interface, and encap information when IPv4 is disabled?

IPv6 relies on this function to serialize its nexthop info in rt6_fill_node():

rt6_fill_node()
    if (fib_nexthop_info(skb, &rt->fib6_nh->nh_common, AF_INET6, &nh_flags, false) < 0)
        goto nla_put_failure;

Could this be moved to fib_core.c instead of stubbed out so the RTA attributes
aren't dropped?

> +
> +static inline int fib_add_nexthop(struct sk_buff *skb,
> +				  const struct fib_nh_common *nh,
> +				  int nh_weight, u8 rt_family,
> +				  u32 nh_tclassid)
> +{
> +	return 0;
> +}
> +#endif
>  #endif  /* _NET_FIB_H */
> diff --git a/net/core/fib_core.c b/net/core/fib_core.c
> new file mode 100644
> index 0000000000000..93894b5847e97
> --- /dev/null
> +++ b/net/core/fib_core.c
[ ... ]
> +void fib_nhc_update_mtu(struct fib_nh_common *nhc, u32 new, u32 orig)
> +{
> +	struct fnhe_hash_bucket *bucket;
> +	int i;
> +
> +	bucket = rcu_dereference_protected(nhc->nhc_exceptions, 1);
> +	if (!bucket)
> +		return;
> +
> +	for (i = 0; i < FNHE_HASH_SIZE; i++) {
> +		struct fib_nh_exception *fnhe;
> +
> +		for (fnhe = rcu_dereference_protected(bucket[i].chain, 1);
> +		     fnhe;
> +		     fnhe = rcu_dereference_protected(fnhe->fnhe_next, 1)) {

[Severity: High]
This isn't a bug introduced by this patch, but does traversing the
nhc_exceptions list here without holding rcu_read_lock() or fnhe_lock risk a
use-after-free?

This loop traverses the fnhe_next chain using rcu_dereference_protected(..., 1).
Concurrently, update_or_create_fnhe() running in softirq can unlink and free a
fnhe entry via kfree_rcu(). Because rtnl_lock does not disable preemption,
can a grace period elapse and cause the traversal to follow a freed pointer?

> +			if (fnhe->fnhe_mtu_locked) {
> +				if (new <= fnhe->fnhe_pmtu) {
> +					fnhe->fnhe_pmtu = new;
> +					fnhe->fnhe_mtu_locked = false;
> +				}
> +			} else if (new < fnhe->fnhe_pmtu ||
> +				   orig == fnhe->fnhe_pmtu) {
> +				fnhe->fnhe_pmtu = new;
> +			}
> +		}
> +	}
> +}
[ ... ]
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index a130cdfaebfbe..8b5f855c8dcbf 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -645,9 +645,11 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
>  
>  	rcu_read_lock();
>  	if (cb->strict_check) {
> +#if IS_ENABLED(CONFIG_IPV4)
>  		err = ip_valid_fib_dump_req(net, nlh, &arg.filter, cb);
>  		if (err < 0)
>  			goto unlock;
> +#endif

[Severity: High]
When IPv4 is disabled, does this skip filter parsing and cause strict route
netlink dumps to unconditionally dump the entire routing table?

If the filter parsing is bypassed, arg.filter remains zero-initialized and
userspace filters like table ID or device constraints are ignored. Could this
break the NLM_F_STRICT_CHK uAPI guarantee?

>  	} else if (nlmsg_len(nlh) >= sizeof(struct rtmsg)) {
>  		struct rtmsg *rtm = nlmsg_data(nlh);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712013941.4570-1-fmancera@suse.de?part=9