[PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[Arun Ajith S]

Add a new neighbour cache entry in STALE state for routers on receiving
an unsolicited (gratuitous) neighbour advertisement with
target link-layer-address option specified.
This is similar to the arp_accept configuration for IPv4.
A new sysctl endpoint is created to turn on this behaviour:
/proc/sys/net/ipv6/conf/interface/accept_unsolicited_na.

Signed-off-by: Arun Ajith S <aajith@arista.com>
---
 Documentation/networking/ip-sysctl.rst        |  23 ++
 include/linux/ipv6.h                          |   1 +
 include/uapi/linux/ipv6.h                     |   1 +
 net/ipv6/addrconf.c                           |  10 +
 net/ipv6/ndisc.c                              |  20 +-
 tools/testing/selftests/net/Makefile          |   1 +
 .../net/ndisc_unsolicited_na_test.py          | 255 ++++++++++++++++++
 7 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/ndisc_unsolicited_na_test.py

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index b0024aa7b051..9e17efe343ac 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2467,6 +2467,29 @@ drop_unsolicited_na - BOOLEAN
 
 	By default this is turned off.
 
+accept_unsolicited_na - BOOLEAN
+	Add a new neighbour cache entry in STALE state for routers on receiving an
+	unsolicited neighbour advertisement with target link-layer address option
+	specified. This is as per router-side behavior documented in RFC9131.
+	This has lower precedence than drop_unsolicited_na.
+	 drop   accept  fwding                   behaviour
+	 ----   ------  ------  ----------------------------------------------
+	    1        X       X  Drop NA packet and don't pass up the stack
+	    0        0       X  Pass NA packet up the stack, don't update NC
+	    0        1       0  Pass NA packet up the stack, don't update NC
+	    0        1       1  Pass NA packet up the stack, and add a STALE
+	                          NC entry
+	This will optimize the return path for the initial off-link communication
+	that is initiated by a directly connected host, by ensuring that
+	the first-hop router which turns on this setting doesn't have to
+	buffer the initial return packets to do neighbour-solicitation.
+	The prerequisite is that the host is configured to send
+	unsolicited neighbour advertisements on interface bringup.
+	This setting should be used in conjunction with the ndisc_notify setting
+	on the host to satisfy this prerequisite.
+
+	By default this is turned off.
+
 enhanced_dad - BOOLEAN
 	Include a nonce option in the IPv6 neighbor solicitation messages used for
 	duplicate address detection per RFC7527. A received DAD NS will only signal
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 16870f86c74d..918bfea4ef5f 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -61,6 +61,7 @@ struct ipv6_devconf {
 	__s32		suppress_frag_ndisc;
 	__s32		accept_ra_mtu;
 	__s32		drop_unsolicited_na;
+	__s32		accept_unsolicited_na;
 	struct ipv6_stable_secret {
 		bool initialized;
 		struct in6_addr secret;
diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
index d4178dace0bf..549ddeaf788b 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -194,6 +194,7 @@ enum {
 	DEVCONF_IOAM6_ID,
 	DEVCONF_IOAM6_ID_WIDE,
 	DEVCONF_NDISC_EVICT_NOCARRIER,
+	DEVCONF_ACCEPT_UNSOLICITED_NA,
 	DEVCONF_MAX
 };
 
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 1afc4c024981..6473dc84b71d 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5587,6 +5587,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
 	array[DEVCONF_IOAM6_ID] = cnf->ioam6_id;
 	array[DEVCONF_IOAM6_ID_WIDE] = cnf->ioam6_id_wide;
 	array[DEVCONF_NDISC_EVICT_NOCARRIER] = cnf->ndisc_evict_nocarrier;
+	array[DEVCONF_ACCEPT_UNSOLICITED_NA] = cnf->accept_unsolicited_na;
 }
 
 static inline size_t inet6_ifla6_size(void)
@@ -7037,6 +7038,15 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.extra1		= (void *)SYSCTL_ZERO,
 		.extra2		= (void *)SYSCTL_ONE,
 	},
+	{
+		.procname	= "accept_unsolicited_na",
+		.data		= &ipv6_devconf.accept_unsolicited_na,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+		.extra1		= (void *)SYSCTL_ZERO,
+		.extra2		= (void *)SYSCTL_ONE,
+	},
 	{
 		/* sentinel */
 	}
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index fcb288b0ae13..254addad0dd3 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -979,6 +979,7 @@ static void ndisc_recv_na(struct sk_buff *skb)
 	struct inet6_dev *idev = __in6_dev_get(dev);
 	struct inet6_ifaddr *ifp;
 	struct neighbour *neigh;
+	bool create_neigh;
 
 	if (skb->len < sizeof(struct nd_msg)) {
 		ND_PRINTK(2, warn, "NA: packet too short\n");
@@ -999,6 +1000,7 @@ static void ndisc_recv_na(struct sk_buff *skb)
 	/* For some 802.11 wireless deployments (and possibly other networks),
 	 * there will be a NA proxy and unsolicitd packets are attacks
 	 * and thus should not be accepted.
+	 * drop_unsolicited_na takes precedence over accept_unsolicited_na
 	 */
 	if (!msg->icmph.icmp6_solicited && idev &&
 	    idev->cnf.drop_unsolicited_na)
@@ -1039,7 +1041,23 @@ static void ndisc_recv_na(struct sk_buff *skb)
 		in6_ifa_put(ifp);
 		return;
 	}
-	neigh = neigh_lookup(&nd_tbl, &msg->target, dev);
+	/* RFC 9131 updates original Neighbour Discovery RFC 4861.
+	 * An unsolicited NA can now create a neighbour cache entry
+	 * on routers if it has Target LL Address option.
+	 *
+	 * drop   accept  fwding                   behaviour
+	 * ----   ------  ------  ----------------------------------------------
+	 *    1        X       X  Drop NA packet and don't pass up the stack
+	 *    0        0       X  Pass NA packet up the stack, don't update NC
+	 *    0        1       0  Pass NA packet up the stack, don't update NC
+	 *    0        1       1  Pass NA packet up the stack, and add a STALE
+	 *                          NC entry
+	 * Note that we don't do a (daddr == all-routers-mcast) check.
+	 */
+	create_neigh = !msg->icmph.icmp6_solicited && lladdr &&
+		       idev && idev->cnf.forwarding &&
+		       idev->cnf.accept_unsolicited_na;
+	neigh = __neigh_lookup(&nd_tbl, &msg->target, dev, create_neigh);
 
 	if (neigh) {
 		u8 old_flags = neigh->flags;
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 3fe2515aa616..69415dbb61d2 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -36,6 +36,7 @@ TEST_PROGS += srv6_end_dt4_l3vpn_test.sh
 TEST_PROGS += srv6_end_dt6_l3vpn_test.sh
 TEST_PROGS += vrf_strict_mode_test.sh
 TEST_PROGS += arp_ndisc_evict_nocarrier.sh
+TEST_PROGS += ndisc_unsolicited_na_test.py
 TEST_PROGS_EXTENDED := in_netns.sh setup_loopback.sh setup_veth.sh
 TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh
 TEST_GEN_FILES =  socket nettest
diff --git a/tools/testing/selftests/net/ndisc_unsolicited_na_test.py b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
new file mode 100755
index 000000000000..f508657ee126
--- /dev/null
+++ b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
@@ -0,0 +1,255 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This test is for the accept_unsolicited_na feature to
+# enable RFC9131 behaviour. The following is the test-matrix.
+# drop   accept  fwding                   behaviour
+# ----   ------  ------  ----------------------------------------------
+#    1        X       X  Drop NA packet and don't pass up the stack
+#    0        0       X  Pass NA packet up the stack, don't update NC
+#    0        1       0  Pass NA packet up the stack, don't update NC
+#    0        1       1  Pass NA packet up the stack, and add a STALE
+#                           NC entry
+
+ret=0
+# Kselftest framework requirement - SKIP code is 4.
+ksft_skip=4
+
+PAUSE_ON_FAIL=no
+PAUSE=no
+
+HOST_NS="ns-host"
+ROUTER_NS="ns-router"
+
+HOST_INTF="veth-host"
+ROUTER_INTF="veth-router"
+
+ROUTER_ADDR="2000:20::1"
+HOST_ADDR="2000:20::2"
+SUBNET_WIDTH=64
+ROUTER_ADDR_WITH_MASK="${ROUTER_ADDR}/${SUBNET_WIDTH}"
+HOST_ADDR_WITH_MASK="${HOST_ADDR}/${SUBNET_WIDTH}"
+
+IP_HOST="ip -6 -netns ${HOST_NS}"
+IP_HOST_EXEC="ip netns exec ${HOST_NS}"
+IP_ROUTER="ip -6 -netns ${ROUTER_NS}"
+IP_ROUTER_EXEC="ip netns exec ${ROUTER_NS}"
+
+tcpdump_stdout=
+tcpdump_stderr=
+
+log_test()
+{
+	local rc=$1
+	local expected=$2
+	local msg="$3"
+
+	if [ ${rc} -eq ${expected} ]; then
+		printf "    TEST: %-60s  [ OK ]\n" "${msg}"
+		nsuccess=$((nsuccess+1))
+	else
+		ret=1
+		nfail=$((nfail+1))
+		printf "    TEST: %-60s  [FAIL]\n" "${msg}"
+		if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
+		echo
+			echo "hit enter to continue, 'q' to quit"
+			read a
+			[ "$a" = "q" ] && exit 1
+		fi
+	fi
+
+	if [ "${PAUSE}" = "yes" ]; then
+		echo
+		echo "hit enter to continue, 'q' to quit"
+		read a
+		[ "$a" = "q" ] && exit 1
+	fi
+}
+
+setup()
+{
+	set -e
+
+	local drop_unsolicited_na=$1
+	local accept_unsolicited_na=$2
+	local forwarding=$3
+
+	# Setup two namespaces and a veth tunnel across them.
+	# On end of the tunnel is a router and the other end is a host.
+	ip netns add ${HOST_NS}
+	ip netns add ${ROUTER_NS}
+	${IP_ROUTER} link add ${ROUTER_INTF} type veth \
+                peer name ${HOST_INTF} netns ${HOST_NS}
+
+	# Enable IPv6 on both router and host, and configure static addresses.
+	# The router here is the DUT
+	# Setup router configuration as specified by the arguments.
+	# forwarding=0 case is to check that a non-router
+	# doesn't add neighbour entries.
+        ROUTER_CONF=net.ipv6.conf.${ROUTER_INTF}
+	${IP_ROUTER_EXEC} sysctl -qw \
+                ${ROUTER_CONF}.forwarding=${forwarding}
+	${IP_ROUTER_EXEC} sysctl -qw \
+                ${ROUTER_CONF}.drop_unsolicited_na=${drop_unsolicited_na}
+	${IP_ROUTER_EXEC} sysctl -qw \
+                ${ROUTER_CONF}.accept_unsolicited_na=${accept_unsolicited_na}
+	${IP_ROUTER_EXEC} sysctl -qw ${ROUTER_CONF}.disable_ipv6=0
+	${IP_ROUTER} addr add ${ROUTER_ADDR_WITH_MASK} dev ${ROUTER_INTF}
+
+	# Turn on ndisc_notify on host interface so that
+	# the host sends unsolicited NAs.
+	HOST_CONF=net.ipv6.conf.${HOST_INTF}
+	${IP_HOST_EXEC} sysctl -qw ${HOST_CONF}.ndisc_notify=1
+	${IP_HOST_EXEC} sysctl -qw ${HOST_CONF}.disable_ipv6=0
+	${IP_HOST} addr add ${HOST_ADDR_WITH_MASK} dev ${HOST_INTF}
+
+	set +e
+}
+
+start_tcpdump() {
+	set -e
+	tcpdump_stdout=`mktemp`
+	tcpdump_stderr=`mktemp`
+	${IP_ROUTER_EXEC} timeout 15s \
+                tcpdump --immediate-mode -tpni ${ROUTER_INTF} -c 1 \
+                "icmp6 && icmp6[0] == 136 && src ${HOST_ADDR}" \
+                > ${tcpdump_stdout} 2> /dev/null
+	set +e
+}
+
+cleanup_tcpdump()
+{
+	set -e
+	[[ ! -z  ${tcpdump_stdout} ]] && rm -f ${tcpdump_stdout}
+	[[ ! -z  ${tcpdump_stderr} ]] && rm -f ${tcpdump_stderr}
+	tcpdump_stdout=
+	tcpdump_stderr=
+	set +e
+}
+
+cleanup()
+{
+	cleanup_tcpdump
+	ip netns del ${HOST_NS}
+	ip netns del ${ROUTER_NS}
+}
+
+link_up() {
+	set -e
+	${IP_ROUTER} link set dev ${ROUTER_INTF} up
+	${IP_HOST} link set dev ${HOST_INTF} up
+	set +e
+}
+
+verify_ndisc() {
+	local drop_unsolicited_na=$1
+	local accept_unsolicited_na=$2
+	local forwarding=$3
+
+	neigh_show_output=$(${IP_ROUTER} neigh show \
+                to ${HOST_ADDR} dev ${ROUTER_INTF} nud stale)
+	if [ ${drop_unsolicited_na} -eq 0 ] && \
+			[ ${accept_unsolicited_na} -eq 1 ] && \
+			[ ${forwarding} -eq 1 ]; then
+		# Neighbour entry expected to be present for 011 case
+		[[ ${neigh_show_output} ]]
+	else
+		# Neighbour entry expected to be absent for all other cases
+		[[ -z ${neigh_show_output} ]]
+	fi
+}
+
+test_unsolicited_na_common()
+{
+	# Setup the test bed, but keep links down
+	setup $1 $2 $3
+
+	# Bring the link up, wait for the NA,
+	# and add a delay to ensure neighbour processing is done.
+	link_up
+	start_tcpdump
+
+	# Verify the neighbour table
+	verify_ndisc $1 $2 $3
+
+}
+
+test_unsolicited_na_combination() {
+	test_unsolicited_na_common $1 $2 $3
+	test_msg=("test_unsolicited_na: "
+		"drop_unsolicited_na=$1 "
+		"accept_unsolicited_na=$2 "
+		"forwarding=$3")
+	log_test $? 0 "${test_msg[*]}"
+	cleanup
+}
+
+test_unsolicited_na_combinations() {
+	# Args: drop_unsolicited_na accept_unsolicited_na forwarding
+
+	# Expect entry
+	test_unsolicited_na_combination 0 1 1
+
+	# Expect no entry
+	test_unsolicited_na_combination 0 0 0
+	test_unsolicited_na_combination 0 0 1
+	test_unsolicited_na_combination 0 1 0
+	test_unsolicited_na_combination 1 0 0
+	test_unsolicited_na_combination 1 0 1
+	test_unsolicited_na_combination 1 1 0
+	test_unsolicited_na_combination 1 1 1
+}
+
+###############################################################################
+# usage
+
+usage()
+{
+	cat <<EOF
+usage: ${0##*/} OPTS
+        -p          Pause on fail
+        -P          Pause after each test before cleanup
+EOF
+}
+
+###############################################################################
+# main
+
+while getopts :pPh o
+do
+	case $o in
+		p) PAUSE_ON_FAIL=yes;;
+		P) PAUSE=yes;;
+		h) usage; exit 0;;
+		*) usage; exit 1;;
+	esac
+done
+
+# make sure we don't pause twice
+[ "${PAUSE}" = "yes" ] && PAUSE_ON_FAIL=no
+
+if [ "$(id -u)" -ne 0 ];then
+	echo "SKIP: Need root privileges"
+	exit $ksft_skip;
+fi
+
+if [ ! -x "$(command -v ip)" ]; then
+	echo "SKIP: Could not run test without ip tool"
+	exit $ksft_skip
+fi
+
+if [ ! -x "$(command -v tcpdump)" ]; then
+	echo "SKIP: Could not run test without tcpdump tool"
+	exit $ksft_skip
+fi
+
+# start clean
+cleanup &> /dev/null
+
+test_unsolicited_na_combinations
+
+printf "\nTests passed: %3d\n" ${nsuccess}
+printf "Tests failed: %3d\n"   ${nfail}
+
+exit $ret
-- 
2.27.0
---
Changes from v2:
1. Address David Ahern's review comments
 - Remove Tested-by from commit description
 - Add limits to new sysctl with extra1 and extra2
 - Add an unit test, added to TEST_PROGS

