Hi Viacheslav,
-----Original Message----- From: Vyacheslav V. Mitrofanov v.v.mitrofanov@yadro.com Sent: Thursday, March 16, 2023 3:47 AM To: u-boot@lists.denx.de; emohandesi@linux.microsoft.com Cc: joe.hershberger@ni.com; xypron.glpk@gmx.de; dphadke@linux.microsoft.com; saproj@gmail.com; rfried.dev@gmail.com; ilias.apalodimas@linaro.org; Ehsan Mohandesi emohandesi@microsoft.com; john@metanate.com; sjg@chromium.org; masahisa.kojima@linaro.org Subject: [EXTERNAL] Re: [PATCH] net: ipv6: Add support for default gateway discovery.
On Thu, 2023-03-02 at 08:58 -0800, emohandesi@linux.microsoft.com wrote:
From: Ehsan Mohandesi emohandesi@microsoft.com
In IPv6, the default gateway and prefix length are determined by receiving a router advertisement as defined in -
https://www.rf/ c- editor.org%2Frfc%2Frfc4861&data=05%7C01%7Cemohandesi%40microsoft.co m%7C6dec635abc8c4861feb708db25fb05d6%7C72f988bf86f141af91ab2d7cd01 1db47%7C1%7C0%7C638145532341238481%7CUnknown%7CTWFpbGZsb3d8ey JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C 3000%7C%7C%7C&sdata=tAhREBvBgVQKOFqEQT2%2FKphGxYXUMo3UF5vvQpY B%2Be0%3D&reserved=0.
Add support for sending router solicitation (RS) and processing router advertisements (RA).
If the RA has prefix info option and following conditions are met, then gatewayip6 and net_prefix_length of ip6addr env variables are initialized. These are later consumed by IPv6 code for non-local destination IP.
- "Router Lifetime" != 0
- Prefix is NOT link-local prefix (0xfe80::/10)
- L flag is 1
- "Valid Lifetime" != 0
Timing Parameters:
- MAX_RTR_SOLICITATION_DELAY (0-1s)
- RTR_SOLICITATION_INTERVAL (4s) (min retransmit delay)
- MAX_RTR_SOLICITATIONS (3 RS transmissions)
The functionality is enabled by CONFIG_IPV6_ROUTER_DISCOVERY and invoked automatically from net_init_loop().
Signed-off-by: Ehsan Mohandesi emohandesi@microsoft.com
Conflicts: cmd/Kconfig include/net.h net/net.c
cmd/Kconfig | 7 ++ include/ndisc.h | 23 ++++++ include/net.h | 2 +- include/net6.h | 40 ++++++++++ net/ndisc.c | 243 +++++++++++++++++++++++++++++++++++++++++++++++++++++--- net/net.c | 23 +++++- net/net6.c | 1 + 7 files changed, 327 insertions(+), 12 deletions(-)
I reviewed this patch and it looks good. I have no critical remarks, only some small notes.
I've tested it on SiFive Unmatched board.
+config IPV6_ROUTER_DISCOVERY
bool "Do router discovery"depends on IPV6helpWill automatically perform router solicitation on firstIPv6
network operationendif
I think it is better to write sth like Do IPv6 router discovery because IPv4 has also router discovery protocol and it could lead to misunderstanding
net_set_timeout_handler(0, 0);
Maybe net_set_timeout_handler(0, NULL); is better
+/*
- validate_ra() - Validate the router advertisement message.
- @ip6:
- @len: Length of the router advertisement packet
- Check if the router advertisement message is valid. Conditions
are
- according to RFC 4861 section 6.1.2. Validation of Router
Advertisement
- Messages.
- Return: true if the message is valid and false if it is invalid.
- */
+static bool validate_ra(struct ip6_hdr *ip6, int len) {
struct icmp6hdr *icmp = (struct icmp6hdr *)(ip6 + 1);/* ICMP length (derived from the IP length) should be 16 ormore octets. */
if (ip6->payload_len < 16)return false;/* Source IP Address should be a valid link-local address. */if ((ntohs(ip6->saddr.s6_addr16[0]) & IPV6_LINK_LOCAL_MASK)!=
IPV6_LINK_LOCAL_PREFIX)return false;/** The IP Hop Limit field should have a value of 255, i.e.,the packet
* could not possibly have been forwarded by a router.*/if (ip6->hop_limit != 255)return false;Unicast hop limit only?
Sorry, I do not understand what you mean here. What kind of scenario are you talking about? It does not matter if the router advertisement is unicast or multicast. In both cases, the hop limit needs to be 255. A router always sets the hop limit to 255. We do not want an advertisement that is forwarded from another node. Refer to this for more information. https://www.rfc-editor.org/rfc/rfc4861#section-6.1.2
diff --git a/net/net.c b/net/net.c index c9a749f..39f0b81 100644 --- a/net/net.c +++ b/net/net.c @@ -24,7 +24,7 @@
- name of bootfileNext step: ARP
- LINK_LOCAL:
- LINKLOCAL:
Maybe it is better to move to other patch?!
The underscore change on this line is a needed change. LINKLOCAL is used this way in the code. In the comment, it has an extra _ in it. It causes confusion. An important feature of Linux coding is being able to grep for strings. When I grepped for LINK_LOCAL I did not find what I was looking for. If there are more important reasons not to make this change, please let me know to revert it. I did not directly change any LINKLOCAL code, but I needed it for writing my code. It could happen to anyone. I think it is better to fix it here and save everyone's time now.
Reviewed-by: Viacheslav Mitrofanov v.v.mitrofanov@yadro.com Tested-by: Viacheslav Mitrofanov v.v.mitrofanov@yadro.com