Re: [PATCH net-next 0/5] bridge: Validate and clean up IPv6 neighbour suppression

[Petr Machata <petrm@nvidia.com>]

Nikolay Aleksandrov <razor@blackwall.org> writes:

> On 23/07/2026 09:40, Danielle Ratson wrote:
>>> -----Original Message-----
>>> From: Nikolay Aleksandrov <razor@blackwall.org>
>>> Sent: Monday, 20 July 2026 12:31
>>> 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 0/5] bridge: Validate and clean up IPv6
>>> neighbour suppression
>>>
>>> On 19/07/2026 16:34, Danielle Ratson wrote:
>>>> The bridge implements IPv6 neighbour suppression by snooping Neighbour
>>>> Solicitation and Neighbour Advertisement messages, but it previously
>>>> only checked the ICMPv6 type and code before acting on them. This
>>>> leaves it open to acting on malformed or spoofed packets that any RFC
>>>> 4861 compliant node should reject, and the option parsing in
>>>> br_nd_send() open-codes a loop that has historically been a source of bugs.
>>>>
>>>> This series hardens and cleans up that path:
>>>>
>>>> Add ndisc_check_ns_na(), a standalone NS/NA validator modeled after
>>>> ipv6_mc_check_mld(), implementing the RFC 4861 section 7.1.1 / 7.1.2
>>>> mandatory receive checks (hop limit, checksum, code, length, target
>>>> and option validation). Wire the bridge into it so NS/NA messages are
>>>> validated to the same standard MLD already enjoys.
>>>>
>>>> Replace the manual ND option parsing loop in br_nd_send() with
>>>> ndisc_parse_options() and ndisc_opt_addr_data(), and linearize the skb
>>>> once it has been validated as an NS/NA message so that this and any
>>>> future ND message handling operate on a linear buffer. The first patch
>>>> is a small preparatory cleanup that drops the now-unnecessary
>>>> skb_header_pointer() fallback from br_is_nd_neigh_msg().
>>>>
>>>> No functional change is intended for well-formed packets.
>>>>
>>>> Patchset overview:
>>>> Patch #1: drop the skb_header_pointer() fallback.
>>>> Patches #2-#3: add ndisc_check_ns_na() and validate NS/NA with it.
>>>> Patch #4: linearize once the ND message type is validated.
>>>> Patch #5: parse options via ndisc_parse_options().
>>>>
>>>> Danielle Ratson (5):
>>>>     bridge: Use direct pointer in br_is_nd_neigh_msg()
>>>>     ipv6: ndisc: Add ndisc_check_ns_na() validation helper
>>>>     bridge: Validate NS/NA messages using ndisc_check_ns_na()
>>>>     bridge: Linearize skb once the ND message type is validated
>>>>     bridge: Use ndisc_parse_options() to parse ND options in
>>>> br_nd_send()
>>>>
>>>>    include/net/ndisc.h          |   2 +
>>>>    net/bridge/br_arp_nd_proxy.c |  54 +++++-----
>>>>    net/bridge/br_device.c       |   4 +-
>>>>    net/bridge/br_input.c        |   4 +-
>>>>    net/bridge/br_private.h      |   2 +-
>>>>    net/ipv6/Makefile            |   2 +-
>>>>    net/ipv6/ndisc.c             |   1 +
>>>>    net/ipv6/ndisc_snoop.c       | 190
>>> +++++++++++++++++++++++++++++++++++
>>>>    8 files changed, 224 insertions(+), 35 deletions(-)
>>>>    create mode 100644 net/ipv6/ndisc_snoop.c
>>>>
>>>
>>> Nice set, but I'm curious - any reason not to use EXPORT_SYMBOL_GPL()
>>> instead?
>>>
>> I guess you are referring to ndisc_parse_options and ndisc_check_ns_na?
>> The reason is basically that other sibling functions used EXPORT_SYMBOL() rather than EXPORT_SYMBOL_GPL() (ndisc_send_skb, ndisc_ns_create, and the ndisc_check_ns_na() equivalent in mld, ipv6_mc_check_mld()).
>> However, ndisc_send_na() for example is newer and uses EXPORT_SYMBOL_GPL(). So it might make sense to use that in ndisc_parse_options(), if you prefer.
>> 
>
> Yep, I was referring to those newly exported symbols.
> Personally I'd always go for the _GPL variant when possible, but it's not a
> strong preference, more a personal one. I was just curious if there was some
> particular reason not to be. :)

Yeah, I missed this during the review.
I'm in favor of GPL as well.