Test output:
#  time ./ndisc_unsolicited_na_test.py
    TEST: test_unsolicited_na:  drop_unsolicited_na=0  accept_unsolicited_na=1  forwarding=1  [ OK ]
    TEST: test_unsolicited_na:  drop_unsolicited_na=0  accept_unsolicited_na=0  forwarding=0  [ OK ]
    TEST: test_unsolicited_na:  drop_unsolicited_na=0  accept_unsolicited_na=0  forwarding=1  [ OK ]
    TEST: test_unsolicited_na:  drop_unsolicited_na=0  accept_unsolicited_na=1  forwarding=0  [ OK ]
    TEST: test_unsolicited_na:  drop_unsolicited_na=1  accept_unsolicited_na=0  forwarding=0  [ OK ]
    TEST: test_unsolicited_na:  drop_unsolicited_na=1  accept_unsolicited_na=0  forwarding=1  [ OK ]
    TEST: test_unsolicited_na:  drop_unsolicited_na=1  accept_unsolicited_na=1  forwarding=0  [ OK ]
    TEST: test_unsolicited_na:  drop_unsolicited_na=1  accept_unsolicited_na=1  forwarding=1  [ OK ]

Tests passed:   8
Tests failed:   0

real    0m21.700s
user    0m1.365s
sys     0m0.049s

