| Message ID | 20230726230701.919212-1-prohr@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5027d54a9c30bc7ec808360378e2b4753f053f25 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: change accept_ra_min_rtr_lft to affect all RA lifetimes | expand |
On Wed, Jul 26, 2023 at 04:07:01PM -0700, Patrick Rohr 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. Hi Patrick, all, I am concerned about UAPI-breakage aspects of changing the name of a sysctl. Can we discuss that?
On Thu, Jul 27, 2023 at 2:24 PM Simon Horman <simon.horman@corigine.com> wrote: > > On Wed, Jul 26, 2023 at 04:07:01PM -0700, Patrick Rohr 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. > > Hi Patrick, all, > > I am concerned about UAPI-breakage aspects of changing the name of a sysctl. > Can we discuss that? This isn't uapi yet as it (the old name) was only merged a few days ago.
On Thu, Jul 27, 2023 at 02:38:24PM +0200, Maciej Żenczykowski wrote: > On Thu, Jul 27, 2023 at 2:24 PM Simon Horman <simon.horman@corigine.com> wrote: > > > > On Wed, Jul 26, 2023 at 04:07:01PM -0700, Patrick Rohr 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. > > > > Hi Patrick, all, > > > > I am concerned about UAPI-breakage aspects of changing the name of a sysctl. > > Can we discuss that? > > This isn't uapi yet as it (the old name) was only merged a few days ago. Ack, then I have no UAPI concern.
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)
On 7/26/23 5:07 PM, Patrick Rohr 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(-) > Reviewed-by: David Ahern <dsahern@kernel.org>
On Wed, 26 Jul 2023 16:07:01 -0700 Patrick Rohr wrote: > + if (valid_lft != 0 && valid_lft < in6_dev->cnf.accept_ra_min_lft) { > + return; > + } I'll drop the superfluous brackets when applying, hope you don't mind. Otherwise some script kiddie will send us a patch to do that :| Applied, thanks!
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 26 Jul 2023 16:07:01 -0700 you 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. > > [...] Here is the summary with links: - [net-next,v2] net: change accept_ra_min_rtr_lft to affect all RA lifetimes https://git.kernel.org/netdev/net-next/c/5027d54a9c30 You are awesome, thank you!
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)
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(-)