On Sun, Jul 26, 2026 at 11:03:54AM +0000, Danielle Ratson wrote: > > -----Original Message----- > > From: Nikolay Aleksandrov <[email protected]> > > Sent: Monday, 20 July 2026 12:26 > > To: Danielle Ratson <[email protected]>; [email protected] > > Cc: [email protected]; Ido Schimmel <[email protected]>; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; Petr Machata > > <[email protected]>; [email protected]; [email protected]; > > [email protected]; [email protected] > > 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 <[email protected]> > > > Signed-off-by: Danielle Ratson <[email protected]> > > > --- > > > 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) + >
