[PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.

There are several issues with this patch.
It completely cancels the security changes introduced by 6fd99094de2b
("ipv6: Don't reduce hop limit for an interface").
The current default value (min hop limit = 1) can result in the same
denial of service that 6fd99094de2b prevents, but it is hard to define
a correct and sane default value.
More generally, it is yet another IPv6 sysctl, and we already have too
many.

This was introduced to satisfy a TAHI test case which, in my opinion, is
too strict, turning the RFC's "SHOULD" into a "MUST":

    If the received Cur Hop Limit value is non-zero, the host
    SHOULD set its CurHopLimit variable to the received value.

The behavior of this sysctl is wrong in multiple ways.  Some are
fixable, but let's not rush this commit into mainline, and revert this
while we still can, then we can come up with a better solution.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 Documentation/networking/ip-sysctl.txt |  8 --------
 include/linux/ipv6.h                   |  1 -
 include/uapi/linux/ipv6.h              |  1 -
 net/ipv6/addrconf.c                    | 10 ----------
 net/ipv6/ndisc.c                       | 16 +++++++++-------
 5 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index ebe94f2cab98..c845aca413c8 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt

@@ -1371,14 +1371,6 @@ accept_ra_from_local - BOOLEAN
 	   disabled if accept_ra_from_local is disabled
                on a specific interface.
 
-accept_ra_min_hop_limit - INTEGER
-	Minimum hop limit Information in Router Advertisement.
-
-	Hop limit Information in Router Advertisement less than this
-	variable shall be ignored.
-
-	Default: 1
-
 accept_ra_pinfo - BOOLEAN
 	Learn Prefix Information in Router Advertisement.
 

diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index f1f32af6d9b9..5e6d3b8f5a42 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h

@@ -29,7 +29,6 @@ struct ipv6_devconf {
 	__s32		max_desync_factor;
 	__s32		max_addresses;
 	__s32		accept_ra_defrtr;
-	__s32		accept_ra_min_hop_limit;
 	__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 38b4fef20219..44d13121de52 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h

@@ -172,7 +172,6 @@ enum {
 	DEVCONF_ACCEPT_RA_MTU,
 	DEVCONF_STABLE_SECRET,
 	DEVCONF_USE_OIF_ADDRS_ONLY,
-	DEVCONF_ACCEPT_RA_MIN_HOP_LIMIT,
 	DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN,
 	DEVCONF_MAX
 };

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 99c0f2b843f0..c8829ce0bbe7 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -195,7 +195,6 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {
 	.max_addresses		= IPV6_MAX_ADDRESSES,
 	.accept_ra_defrtr	= 1,
 	.accept_ra_from_local	= 0,
-	.accept_ra_min_hop_limit= 1,
 	.accept_ra_pinfo	= 1,
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	.accept_ra_rtr_pref	= 1,

@@ -239,7 +238,6 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
 	.max_addresses		= IPV6_MAX_ADDRESSES,
 	.accept_ra_defrtr	= 1,
 	.accept_ra_from_local	= 0,
-	.accept_ra_min_hop_limit= 1,
 	.accept_ra_pinfo	= 1,
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	.accept_ra_rtr_pref	= 1,

@@ -4658,7 +4656,6 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
 	array[DEVCONF_MAX_DESYNC_FACTOR] = cnf->max_desync_factor;
 	array[DEVCONF_MAX_ADDRESSES] = cnf->max_addresses;
 	array[DEVCONF_ACCEPT_RA_DEFRTR] = cnf->accept_ra_defrtr;
-	array[DEVCONF_ACCEPT_RA_MIN_HOP_LIMIT] = cnf->accept_ra_min_hop_limit;
 	array[DEVCONF_ACCEPT_RA_PINFO] = cnf->accept_ra_pinfo;
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	array[DEVCONF_ACCEPT_RA_RTR_PREF] = cnf->accept_ra_rtr_pref;

@@ -5594,13 +5591,6 @@ static struct addrconf_sysctl_table
 			.proc_handler	= proc_dointvec,
 		},
 		{
-			.procname	= "accept_ra_min_hop_limit",
-			.data		= &ipv6_devconf.accept_ra_min_hop_limit,
-			.maxlen		= sizeof(int),
-			.mode		= 0644,
-			.proc_handler	= proc_dointvec,
-		},
-		{
 			.procname	= "accept_ra_pinfo",
 			.data		= &ipv6_devconf.accept_ra_pinfo,
 			.maxlen		= sizeof(int),

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 64a71354b069..fac72658bf59 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c

@@ -1235,16 +1235,18 @@ static void ndisc_router_discovery(struct sk_buff *skb)
 
 	if (rt)
 		rt6_set_expires(rt, jiffies + (HZ * lifetime));
-	if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
-	    ra_msg->icmph.icmp6_hop_limit) {
-		if (in6_dev->cnf.accept_ra_min_hop_limit <= ra_msg->icmph.icmp6_hop_limit) {
+	if (ra_msg->icmph.icmp6_hop_limit) {
+		/* Only set hop_limit on the interface if it is higher than
+		 * the current hop_limit.
+		 */
+		if (in6_dev->cnf.hop_limit < ra_msg->icmph.icmp6_hop_limit) {
 			in6_dev->cnf.hop_limit = ra_msg->icmph.icmp6_hop_limit;
-			if (rt)
-				dst_metric_set(&rt->dst, RTAX_HOPLIMIT,
-					       ra_msg->icmph.icmp6_hop_limit);
 		} else {
-			ND_PRINTK(2, warn, "RA: Got route advertisement with lower hop_limit than minimum\n");
+			ND_PRINTK(2, warn, "RA: Got route advertisement with lower hop_limit than current\n");
 		}
+		if (rt)
+			dst_metric_set(&rt->dst, RTAX_HOPLIMIT,
+				       ra_msg->icmph.icmp6_hop_limit);
 	}
 
 skip_defrtr:

-- 
2.5.1

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Wed, 2 Sep 2015 11:43:01 +0200

This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.

There are several issues with this patch.
It completely cancels the security changes introduced by 6fd99094de2b
("ipv6: Don't reduce hop limit for an interface").
The current default value (min hop limit = 1) can result in the same
denial of service that 6fd99094de2b prevents, but it is hard to define
a correct and sane default value.
More generally, it is yet another IPv6 sysctl, and we already have too
many.

This was introduced to satisfy a TAHI test case which, in my opinion, is
too strict, turning the RFC's "SHOULD" into a "MUST":

    If the received Cur Hop Limit value is non-zero, the host
    SHOULD set its CurHopLimit variable to the received value.

The behavior of this sysctl is wrong in multiple ways.  Some are
fixable, but let's not rush this commit into mainline, and revert this
while we still can, then we can come up with a better solution.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

I don't agree with this revert.

If you look at the original commit, the quoted RFC recommends adding
a configurable method to protect against this.

And that's exactly what the commit you are trying to revert is doing.

The only thing I would entertain is potentially an adjustment of the
default, working in concert with the TAHI folks to make sure their
tests still pass with any new default.

Thanks.

David Miller [off-list ref] wrote:

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Wed, 2 Sep 2015 11:43:01 +0200

quoted

This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.

There are several issues with this patch.
It completely cancels the security changes introduced by 6fd99094de2b
("ipv6: Don't reduce hop limit for an interface").
The current default value (min hop limit = 1) can result in the same
denial of service that 6fd99094de2b prevents, but it is hard to define
a correct and sane default value.
More generally, it is yet another IPv6 sysctl, and we already have too
many.

This was introduced to satisfy a TAHI test case which, in my opinion, is
too strict, turning the RFC's "SHOULD" into a "MUST":

    If the received Cur Hop Limit value is non-zero, the host
    SHOULD set its CurHopLimit variable to the received value.

The behavior of this sysctl is wrong in multiple ways.  Some are
fixable, but let's not rush this commit into mainline, and revert this
while we still can, then we can come up with a better solution.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

I don't agree with this revert.

If you look at the original commit, the quoted RFC recommends adding
a configurable method to protect against this.

Which also means it recommends a configurable method to NOT protect
against this.

Which begs the question in which scenario you would want to configure
end hosts in such a way that an RA can shrink hoplimit to values
where machines can't talk to internet hosts anymore.

The only thing I would entertain is potentially an adjustment of the
default, working in concert with the TAHI folks to make sure their
tests still pass with any new default.

So, assuming we would change the default to 64 (the hoplimit default).
Where would it make sense to reconfigure this to a lower value?

Moreover, if we would (hypothetically) assume that an administrator wants
a smaller hop limit value and has to change knob to allow e.g. min
hoplimit of 10 they might as well just change the default hoplimit value
rather than altering min hoplimit and then set it via RA...?

2015-09-02, 16:11:10 -0700, David Miller wrote:

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Wed, 2 Sep 2015 11:43:01 +0200

quoted

This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.

There are several issues with this patch.
It completely cancels the security changes introduced by 6fd99094de2b
("ipv6: Don't reduce hop limit for an interface").
The current default value (min hop limit = 1) can result in the same
denial of service that 6fd99094de2b prevents, but it is hard to define
a correct and sane default value.
More generally, it is yet another IPv6 sysctl, and we already have too
many.

This was introduced to satisfy a TAHI test case which, in my opinion, is
too strict, turning the RFC's "SHOULD" into a "MUST":

    If the received Cur Hop Limit value is non-zero, the host
    SHOULD set its CurHopLimit variable to the received value.

The behavior of this sysctl is wrong in multiple ways.  Some are
fixable, but let's not rush this commit into mainline, and revert this
while we still can, then we can come up with a better solution.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

I don't agree with this revert.

If you look at the original commit, the quoted RFC recommends adding
a configurable method to protect against this.

And that's exactly what the commit you are trying to revert is doing.

The only thing I would entertain is potentially an adjustment of the
default, working in concert with the TAHI folks to make sure their
tests still pass with any new default.

Would you agree with a default of 64, as Florian suggested?


Can we still modify the behavior of this sysctl? It's already been in
Linus's tree for a while, but if we can, I would rather restrict the
values we let the user write to accept_ra_min_hop_limit, as anything
outside [0..255] does not really make sense.

Allowing an RA to update the hop limit if

   current hop limit < RA.hop_limit < accept_ra_min_hop_limit

might also be desirable, but I'm not so sure about this case.


-- 
Sabrina

Hi Sabrina,

2015-09-09 18:10 GMT+08:00 Sabrina Dubroca [off-list ref]:

2015-09-02, 16:11:10 -0700, David Miller wrote:

quoted

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Wed, 2 Sep 2015 11:43:01 +0200

quoted

This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.

There are several issues with this patch.
It completely cancels the security changes introduced by 6fd99094de2b
("ipv6: Don't reduce hop limit for an interface").
The current default value (min hop limit = 1) can result in the same
denial of service that 6fd99094de2b prevents, but it is hard to define
a correct and sane default value.
More generally, it is yet another IPv6 sysctl, and we already have too
many.

This was introduced to satisfy a TAHI test case which, in my opinion, is
too strict, turning the RFC's "SHOULD" into a "MUST":

    If the received Cur Hop Limit value is non-zero, the host
    SHOULD set its CurHopLimit variable to the received value.

The behavior of this sysctl is wrong in multiple ways.  Some are
fixable, but let's not rush this commit into mainline, and revert this
while we still can, then we can come up with a better solution.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

I don't agree with this revert.

If you look at the original commit, the quoted RFC recommends adding
a configurable method to protect against this.

And that's exactly what the commit you are trying to revert is doing.

The only thing I would entertain is potentially an adjustment of the
default, working in concert with the TAHI folks to make sure their
tests still pass with any new default.

Would you agree with a default of 64, as Florian suggested?

Set default to 64 make sense to me. It can make the system more security
by default.

Can we still modify the behavior of this sysctl? It's already been in
Linus's tree for a while, but if we can, I would rather restrict the
values we let the user write to accept_ra_min_hop_limit, as anything
outside [0..255] does not really make sense.

Yes, so the checked if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
ra_msg->icmph.icmp6_hop_limit)  make sure we only update the value between
[1..255].

Allowing an RA to update the hop limit if

   current hop limit < RA.hop_limit < accept_ra_min_hop_limit

might also be desirable, but I'm not so sure about this case.

Yes, and we also should allow an RA to update the hop limit if

accept_ra_min_hop_limit <= RA.hop_limit < current hop limit

e.g accept_ra_min_hop_limit = RA.hop_limit =64, current hop limit = 128

Thanks
Hangbin Liu

Sabrina Dubroca wrote:

2015-09-02, 16:11:10 -0700, David Miller wrote:

quoted

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Wed, 2 Sep 2015 11:43:01 +0200

quoted

This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.

There are several issues with this patch.
It completely cancels the security changes introduced by 6fd99094de2b
("ipv6: Don't reduce hop limit for an interface").
The current default value (min hop limit = 1) can result in the same
denial of service that 6fd99094de2b prevents, but it is hard to define
a correct and sane default value.
More generally, it is yet another IPv6 sysctl, and we already have too
many.

This was introduced to satisfy a TAHI test case which, in my opinion, is
too strict, turning the RFC's "SHOULD" into a "MUST":

    If the received Cur Hop Limit value is non-zero, the host
    SHOULD set its CurHopLimit variable to the received value.

The behavior of this sysctl is wrong in multiple ways.  Some are
fixable, but let's not rush this commit into mainline, and revert this
while we still can, then we can come up with a better solution.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

I don't agree with this revert.

If you look at the original commit, the quoted RFC recommends adding
a configurable method to protect against this.

And that's exactly what the commit you are trying to revert is doing.

The only thing I would entertain is potentially an adjustment of the
default, working in concert with the TAHI folks to make sure their
tests still pass with any new default.

Would you agree with a default of 64, as Florian suggested?

1 was chosen to restore our behavior before introduction of current
hoplimit check.  I am not in favor of changing that value.

Plus, 64 is too restrictive and 32 would be enough for global
internet, IMHO.

Can we still modify the behavior of this sysctl? It's already been in
Linus's tree for a while, but if we can, I would rather restrict the
values we let the user write to accept_ra_min_hop_limit, as anything
outside [0..255] does not really make sense.

[1..256], maybe, but it is not harmful to set outside the range.
0 is always ignored. If it is set to 256 or more, the option is
completely ignored.

Allowing an RA to update the hop limit if

   current hop limit < RA.hop_limit < accept_ra_min_hop_limit

might also be desirable, but I'm not so sure about this case.

It might be... byt I don't think it is a good idea since it becomes
more complex.

-- 
Hideaki Yoshifuji [off-list ref]
Technical Division, MIRACLE LINUX CORPORATION

Hello,

2015-09-10, 10:54:38 +0800, Hangbin Liu wrote:

quoted

Can we still modify the behavior of this sysctl? It's already been in
Linus's tree for a while, but if we can, I would rather restrict the
values we let the user write to accept_ra_min_hop_limit, as anything
outside [0..255] does not really make sense.

Yes, so the checked if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
ra_msg->icmph.icmp6_hop_limit)  make sure we only update the value between
[1..255].

I was thinking of returning -EINVAL when the user tries to set it to
300, using proc_dointvec_minmax.

quoted

Allowing an RA to update the hop limit if

   current hop limit < RA.hop_limit < accept_ra_min_hop_limit

might also be desirable, but I'm not so sure about this case.

Yes, and we also should allow an RA to update the hop limit if

accept_ra_min_hop_limit <= RA.hop_limit < current hop limit

e.g accept_ra_min_hop_limit = RA.hop_limit =64, current hop limit = 128

Yes, that's what we're doing at the moment, and I would leave it as is.


Thanks,

-- 
Sabrina

2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:

Sabrina Dubroca wrote:

quoted

2015-09-02, 16:11:10 -0700, David Miller wrote:

quoted

The only thing I would entertain is potentially an adjustment of the
default, working in concert with the TAHI folks to make sure their
tests still pass with any new default.

Would you agree with a default of 64, as Florian suggested?

1 was chosen to restore our behavior before introduction of current
hoplimit check.  I am not in favor of changing that value.

But our old behavior had a security issue, which is why the >= current
check was introduced.

Plus, 64 is too restrictive and 32 would be enough for global
internet, IMHO.

I guess I could live with that, if 32 is indeed enough for everybody.

quoted

Can we still modify the behavior of this sysctl? It's already been in
Linus's tree for a while, but if we can, I would rather restrict the
values we let the user write to accept_ra_min_hop_limit, as anything
outside [0..255] does not really make sense.

[1..256], maybe, but it is not harmful to set outside the range.
0 is always ignored. If it is set to 256 or more, the option is
completely ignored.

Not harmful, but maybe slightly misleading, and requires the "< 256"
check when processing a RA.

quoted

Allowing an RA to update the hop limit if

   current hop limit < RA.hop_limit < accept_ra_min_hop_limit

might also be desirable, but I'm not so sure about this case.

It might be... byt I don't think it is a good idea since it becomes
more complex.

A bit more complex, yes.  But I don't think this should hold us back
if it results in better behavior.


Thanks,

-- 
Sabrina

Hi Sabrina,

2015-09-10 17:19 GMT+08:00 Sabrina Dubroca [off-list ref]:

Hello,

2015-09-10, 10:54:38 +0800, Hangbin Liu wrote:

quoted

quoted

Can we still modify the behavior of this sysctl? It's already been in
Linus's tree for a while, but if we can, I would rather restrict the
values we let the user write to accept_ra_min_hop_limit, as anything
outside [0..255] does not really make sense.

Yes, so the checked if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
ra_msg->icmph.icmp6_hop_limit)  make sure we only update the value between
[1..255].

I was thinking of returning -EINVAL when the user tries to set it to
300, using proc_dointvec_minmax.

I'm OK with this. But we can also set cnf.hop_limit to 300,  if add check for
accept_ra_min_hop_limit, we'd better check all hop_limit values.

Thanks
Hangbin

Sabrina Dubroca wrote:

2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:

quoted

Sabrina Dubroca wrote:

quoted

2015-09-02, 16:11:10 -0700, David Miller wrote:

quoted

The only thing I would entertain is potentially an adjustment of the
default, working in concert with the TAHI folks to make sure their
tests still pass with any new default.

Would you agree with a default of 64, as Florian suggested?

1 was chosen to restore our behavior before introduction of current
hoplimit check.  I am not in favor of changing that value.

But our old behavior had a security issue, which is why the >= current
check was introduced.

We have the knob to "protect" ourselves now but it has drawbacks no to
accept lower values than specified.  We can never have ultimate default
for everybody.  The knob might "mitigate" the issue but once we have
any rouge routers on our L2, we lose anyway.  So, I do want to keep it
as-is not to change our traditional behavior.

quoted

Plus, 64 is too restrictive and 32 would be enough for global
internet, IMHO.

I guess I could live with that, if 32 is indeed enough for everybody.

quoted

quoted

Can we still modify the behavior of this sysctl? It's already been in
Linus's tree for a while, but if we can, I would rather restrict the
values we let the user write to accept_ra_min_hop_limit, as anything
outside [0..255] does not really make sense.

[1..256], maybe, but it is not harmful to set outside the range.
0 is always ignored. If it is set to 256 or more, the option is
completely ignored.

Not harmful, but maybe slightly misleading, and requires the "< 256"
check when processing a RA.

The "Cur Hop Limit" field is 8bit long...

quoted

quoted

Allowing an RA to update the hop limit if

   current hop limit < RA.hop_limit < accept_ra_min_hop_limit

might also be desirable, but I'm not so sure about this case.

It might be... byt I don't think it is a good idea since it becomes
more complex.

A bit more complex, yes.  But I don't think this should hold us back
if it results in better behavior.


Thanks,

-- 
Hideaki Yoshifuji [off-list ref]
Technical Division, MIRACLE LINUX CORPORATION

YOSHIFUJI Hideaki [off-list ref] wrote:

Sabrina Dubroca wrote:

quoted

2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:

quoted

Sabrina Dubroca wrote:

quoted

Would you agree with a default of 64, as Florian suggested?

1 was chosen to restore our behavior before introduction of current
hoplimit check.  I am not in favor of changing that value.

But our old behavior had a security issue, which is why the >= current
check was introduced.

We have the knob to "protect" ourselves now but it has drawbacks no to
accept lower values than specified.  We can never have ultimate default
for everybody.  The knob might "mitigate" the issue but once we have
any rouge routers on our L2, we lose anyway.  So, I do want to keep it
as-is not to change our traditional behavior.

If that argument is brough forward (and it's a good point!), then the
entire case for rejecting 'low' hoplimit values in first place becomes moot.

If this is an important security issue, then either the sysctl has to be
removed or the default raised to some 'safe' value (32, for example).

If its not a security issue -- and it isn't if we think "1" is a good
default choice -- then we should seriously consider reverting both
the added sysctl and the 'original' commit (6fd99094de2b; "ipv6: Don't
reduce hop limit for an interface").

Cheers,
Florian

On 11/09/15 12:53, Florian Westphal wrote:

YOSHIFUJI Hideaki [off-list ref] wrote:

quoted

Sabrina Dubroca wrote:

quoted

2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:

quoted

Sabrina Dubroca wrote:

quoted

Would you agree with a default of 64, as Florian suggested?

1 was chosen to restore our behavior before introduction of current
hoplimit check.  I am not in favor of changing that value.

But our old behavior had a security issue, which is why the >= current
check was introduced.

We have the knob to "protect" ourselves now but it has drawbacks no to
accept lower values than specified.  We can never have ultimate default
for everybody.  The knob might "mitigate" the issue but once we have
any rouge routers on our L2, we lose anyway.  So, I do want to keep it
as-is not to change our traditional behavior.

If that argument is brough forward (and it's a good point!), then the
entire case for rejecting 'low' hoplimit values in first place becomes moot.

If this is an important security issue, then either the sysctl has to be
removed or the default raised to some 'safe' value (32, for example).

If its not a security issue -- and it isn't if we think "1" is a good
default choice -- then we should seriously consider reverting both
the added sysctl and the 'original' commit (6fd99094de2b; "ipv6: Don't
reduce hop limit for an interface").

The most common use-case for this is public WiFi.  So far, a negible
amount of access points have even remote ability to filter "unwanted" L2
traffic.

The fact that a single, empty RA packet with a hop limit of 2 will take
down your entire ipv6, even if your infrastructure uses DHCPv6 for
addressing is problematic.

There are scenarios where an L2 agent can push a link-local or
Peer-to-peer routes with a low hoplimit. These routes would then lower
the interface-level hop limit to something that breaks your other routing.

Personally, I think the concept of hop-limit being per interface in IPv6
is disasterously stupid, but I'm not arguing against the RFC there.

//D.S.



-- 
8362 CB14 98AD 11EF CEB6  FA81 FCC3 7674 449E 3CFC