Re: [PATCH net-next 4/5] bridge: Linearize skb once the ND message type is validated

[Nikolay Aleksandrov <razor@blackwall.org>]

On Sun, Jul 26, 2026 at 11:03:54AM +0000, Danielle Ratson wrote:
> > -----Original Message-----
> > From: Nikolay Aleksandrov <razor@blackwall.org>
> > Sent: Monday, 20 July 2026 12:26
> > To: Danielle Ratson <danieller@nvidia.com>; netdev@vger.kernel.org
> > Cc: dsahern@kernel.org; Ido Schimmel <idosch@nvidia.com>;
> > davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; horms@kernel.org; ja@ssi.bg; Petr Machata
> > <petrm@nvidia.com>; fw@strlen.de; kuniyu@google.com;
> > bridge@lists.linux.dev; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH net-next 4/5] bridge: Linearize skb once the ND message
> > type is validated
> > 
> > On 19/07/2026 16:34, Danielle Ratson wrote:
> > > br_nd_send() parses ND options from ns->opt[] and therefore needs the
> > > skb to be linear. Commit a01aee7cafc5 ("bridge: br_nd_send: linearize
> > > skb before parsing ND options") ensured that by linearizing inside
> > > br_nd_send() itself.
> > >
> > > Move the linearization up into br_is_nd_neigh_msg(), right after
> > > ndisc_check_ns_na() has validated the message as an NS/NA. This makes
> > > a linear buffer a property of every recognized ND message, so that
> > > this and any future ND message handling operate on a linear skb and
> > > cannot reintroduce that class of bug by forgetting to linearize.
> > >
> > > Since the skb is now linear by the time br_nd_send() runs, drop the
> > > linearization there and derive ns from the transport header set by
> > > ndisc_check_ns_na(), instead of recomputing it from the network header.
> > >
> > > If linearization fails under memory pressure, br_is_nd_neigh_msg()
> > > returns NULL and the packet falls back to normal forwarding rather
> > > than being suppressed.
> > >
> > > Reviewed-by: Petr Machata <petrm@nvidia.com>
> > > Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> > > ---
> > >   net/bridge/br_arp_nd_proxy.c | 8 +++++---
> > >   1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/bridge/br_arp_nd_proxy.c
> > > b/net/bridge/br_arp_nd_proxy.c index 445c930ed59b..46779d9fad61
> > 100644
> > > --- a/net/bridge/br_arp_nd_proxy.c
> > > +++ b/net/bridge/br_arp_nd_proxy.c
> > > @@ -240,6 +240,9 @@ struct nd_msg *br_is_nd_neigh_msg(struct sk_buff
> > *skb)
> > >   	if (ndisc_check_ns_na(skb))
> > >   		return NULL;
> > >
> > > +	if (skb_linearize(skb))
> > > +		return NULL;
> > > +
> > >   	return (struct nd_msg *)skb_transport_header(skb);
> > >   }
> > 
> > This one is a bit weird - I wouldn't expect the check and validation to also
> > linearize the skb. I'd rename this helper to show that it also linearizes the skb.
> > Other than that the patch looks good.
> 
> I agree it's not obvious from the name. But, I think it might be better to add a short comment than rename.
> The name captures the intent, while linearizing is just prep for the option parsing in br_nd_send().
> This also follows ipv6_mc_check_mld(), which is also a "check" that sets the transport header and documents its skb side effects in text rather than the name.
> 

Yeah, I'm not a fan of check functions having side effects at all, but
it's not that big of a deal and more of a personal preference.

> No other function here has a kdoc, so I'd keep it a plain comment:
> 
> /* Validate skb as an NS/NA and linearize it for br_nd_send()'s ND
>   * option parsing; returns the nd_msg, or NULL on failure.
>   */
>   
> Would that work, or do you feel strongly about the rename?
> 

That is ok.

Thanks,
 Nik

> > 
> > >
> > > @@ -259,7 +262,7 @@ static void br_nd_send(struct net_bridge *br, struct
> > net_bridge_port *p,
> > >   	bool dad;
> > >   	u16 pvid;
> > >
> > > -	if (!dev || skb_linearize(request))
> > > +	if (!dev)
> > >   		return;
> > >
> > >   	len = LL_RESERVED_SPACE(dev) + sizeof(struct ipv6hdr) + @@ -276,8
> > > +279,7 @@ static void br_nd_send(struct net_bridge *br, struct
> > net_bridge_port *p,
> > >   	skb_set_mac_header(reply, 0);
> > >
> > >   	daddr = eth_hdr(request)->h_source;
> > > -	ns = (struct nd_msg *)(skb_network_header(request) +
> > > -			       sizeof(struct ipv6hdr));
> > > +	ns = (struct nd_msg *)skb_transport_header(request);
> > >
> > >   	/* Do we need option processing ? */
> > >   	ns_olen = request->len - (skb_network_offset(request) +
>