Re: [PATCH] net: ipv6: Add support for default gateway discovery.

Vyacheslav V. Mitrofanov
2023-03-16T08:47:16Z

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.rfc-editor.org/rfc/rfc4861.
>
> 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 IPV6
> + help
> + Will automatically perform router solicitation on first
> IPv6
> + network operation
> +
> endif
>
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 or
> more 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?

> 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 bootfile
> * Next step: ARP
> *
> - * LINK_LOCAL:
> + * LINKLOCAL:
>
Maybe it is better to move to other patch?!

Reviewed-by: Viacheslav Mitrofanov <v.v.mitrofanov@yadro.com>
Tested-by: Viacheslav Mitrofanov <v.v.mitrofanov@yadro.com>