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

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

[Patrick Rohr]

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.

Fixes: 1671bcfd76fd ("net: add sysctl accept_ra_min_rtr_lft")
Signed-off-by: Patrick Rohr <prohr@google.com>
Cc: Maciej Żenczykowski <maze@google.com>
Cc: Lorenzo Colitti <lorenzo@google.com>
---
 Documentation/networking/ip-sysctl.rst |  7 ++++---
 include/linux/ipv6.h                   |  2 +-
 include/uapi/linux/ipv6.h              |  2 +-
 net/ipv6/addrconf.c                    | 15 +++++++++-----
 net/ipv6/ndisc.c                       | 27 +++++++++++---------------
 5 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 37603ad6126b..4ac892fa64da 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2288,11 +2288,12 @@ accept_ra_min_hop_limit - INTEGER
 
 	Default: 1
 
-accept_ra_min_rtr_lft - INTEGER
+accept_ra_min_lft - INTEGER
 	Minimum acceptable router lifetime in Router Advertisement.
 
-	RAs with a router lifetime less than this value shall be
-	ignored. RAs with a router lifetime of 0 are unaffected.
+	RAs sections with a router lifetime, PIO preferred lifetime,
+	or RIO 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..c7549bd9f1b7 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,11 @@ 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) {
+		net_info_ratelimited("addrconf: prefix option lifetime too short\n");
+		return;
+	}
+
 	/*
 	 *	Two things going on here:
 	 *	1) Add routes for on-link prefixes
@@ -5598,7 +5603,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 +6798,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

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

[David Ahern]

On 7/25/23 12:31 PM, Patrick Rohr wrote:
> @@ -2727,6 +2727,11 @@ 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) {
> +		net_info_ratelimited("addrconf: prefix option lifetime too short\n");

The error message does not really provide any value besides spamming the
logs. Similar comment applies to existing error logging in that function
too. I think a counter for invalid prefix packets would be more useful.

> +		return;
> +	}
> +
>  	/*
>  	 *	Two things going on here:
>  	 *	1) Add routes for on-link prefixes
> @@ -5598,7 +5603,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 +6798,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;
> -	}
> -

The commit mentioned in the Fixes was just applied and you are already
sending a follow up moving the same code around again.

>  #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)

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

[Patrick Rohr]

On Tue, Jul 25, 2023 at 4:28 PM David Ahern <dsahern@kernel.org> wrote:
>
> On 7/25/23 12:31 PM, Patrick Rohr wrote:
> > @@ -2727,6 +2727,11 @@ 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) {
> > +             net_info_ratelimited("addrconf: prefix option lifetime too short\n");
>
> The error message does not really provide any value besides spamming the
> logs. Similar comment applies to existing error logging in that function
> too. I think a counter for invalid prefix packets would be more useful.
>

Agreed. Let me remove the error log in this commit and clean up the
entire function in a follow up.

> The commit mentioned in the Fixes was just applied and you are already
> sending a follow up moving the same code around again.

I got feedback off of the mailing list after the patch was applied. 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 such 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. I am
sending this follow up patch now to fix 1671bcfd76fd before it is released.

>
> >  #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)
>

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

[David Ahern]

On 7/26/23 11:33 AM, Patrick Rohr wrote:
>> The commit mentioned in the Fixes was just applied and you are already
>> sending a follow up moving the same code around again.
> 
> I got feedback off of the mailing list after the patch was applied.

That offlist discussion should be summarized in the commit message (text
below?)

> 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 such 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. I am
> sending this follow up patch now to fix 1671bcfd76fd before it is released.
>