Re: [PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[Eric Dumazet]

On 4/13/22 07:34, Arun Ajith S wrote:
> Add a new neighbour cache entry in STALE state for routers on receiving
> an unsolicited (gratuitous) neighbour advertisement with
> target link-layer-address option specified.
> This is similar to the arp_accept configuration for IPv4.
> A new sysctl endpoint is created to turn on this behaviour:
> /proc/sys/net/ipv6/conf/interface/accept_unsolicited_na.


Do we really need to expose this to /proc/sys, for every interface added 
in the host ?

/proc files creations/deletion cost a lot in environments 
adding/deleting netns very often.

I would prefer using NETLINK attributes, a single recvmsg() syscall can 
fetch/set hundreds of them.

Re: [PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[David Ahern]

On 4/13/22 3:22 PM, Eric Dumazet wrote:
> 
> On 4/13/22 07:34, Arun Ajith S wrote:
>> Add a new neighbour cache entry in STALE state for routers on receiving
>> an unsolicited (gratuitous) neighbour advertisement with
>> target link-layer-address option specified.
>> This is similar to the arp_accept configuration for IPv4.
>> A new sysctl endpoint is created to turn on this behaviour:
>> /proc/sys/net/ipv6/conf/interface/accept_unsolicited_na.
> 
> 
> Do we really need to expose this to /proc/sys, for every interface added
> in the host ?
> 
> /proc files creations/deletion cost a lot in environments
> adding/deleting netns very often.

agree with the general intent (along with the increasing memory costs).
I do think this case should be done as a /proc/sys entry for consistency
with both ARP and existing related NA settings.

> 
> I would prefer using NETLINK attributes, a single recvmsg() syscall can
> fetch/set hundreds of them.

What do you have in mind here? A link attribute managed through `ip link
set`?

Re: [PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[Eric Dumazet]

On 4/13/22 15:00, David Ahern wrote:
> On 4/13/22 3:22 PM, Eric Dumazet wrote:
>> On 4/13/22 07:34, Arun Ajith S wrote:
>>> Add a new neighbour cache entry in STALE state for routers on receiving
>>> an unsolicited (gratuitous) neighbour advertisement with
>>> target link-layer-address option specified.
>>> This is similar to the arp_accept configuration for IPv4.
>>> A new sysctl endpoint is created to turn on this behaviour:
>>> /proc/sys/net/ipv6/conf/interface/accept_unsolicited_na.
>>
>> Do we really need to expose this to /proc/sys, for every interface added
>> in the host ?
>>
>> /proc files creations/deletion cost a lot in environments
>> adding/deleting netns very often.
> agree with the general intent (along with the increasing memory costs).
> I do think this case should be done as a /proc/sys entry for consistency
> with both ARP and existing related NA settings.
>
>> I would prefer using NETLINK attributes, a single recvmsg() syscall can
>> fetch/set hundreds of them.
> What do you have in mind here? A link attribute managed through `ip link
> set`?


Yes, something like that, if this is a netdevice/link attribute.

Re: [PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[David Ahern]

On 4/13/22 8:34 AM, Arun Ajith S wrote:
> diff --git a/tools/testing/selftests/net/ndisc_unsolicited_na_test.py b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
> new file mode 100755
> index 000000000000..f508657ee126
> --- /dev/null
> +++ b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
> @@ -0,0 +1,255 @@
> +#!/bin/bash

that file name suffix should be .sh since it is a bash script; not .py

other than that looks good to me.

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[Arun Ajith S]

On Thu, Apr 14, 2022 at 3:37 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 4/13/22 8:34 AM, Arun Ajith S wrote:
> > diff --git a/tools/testing/selftests/net/ndisc_unsolicited_na_test.py b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
> > new file mode 100755
> > index 000000000000..f508657ee126
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
> > @@ -0,0 +1,255 @@
> > +#!/bin/bash
>
> that file name suffix should be .sh since it is a bash script; not .py
>
> other than that looks good to me.
>
> Reviewed-by: David Ahern <dsahern@kernel.org>

Hi David,

It has been pointed out to me that I might have read RFC9131 in a
narrower sense than what was intended.
The behavior of adding a new entry in the neighbour cache on receiving
a NA if none exists presently
shouldn't be limited to unsolicited NAs like in my original patch,
rather it should extend to all NAs.

I am quoting from the RFC below

   |  When a valid Neighbor Advertisement is received (either solicited
   |  or unsolicited), the Neighbor Cache is searched for the target's
   |  entry.  If no entry exists:
   |
   |  *  Hosts SHOULD silently discard the advertisement.  There is no
   |     need to create an entry if none exists, since the recipient has
   |     apparently not initiated any communication with the target.
   |
   |  *  Routers SHOULD create a new entry for the target address with
   |     the link-layer address set to the Target Link-Layer Address
   |     Option (if supplied).  The entry's reachability state MUST be
   |     set to STALE.  If the received Neighbor Advertisement does not
   |     contain the Target Link-Layer Address Option, the advertisement
   |     SHOULD be silently discarded.

I want to fix this, but this would mean the sysctl name
accept_unsolicited_na is no longer appropriate
I see that the net-next window for 5.19 is still open and changing the
sysctl name
wouldn't mean changing an existing interface.
I was thinking of renaming the sysctl to accept_untracked_na to
highlight that we are accepting NAs even if there is
no corresponding entry tracked in the neighbor cache.

Also, there's an error in my comment, where I say "pass up the stack"
as we don't pass NAs up the stack.
The comment can be updated as:
        /* RFC 9131 updates original Neighbour Discovery RFC 4861.
         * NAs with Target LL Address option without a corresponding
         * entry in the neighbour cache can now create a STALE neighbour
         * cache entry on routers.
         *
         *   entry accept  fwding  solicited        behaviour
         * ------- ------  ------  ---------    ----------------------
         * present      X       X         0     Set state to STALE
         * present      X       X         1     Set state to REACHABLE
         *  absent      0       X         X     Do nothing
         *  absent      1       0         X     Do nothing
         *  absent      1       1         X     Add a new STALE entry
         */

In summary
1. accept=0 keeps original(5.18) behavior for all cases.
2. accept=1 changes original behavior for entry=asbent, fwding=1 case
provided the NA had specified target link-layer address.

Please let me know what you think.

Thanks,
Arun

Re: [PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[David Ahern]

On 5/20/22 1:19 AM, Arun Ajith S wrote:
> On Thu, Apr 14, 2022 at 3:37 AM David Ahern <dsahern@kernel.org> wrote:
>>
>> On 4/13/22 8:34 AM, Arun Ajith S wrote:
>>> diff --git a/tools/testing/selftests/net/ndisc_unsolicited_na_test.py b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
>>> new file mode 100755
>>> index 000000000000..f508657ee126
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
>>> @@ -0,0 +1,255 @@
>>> +#!/bin/bash
>>
>> that file name suffix should be .sh since it is a bash script; not .py
>>
>> other than that looks good to me.
>>
>> Reviewed-by: David Ahern <dsahern@kernel.org>
> 
> Hi David,
> 
> It has been pointed out to me that I might have read RFC9131 in a
> narrower sense than what was intended.
> The behavior of adding a new entry in the neighbour cache on receiving
> a NA if none exists presently
> shouldn't be limited to unsolicited NAs like in my original patch,
> rather it should extend to all NAs.
> 
> I am quoting from the RFC below
> 
>    |  When a valid Neighbor Advertisement is received (either solicited
>    |  or unsolicited), the Neighbor Cache is searched for the target's
>    |  entry.  If no entry exists:
>    |
>    |  *  Hosts SHOULD silently discard the advertisement.  There is no
>    |     need to create an entry if none exists, since the recipient has
>    |     apparently not initiated any communication with the target.
>    |
>    |  *  Routers SHOULD create a new entry for the target address with
>    |     the link-layer address set to the Target Link-Layer Address
>    |     Option (if supplied).  The entry's reachability state MUST be
>    |     set to STALE.  If the received Neighbor Advertisement does not
>    |     contain the Target Link-Layer Address Option, the advertisement
>    |     SHOULD be silently discarded.
> 
> I want to fix this, but this would mean the sysctl name
> accept_unsolicited_na is no longer appropriate
> I see that the net-next window for 5.19 is still open and changing the
> sysctl name
> wouldn't mean changing an existing interface.
> I was thinking of renaming the sysctl to accept_untracked_na to
> highlight that we are accepting NAs even if there is
> no corresponding entry tracked in the neighbor cache.
> 
> Also, there's an error in my comment, where I say "pass up the stack"
> as we don't pass NAs up the stack.
> The comment can be updated as:
>         /* RFC 9131 updates original Neighbour Discovery RFC 4861.
>          * NAs with Target LL Address option without a corresponding
>          * entry in the neighbour cache can now create a STALE neighbour
>          * cache entry on routers.
>          *
>          *   entry accept  fwding  solicited        behaviour
>          * ------- ------  ------  ---------    ----------------------
>          * present      X       X         0     Set state to STALE
>          * present      X       X         1     Set state to REACHABLE
>          *  absent      0       X         X     Do nothing
>          *  absent      1       0         X     Do nothing
>          *  absent      1       1         X     Add a new STALE entry
>          */
> 
> In summary
> 1. accept=0 keeps original(5.18) behavior for all cases.
> 2. accept=1 changes original behavior for entry=asbent, fwding=1 case
> provided the NA had specified target link-layer address.
> 
> Please let me know what you think.
> 

Changes can be made until it is in a released kernel to users. This
feature has many weeks before it hits that level.

Re: [PATCH net-next v3] net/ipv6: Introduce accept_unsolicited_na knob to implement router-side changes for RFC9131

[Arun Ajith S]

On Sat, May 21, 2022 at 7:30 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 5/20/22 1:19 AM, Arun Ajith S wrote:
> > On Thu, Apr 14, 2022 at 3:37 AM David Ahern <dsahern@kernel.org> wrote:
> >>
> >> On 4/13/22 8:34 AM, Arun Ajith S wrote:
> >>> diff --git a/tools/testing/selftests/net/ndisc_unsolicited_na_test.py b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
> >>> new file mode 100755
> >>> index 000000000000..f508657ee126
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/net/ndisc_unsolicited_na_test.py
> >>> @@ -0,0 +1,255 @@
> >>> +#!/bin/bash
> >>
> >> that file name suffix should be .sh since it is a bash script; not .py
> >>
> >> other than that looks good to me.
> >>
> >> Reviewed-by: David Ahern <dsahern@kernel.org>
> >
> > Hi David,
> >
> > It has been pointed out to me that I might have read RFC9131 in a
> > narrower sense than what was intended.
> > The behavior of adding a new entry in the neighbour cache on receiving
> > a NA if none exists presently
> > shouldn't be limited to unsolicited NAs like in my original patch,
> > rather it should extend to all NAs.
> >
> > I am quoting from the RFC below
> >
> >    |  When a valid Neighbor Advertisement is received (either solicited
> >    |  or unsolicited), the Neighbor Cache is searched for the target's
> >    |  entry.  If no entry exists:
> >    |
> >    |  *  Hosts SHOULD silently discard the advertisement.  There is no
> >    |     need to create an entry if none exists, since the recipient has
> >    |     apparently not initiated any communication with the target.
> >    |
> >    |  *  Routers SHOULD create a new entry for the target address with
> >    |     the link-layer address set to the Target Link-Layer Address
> >    |     Option (if supplied).  The entry's reachability state MUST be
> >    |     set to STALE.  If the received Neighbor Advertisement does not
> >    |     contain the Target Link-Layer Address Option, the advertisement
> >    |     SHOULD be silently discarded.
> >
> > I want to fix this, but this would mean the sysctl name
> > accept_unsolicited_na is no longer appropriate
> > I see that the net-next window for 5.19 is still open and changing the
> > sysctl name
> > wouldn't mean changing an existing interface.
> > I was thinking of renaming the sysctl to accept_untracked_na to
> > highlight that we are accepting NAs even if there is
> > no corresponding entry tracked in the neighbor cache.
> >
> > Also, there's an error in my comment, where I say "pass up the stack"
> > as we don't pass NAs up the stack.
> > The comment can be updated as:
> >         /* RFC 9131 updates original Neighbour Discovery RFC 4861.
> >          * NAs with Target LL Address option without a corresponding
> >          * entry in the neighbour cache can now create a STALE neighbour
> >          * cache entry on routers.
> >          *
> >          *   entry accept  fwding  solicited        behaviour
> >          * ------- ------  ------  ---------    ----------------------
> >          * present      X       X         0     Set state to STALE
> >          * present      X       X         1     Set state to REACHABLE
> >          *  absent      0       X         X     Do nothing
> >          *  absent      1       0         X     Do nothing
> >          *  absent      1       1         X     Add a new STALE entry
> >          */
> >
> > In summary
> > 1. accept=0 keeps original(5.18) behavior for all cases.
> > 2. accept=1 changes original behavior for entry=asbent, fwding=1 case
> > provided the NA had specified target link-layer address.
> >
> > Please let me know what you think.
> >
>
> Changes can be made until it is in a released kernel to users. This
> feature has many weeks before it hits that level.

Thanks, I have made the proposed changes here:
https://patchwork.kernel.org/project/netdevbpf/patch/20220527073111.14336-1-aajith@arista.com/