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

[Ehsan Mohandesi]

Hi Viacheslav,

> -----Original Message-----
> From: Vyacheslav V. Mitrofanov <v.v.mitrofanov at yadro.com>
> Sent: Thursday, March 16, 2023 3:47 AM
> To: u-boot at lists.denx.de; emohandesi at linux.microsoft.com
> Cc: joe.hershberger at ni.com; xypron.glpk at gmx.de;
> dphadke at linux.microsoft.com; saproj at gmail.com; rfried.dev at gmail.com;
> ilias.apalodimas at linaro.org; Ehsan Mohandesi <emohandesi at microsoft.com>;
> john at metanate.com; sjg at chromium.org; masahisa.kojima at linaro.org
> Subject: [EXTERNAL] Re: [PATCH] net: ipv6: Add support for default gateway
> discovery.
>
> On Thu, 2023-03-02 at 08:58 -0800, emohandesi at linux.microsoft.com
> wrote:
> >
> > From: Ehsan Mohandesi <emohandesi at 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 at 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?

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 bootfile
> >   *     Next 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 at yadro.com>
> Tested-by: Viacheslav Mitrofanov <v.v.mitrofanov at yadro.com>