* [PATCH net] net/ipv6: Change accept_unsolicited_na to accept_untracked_na
@ 2022-05-27 7:31 Arun Ajith S
2022-05-27 9:30 ` Bagas Sanjaya
0 siblings, 1 reply; 3+ messages in thread
From: Arun Ajith S @ 2022-05-27 7:31 UTC (permalink / raw
To: netdev
Cc: davem, linux-kernel, linux-doc, dsahern, bagasdotme, yoshfuji,
kuba, pabeni, aajith, gilligan, noureddine, gk
RFC 9131 changes default behavior of handling RX of NA messages when the
corresponding entry is absent in the neighbour cache. The current
implementation is limited to accept just unsolicited NAs. However, the
RFC is more generic where it also accepts solicited NAs. Both types
should result in adding a STALE entry for this case.
This change expands the current implementation to match the RFC. The
sysctl knob is also renamed to accept_untracked_na to better reflect the
implementation.
Signed-off-by: Arun Ajith S <aajith@arista.com>
---
This change updates the accept_unsolicited_na feature that merged to net-next
for v5.19 to be better compliant with the RFC. It also involves renaming the sysctl
knob to accept_untracked_na before shipping in a release.
Documentation/networking/ip-sysctl.rst | 18 +---
include/linux/ipv6.h | 2 +-
include/uapi/linux/ipv6.h | 2 +-
net/ipv6/addrconf.c | 6 +-
net/ipv6/ndisc.c | 101 ++++++++++--------
.../net/ndisc_unsolicited_na_test.sh | 23 ++--
6 files changed, 75 insertions(+), 77 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index b882d4238581..2f8a0d01a4bb 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2474,21 +2474,13 @@ drop_unsolicited_na - BOOLEAN
By default this is turned off.
-accept_unsolicited_na - BOOLEAN
+accept_untracked_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.
+ neighbour advertisement with target link-layer address option specified
+ if a corresponding entry is not already present.
+ This is as per router-side behavior documented in RFC9131.
- ==== ====== ====== ==============================================
- 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 has lower precedence than drop_unsolicited_na.
This will optimize the return path for the initial off-link communication
that is initiated by a directly connected host, by ensuring that
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 38c8203d52cb..37dfdcfcdd54 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -61,7 +61,7 @@ struct ipv6_devconf {
__s32 suppress_frag_ndisc;
__s32 accept_ra_mtu;
__s32 drop_unsolicited_na;
- __s32 accept_unsolicited_na;
+ __s32 accept_untracked_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 549ddeaf788b..03cdbe798fe3 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -194,7 +194,7 @@ enum {
DEVCONF_IOAM6_ID,
DEVCONF_IOAM6_ID_WIDE,
DEVCONF_NDISC_EVICT_NOCARRIER,
- DEVCONF_ACCEPT_UNSOLICITED_NA,
+ DEVCONF_ACCEPT_UNTRACKED_NA,
DEVCONF_MAX
};
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ca0aa744593e..1b1932502e9e 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5586,7 +5586,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;
+ array[DEVCONF_ACCEPT_UNTRACKED_NA] = cnf->accept_untracked_na;
}
static inline size_t inet6_ifla6_size(void)
@@ -7038,8 +7038,8 @@ static const struct ctl_table addrconf_sysctl[] = {
.extra2 = (void *)SYSCTL_ONE,
},
{
- .procname = "accept_unsolicited_na",
- .data = &ipv6_devconf.accept_unsolicited_na,
+ .procname = "accept_untracked_na",
+ .data = &ipv6_devconf.accept_untracked_na,
.maxlen = sizeof(int),
.mode = 0644,
.proc_handler = proc_dointvec_minmax,
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 254addad0dd3..adbae7a58513 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -979,7 +979,9 @@ 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;
+ u8 old_flags;
+ struct net *net;
+ u8 new_state;
if (skb->len < sizeof(struct nd_msg)) {
ND_PRINTK(2, warn, "NA: packet too short\n");
@@ -1000,7 +1002,6 @@ 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)
@@ -1041,61 +1042,67 @@ 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.
+ * NAs with Target LL Address option without a corresponding
+ * entry in the neighbour cache can now create a STALE neighbour
+ * cache entry on routers.
*
- * 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.
+ * 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
*/
- 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);
+ new_state = msg->icmph.icmp6_solicited ? NUD_REACHABLE : NUD_STALE;
+ if (!neigh && lladdr &&
+ idev && idev->cnf.forwarding &&
+ idev->cnf.accept_untracked_na) {
+ neigh = neigh_create(&nd_tbl, &msg->target, dev);
+ new_state = NUD_STALE;
+ }
- if (neigh) {
- u8 old_flags = neigh->flags;
- struct net *net = dev_net(dev);
+ if (IS_ERR(neigh) || !neigh)
+ return;
- if (neigh->nud_state & NUD_FAILED)
- goto out;
+ old_flags = neigh->flags;
+ net = dev_net(dev);
+
+ if (neigh->nud_state & NUD_FAILED)
+ goto out;
+
+ /* Don't update the neighbor cache entry on a proxy NA from
+ * ourselves because either the proxied node is off link or it
+ * has already sent a NA to us.
+ */
+ if (lladdr && !memcmp(lladdr, dev->dev_addr, dev->addr_len) &&
+ net->ipv6.devconf_all->forwarding && net->ipv6.devconf_all->proxy_ndp &&
+ pneigh_lookup(&nd_tbl, net, &msg->target, dev, 0)) {
+ /* XXX: idev->cnf.proxy_ndp */
+ goto out;
+ }
+
+ ndisc_update(dev, neigh, lladdr,
+ new_state,
+ NEIGH_UPDATE_F_WEAK_OVERRIDE |
+ (msg->icmph.icmp6_override ? NEIGH_UPDATE_F_OVERRIDE : 0) |
+ NEIGH_UPDATE_F_OVERRIDE_ISROUTER |
+ (msg->icmph.icmp6_router ? NEIGH_UPDATE_F_ISROUTER : 0),
+ NDISC_NEIGHBOUR_ADVERTISEMENT, &ndopts);
+ if ((old_flags & ~neigh->flags) & NTF_ROUTER) {
/*
- * Don't update the neighbor cache entry on a proxy NA from
- * ourselves because either the proxied node is off link or it
- * has already sent a NA to us.
+ * Change: router to host
*/
- if (lladdr && !memcmp(lladdr, dev->dev_addr, dev->addr_len) &&
- net->ipv6.devconf_all->forwarding && net->ipv6.devconf_all->proxy_ndp &&
- pneigh_lookup(&nd_tbl, net, &msg->target, dev, 0)) {
- /* XXX: idev->cnf.proxy_ndp */
- goto out;
- }
-
- ndisc_update(dev, neigh, lladdr,
- msg->icmph.icmp6_solicited ? NUD_REACHABLE : NUD_STALE,
- NEIGH_UPDATE_F_WEAK_OVERRIDE|
- (msg->icmph.icmp6_override ? NEIGH_UPDATE_F_OVERRIDE : 0)|
- NEIGH_UPDATE_F_OVERRIDE_ISROUTER|
- (msg->icmph.icmp6_router ? NEIGH_UPDATE_F_ISROUTER : 0),
- NDISC_NEIGHBOUR_ADVERTISEMENT, &ndopts);
-
- if ((old_flags & ~neigh->flags) & NTF_ROUTER) {
- /*
- * Change: router to host
- */
- rt6_clean_tohost(dev_net(dev), saddr);
- }
+ rt6_clean_tohost(dev_net(dev), saddr);
+ }
out:
- neigh_release(neigh);
- }
+ neigh_release(neigh);
}
static void ndisc_recv_rs(struct sk_buff *skb)
diff --git a/tools/testing/selftests/net/ndisc_unsolicited_na_test.sh b/tools/testing/selftests/net/ndisc_unsolicited_na_test.sh
index f508657ee126..86e621b7b9c7 100755
--- a/tools/testing/selftests/net/ndisc_unsolicited_na_test.sh
+++ b/tools/testing/selftests/net/ndisc_unsolicited_na_test.sh
@@ -1,15 +1,14 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
-# This test is for the accept_unsolicited_na feature to
+# This test is for the accept_untracked_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
+# 1 X X Don't update NC
+# 0 0 X Don't update NC
+# 0 1 0 Don't update NC
+# 0 1 1 Add a STALE NC entry
ret=0
# Kselftest framework requirement - SKIP code is 4.
@@ -72,7 +71,7 @@ setup()
set -e
local drop_unsolicited_na=$1
- local accept_unsolicited_na=$2
+ local accept_untracked_na=$2
local forwarding=$3
# Setup two namespaces and a veth tunnel across them.
@@ -93,7 +92,7 @@ setup()
${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}
+ ${ROUTER_CONF}.accept_untracked_na=${accept_untracked_na}
${IP_ROUTER_EXEC} sysctl -qw ${ROUTER_CONF}.disable_ipv6=0
${IP_ROUTER} addr add ${ROUTER_ADDR_WITH_MASK} dev ${ROUTER_INTF}
@@ -144,13 +143,13 @@ link_up() {
verify_ndisc() {
local drop_unsolicited_na=$1
- local accept_unsolicited_na=$2
+ local accept_untracked_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 ] && \
+ [ ${accept_untracked_na} -eq 1 ] && \
[ ${forwarding} -eq 1 ]; then
# Neighbour entry expected to be present for 011 case
[[ ${neigh_show_output} ]]
@@ -179,14 +178,14 @@ test_unsolicited_na_combination() {
test_unsolicited_na_common $1 $2 $3
test_msg=("test_unsolicited_na: "
"drop_unsolicited_na=$1 "
- "accept_unsolicited_na=$2 "
+ "accept_untracked_na=$2 "
"forwarding=$3")
log_test $? 0 "${test_msg[*]}"
cleanup
}
test_unsolicited_na_combinations() {
- # Args: drop_unsolicited_na accept_unsolicited_na forwarding
+ # Args: drop_unsolicited_na accept_untracked_na forwarding
# Expect entry
test_unsolicited_na_combination 0 1 1
--
2.27.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] net/ipv6: Change accept_unsolicited_na to accept_untracked_na
2022-05-27 7:31 [PATCH net] net/ipv6: Change accept_unsolicited_na to accept_untracked_na Arun Ajith S
@ 2022-05-27 9:30 ` Bagas Sanjaya
2022-05-27 11:30 ` Arun Ajith S
0 siblings, 1 reply; 3+ messages in thread
From: Bagas Sanjaya @ 2022-05-27 9:30 UTC (permalink / raw
To: Arun Ajith S, netdev
Cc: davem, linux-kernel, linux-doc, dsahern, yoshfuji, kuba, pabeni,
gilligan, noureddine, gk
On 5/27/22 14:31, Arun Ajith S wrote:
> This change expands the current implementation to match the RFC. The
> sysctl knob is also renamed to accept_untracked_na to better reflect the
> implementation.
>
Say in imperative, "Expand and rename accept_unsolicited na to
accept_untracked_na" in both commit/patch subject and message.
> -accept_unsolicited_na - BOOLEAN
> +accept_untracked_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.
> + neighbour advertisement with target link-layer address option specified
> + if a corresponding entry is not already present.
> + This is as per router-side behavior documented in RFC9131.
>
> - ==== ====== ====== ==============================================
> - 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 has lower precedence than drop_unsolicited_na.
>
I think you should have made similar logical expansion of drop_unsolicited_na to
drop_untracked_na. Otherwise, ...
> /* 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.
> + * NAs with Target LL Address option without a corresponding
> + * entry in the neighbour cache can now create a STALE neighbour
> + * cache entry on routers.
> *
> - * 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.
> + * 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
> */
The Documentation/ diff above drops behavior table but in the code comment
it is updated. Why didn't update in Documentation/ instead?
And my nitpick: for consistency, prefer en-US words over en-UK or mixed
varieties when writing (s/behaviour/behavior/gc, s/neighbour/neighbor/gc).
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] net/ipv6: Change accept_unsolicited_na to accept_untracked_na
2022-05-27 9:30 ` Bagas Sanjaya
@ 2022-05-27 11:30 ` Arun Ajith S
0 siblings, 0 replies; 3+ messages in thread
From: Arun Ajith S @ 2022-05-27 11:30 UTC (permalink / raw
To: Bagas Sanjaya
Cc: netdev, davem, linux-kernel, linux-doc, dsahern, yoshfuji, kuba,
pabeni, gilligan, noureddine, gk
On Fri, May 27, 2022 at 3:00 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On 5/27/22 14:31, Arun Ajith S wrote:
> > This change expands the current implementation to match the RFC. The
> > sysctl knob is also renamed to accept_untracked_na to better reflect the
> > implementation.
> >
>
> Say in imperative, "Expand and rename accept_unsolicited na to
> accept_untracked_na" in both commit/patch subject and message.
>
Okay, I will fix this.
> > -accept_unsolicited_na - BOOLEAN
> > +accept_untracked_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.
> > + neighbour advertisement with target link-layer address option specified
> > + if a corresponding entry is not already present.
> > + This is as per router-side behavior documented in RFC9131.
> >
> > - ==== ====== ====== ==============================================
> > - 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 has lower precedence than drop_unsolicited_na.
> >
>
> I think you should have made similar logical expansion of drop_unsolicited_na to
> drop_untracked_na. Otherwise, ...
>
drop_unsolicited_na is a separate feature which is pre-existing in linux-5.18.
> > /* 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.
> > + * NAs with Target LL Address option without a corresponding
> > + * entry in the neighbour cache can now create a STALE neighbour
> > + * cache entry on routers.
> > *
> > - * 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.
> > + * 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
> > */
>
> The Documentation/ diff above drops behavior table but in the code comment
> it is updated. Why didn't update in Documentation/ instead?
>
In the documentation, I skipped the table and mentioned the behavior in words.
1. drop_unsolicited_na takes precedence over accept_untracked_na.
2. The feature applies only for routers (fwding=1) and when target
link-layer address
option is specified.
Some of the behavior mentioned in the table is existing behavior and
didn't want to
document under this feature. I thought it best to skip the table in
Documentation as
it is more for code-readability.
> And my nitpick: for consistency, prefer en-US words over en-UK or mixed
> varieties when writing (s/behaviour/behavior/gc, s/neighbour/neighbor/gc).
>
Sure, I tried to use UK-en since the code uses that, but I might have mixed
it up unintentionally. I'll make it uniform in the next version of the patch.
> --
> An old man doll... just what I always wanted! - Clara
I accidentally replied in HTML earlier. Fixing it to be plaintext.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-27 11:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-27 7:31 [PATCH net] net/ipv6: Change accept_unsolicited_na to accept_untracked_na Arun Ajith S
2022-05-27 9:30 ` Bagas Sanjaya
2022-05-27 11:30 ` Arun Ajith S
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.