Re: [PATCH net-next 2/3] ipv4: Pass netns to fib_table_lookup()

[Ido Schimmel <idosch@nvidia.com>]

On Wed, Oct 07, 2026 at 11:16:47PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] This patch adds `const struct net *net` as the new first
>   argument of the fib_table_lookup tracepoint.
> 
> Pre-existing issues:
> - [Low] This problem wasn't introduced by this patch, but the
>   fib_table_lookup trace event's TP_fast_assign leaves `__entry->gw4` (4…
> 
> --
> 
> ipv4: Pass netns to fib_table_lookup()
> 
> This commit adds a struct net argument to fib_table_lookup(). It passes
> that argument as the new first argument of the fib_table_lookup
> tracepoint and updates all callers. The next patch will use it to report
> the netns cookie in the tracepoint.
> 
> > diff --git a/include/trace/events/fib.h b/include/trace/events/fib.h
> > index feb28b359eff2..9a88060aa92e1 100644
> > --- a/include/trace/events/fib.h
> > +++ b/include/trace/events/fib.h
> > @@ -14,10 +14,10 @@
> >  
> >  TRACE_EVENT(fib_table_lookup,
> >  
> > -	TP_PROTO(u32 tb_id, const struct flowi4 *flp,
> > +	TP_PROTO(const struct net *net, u32 tb_id, const struct flowi4 *flp,
> >  		 const struct fib_nh_common *nhc, int err),
> >  
> > -	TP_ARGS(tb_id, flp, nhc, err),
> > +	TP_ARGS(net, tb_id, flp, nhc, err),
> 
> [Severity: Medium]
> Does the raw_tp_null_args[] table in kernel/bpf/btf.c need to change
> along with this prototype? It still has:
> 
> kernel/bpf/btf.c:raw_tp_null_args[] {
>     ...
> 	/* fib */
> 	{ "fib_table_lookup", 0x100 },
>     ...
> }
> 
> btf_ctx_access() applies this mask by argument position. It does this
> after skipping the hidden __data argument:
> 
> 	if (raw_tp_null_args[i].mask & (0x1ULL << (arg * 4)))
> 		info->reg_type |= PTR_MAYBE_NULL;
> 
> Now that net sits at position 0, 0x100 marks arg 2 (flp) as
> PTR_MAYBE_NULL. nhc has moved to arg 3 and is no longer marked nullable.
> 
> nhc can be NULL at runtime. fib_table_lookup() in net/ipv4/fib_trie.c
> passes NULL for it on both -EAGAIN paths and on the out_reject path:
> 
> 	trace_fib_table_lookup(net, tb->tb_id, flp, NULL, -EAGAIN);
> 
> For tp_btf programs, the verifier would then treat nhc as a trusted
> non-NULL pointer. It would accept code that uses nhc without a NULL
> check.
> 
> The verifier would also reject valid programs that read flp fields
> directly. check_mem_access() only allows direct PTR_TO_BTF_ID access when
> !type_may_be_null(reg->type). Programs would be rejected even after they
> are updated for the new argument order, as the commit message asks.
> 
> Should the entry become { "fib_table_lookup", 0x1000 } in this patch?

I will simply make 'net' the last tracepoint argument. It will be
inconsistent with the IPv6 tracepoint, but it will avoid this problem
and won't break BPF programs that attach to the raw tracepoint.

> 
> The later "ipv4: Report netns cookie in fib_table_lookup tracepoint"
> patch in the series does not seem to change it. This also seems to
> contradict the "No functional changes intended" line in the commit
> message.
> 
> >  
> >  	TP_STRUCT__entry(
> >  		__field(	u32,	tb_id		)
> 
> [Severity: Low]
> This is a pre-existing issue and was not introduced by this patch. In
> this event, TP_fast_assign() seems to leave gw4 and gw6 unset when nhc
> is non-NULL and its gateway family is neither AF_INET nor AF_INET6:
> 
> 	if (nhc) {
> 		if (nhc->nhc_gw_family == AF_INET) {
> 			...
> 		} else if (nhc->nhc_gw_family == AF_INET6) {
> 			...
> 		}
> 	} else {
> 		p32 = (__be32 *) __entry->gw4;
> 		*p32 = 0;
> 		...
> 	}
> 
> This happens for gateway-less routes, such as the local and connected
> routes that fib_magic() sets up. For those routes, fib_nh_init() copies
> a gateway family of 0 into the nexthop.
> 
> TP_printk() always prints both arrays with "gw %pI4/%pI6c". The event
> payload is not cleared when it is reserved, so can this print stale bytes
> left by earlier events?
> 
> trace_buffered_event_enable() also allocates its per-CPU buffered-event
> page without __GFP_ZERO. Can up to 20 bytes of old page contents reach
> trace readers that way?
> 
> Would it make sense to zero gw4 and gw6 before the family-specific
> assignments?

Pre-existing. I will take care of it separately.

pw-bot: cr

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006155454.853588-1-idosch%40nvidia.com