Re: [net-next v2] net: change accept_ra_min_rtr_lft to affect all RA lifetimes

["Maciej Żenczykowski" <maze@google.com>]

On Thu, Jul 27, 2023 at 1:07 AM Patrick Rohr <prohr@google.com> wrote:
>
> accept_ra_min_rtr_lft only considered the lifetime of the default route
> and discarded entire RAs accordingly.
>
> This change renames accept_ra_min_rtr_lft to accept_ra_min_lft, and
> applies the value to individual RA sections; in particular, router
> lifetime, PIO preferred lifetime, and RIO lifetime. If any of those
> lifetimes are lower than the configured value, the specific RA section
> is ignored.
>
> In order for the sysctl to be useful to Android, it should really apply
> to all lifetimes in the RA, since that is what determines the minimum
> frequency at which RAs must be processed by the kernel. Android uses
> hardware offloads to drop RAs for a fraction of the minimum of all
> lifetimes present in the RA (some networks have very frequent RAs (5s)
> with high lifetimes (2h)). Despite this, we have encountered networks
> that set the router lifetime to 30s which results in very frequent CPU
> wakeups. Instead of disabling IPv6 (and dropping IPv6 ethertype in the
> WiFi firmware) entirely on such networks, it seems better to ignore the
> misconfigured routers while still processing RAs from other IPv6 routers
> on the same network (i.e. to support IoT applications).
>
> The previous implementation dropped the entire RA based on router
> lifetime. This turned out to be hard to expand to the other lifetimes
> present in the RA in a consistent manner; dropping the entire RA based
> on RIO/PIO lifetimes would essentially require parsing the whole thing
> twice.
>
> Fixes: 1671bcfd76fd ("net: add sysctl accept_ra_min_rtr_lft")
> Cc: Maciej Żenczykowski <maze@google.com>
> Cc: Lorenzo Colitti <lorenzo@google.com>
> Cc: David Ahern <dsahern@kernel.org>
> Signed-off-by: Patrick Rohr <prohr@google.com>
> ---
>  Documentation/networking/ip-sysctl.rst |  8 ++++----
>  include/linux/ipv6.h                   |  2 +-
>  include/uapi/linux/ipv6.h              |  2 +-
>  net/ipv6/addrconf.c                    | 14 ++++++++-----
>  net/ipv6/ndisc.c                       | 27 +++++++++++---------------
>  5 files changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 37603ad6126b..a66054d0763a 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -2288,11 +2288,11 @@ accept_ra_min_hop_limit - INTEGER
>
>         Default: 1
>
> -accept_ra_min_rtr_lft - INTEGER
> -       Minimum acceptable router lifetime in Router Advertisement.
> +accept_ra_min_lft - INTEGER
> +       Minimum acceptable lifetime value in Router Advertisement.
>
> -       RAs with a router lifetime less than this value shall be
> -       ignored. RAs with a router lifetime of 0 are unaffected.
> +       RA sections with a lifetime less than this value shall be
> +       ignored. Zero lifetimes stay unaffected.
>
>         Default: 0
>
> diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
> index 0295b47c10a3..5883551b1ee8 100644
> --- a/include/linux/ipv6.h
> +++ b/include/linux/ipv6.h
> @@ -33,7 +33,7 @@ struct ipv6_devconf {
>         __s32           accept_ra_defrtr;
>         __u32           ra_defrtr_metric;
>         __s32           accept_ra_min_hop_limit;
> -       __s32           accept_ra_min_rtr_lft;
> +       __s32           accept_ra_min_lft;
>         __s32           accept_ra_pinfo;
>         __s32           ignore_routes_with_linkdown;
>  #ifdef CONFIG_IPV6_ROUTER_PREF
> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
> index 8b6bcbf6ed4a..cf592d7b630f 100644
> --- a/include/uapi/linux/ipv6.h
> +++ b/include/uapi/linux/ipv6.h
> @@ -198,7 +198,7 @@ enum {
>         DEVCONF_IOAM6_ID_WIDE,
>         DEVCONF_NDISC_EVICT_NOCARRIER,
>         DEVCONF_ACCEPT_UNTRACKED_NA,
> -       DEVCONF_ACCEPT_RA_MIN_RTR_LFT,
> +       DEVCONF_ACCEPT_RA_MIN_LFT,
>         DEVCONF_MAX
>  };
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 19eb4b3d26ea..7f7d2b677711 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -202,7 +202,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {
>         .ra_defrtr_metric       = IP6_RT_PRIO_USER,
>         .accept_ra_from_local   = 0,
>         .accept_ra_min_hop_limit= 1,
> -       .accept_ra_min_rtr_lft  = 0,
> +       .accept_ra_min_lft      = 0,
>         .accept_ra_pinfo        = 1,
>  #ifdef CONFIG_IPV6_ROUTER_PREF
>         .accept_ra_rtr_pref     = 1,
> @@ -263,7 +263,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
>         .ra_defrtr_metric       = IP6_RT_PRIO_USER,
>         .accept_ra_from_local   = 0,
>         .accept_ra_min_hop_limit= 1,
> -       .accept_ra_min_rtr_lft  = 0,
> +       .accept_ra_min_lft      = 0,
>         .accept_ra_pinfo        = 1,
>  #ifdef CONFIG_IPV6_ROUTER_PREF
>         .accept_ra_rtr_pref     = 1,
> @@ -2727,6 +2727,10 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
>                 return;
>         }
>
> +       if (valid_lft != 0 && valid_lft < in6_dev->cnf.accept_ra_min_lft) {
> +               return;
> +       }
> +
>         /*
>          *      Two things going on here:
>          *      1) Add routes for on-link prefixes
> @@ -5598,7 +5602,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
>         array[DEVCONF_IOAM6_ID_WIDE] = cnf->ioam6_id_wide;
>         array[DEVCONF_NDISC_EVICT_NOCARRIER] = cnf->ndisc_evict_nocarrier;
>         array[DEVCONF_ACCEPT_UNTRACKED_NA] = cnf->accept_untracked_na;
> -       array[DEVCONF_ACCEPT_RA_MIN_RTR_LFT] = cnf->accept_ra_min_rtr_lft;
> +       array[DEVCONF_ACCEPT_RA_MIN_LFT] = cnf->accept_ra_min_lft;
>  }
>
>  static inline size_t inet6_ifla6_size(void)
> @@ -6793,8 +6797,8 @@ static const struct ctl_table addrconf_sysctl[] = {
>                 .proc_handler   = proc_dointvec,
>         },
>         {
> -               .procname       = "accept_ra_min_rtr_lft",
> -               .data           = &ipv6_devconf.accept_ra_min_rtr_lft,
> +               .procname       = "accept_ra_min_lft",
> +               .data           = &ipv6_devconf.accept_ra_min_lft,
>                 .maxlen         = sizeof(int),
>                 .mode           = 0644,
>                 .proc_handler   = proc_dointvec,
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 29ddad1c1a2f..eeb60888187f 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -1280,8 +1280,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>         if (!ndisc_parse_options(skb->dev, opt, optlen, &ndopts))
>                 return SKB_DROP_REASON_IPV6_NDISC_BAD_OPTIONS;
>
> -       lifetime = ntohs(ra_msg->icmph.icmp6_rt_lifetime);
> -
>         if (!ipv6_accept_ra(in6_dev)) {
>                 ND_PRINTK(2, info,
>                           "RA: %s, did not accept ra for dev: %s\n",
> @@ -1289,13 +1287,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>                 goto skip_linkparms;
>         }
>
> -       if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_rtr_lft) {
> -               ND_PRINTK(2, info,
> -                         "RA: router lifetime (%ds) is too short: %s\n",
> -                         lifetime, skb->dev->name);
> -               goto skip_linkparms;
> -       }
> -
>  #ifdef CONFIG_IPV6_NDISC_NODETYPE
>         /* skip link-specific parameters from interior routers */
>         if (skb->ndisc_nodetype == NDISC_NODETYPE_NODEFAULT) {
> @@ -1336,6 +1327,14 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>                 goto skip_defrtr;
>         }
>
> +       lifetime = ntohs(ra_msg->icmph.icmp6_rt_lifetime);
> +       if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_lft) {
> +               ND_PRINTK(2, info,
> +                         "RA: router lifetime (%ds) is too short: %s\n",
> +                         lifetime, skb->dev->name);
> +               goto skip_defrtr;
> +       }
> +
>         /* Do not accept RA with source-addr found on local machine unless
>          * accept_ra_from_local is set to true.
>          */
> @@ -1499,13 +1498,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>                 goto out;
>         }
>
> -       if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_rtr_lft) {
> -               ND_PRINTK(2, info,
> -                         "RA: router lifetime (%ds) is too short: %s\n",
> -                         lifetime, skb->dev->name);
> -               goto out;
> -       }
> -
>  #ifdef CONFIG_IPV6_ROUTE_INFO
>         if (!in6_dev->cnf.accept_ra_from_local &&
>             ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> @@ -1530,6 +1522,9 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>                         if (ri->prefix_len == 0 &&
>                             !in6_dev->cnf.accept_ra_defrtr)
>                                 continue;
> +                       if (ri->lifetime != 0 &&
> +                           ntohl(ri->lifetime) < in6_dev->cnf.accept_ra_min_lft)
> +                               continue;
>                         if (ri->prefix_len < in6_dev->cnf.accept_ra_rt_info_min_plen)
>                                 continue;
>                         if (ri->prefix_len > in6_dev->cnf.accept_ra_rt_info_max_plen)
> --
> 2.41.0.487.g6d72f3e995-goog

Reviewed-by: Maciej Żenczykowski <maze@google.com>

Patrick and I have spoken about this at length, and this (ignoring low
lifetime portions of the RA) seems like the best approach...

(though I will admit that I'm not super knowledgeable about IPv6 RAs
and this particular code)