netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sabrina Dubroca <sd@queasysnail.net>
To: YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com>
Cc: David Miller <davem@davemloft.net>,
	Florian Westphal <fw@strlen.de>,
	netdev@vger.kernel.org, liuhangbin@gmail.com
Subject: Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"
Date: Thu, 10 Sep 2015 11:40:37 +0200	[thread overview]
Message-ID: <20150910094037.GB22575@bistromath.redhat.com> (raw)
In-Reply-To: <55F11AAD.3030209@miraclelinux.com>

2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:
> Sabrina Dubroca wrote:
> > 2015-09-02, 16:11:10 -0700, David Miller wrote:
> >> 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.


> > 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.


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

  reply	other threads:[~2015-09-10  9:40 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-02  9:43 [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit" Sabrina Dubroca
2015-09-02 23:11 ` David Miller
2015-09-03  8:39   ` Florian Westphal
2015-09-09 10:10   ` Sabrina Dubroca
2015-09-10  2:54     ` Hangbin Liu
2015-09-10  9:19       ` Sabrina Dubroca
2015-09-11  1:29         ` Hangbin Liu
2015-09-10  5:52     ` YOSHIFUJI Hideaki
2015-09-10  9:40       ` Sabrina Dubroca [this message]
2015-09-11  3:08         ` YOSHIFUJI Hideaki
2015-09-11 10:53           ` Florian Westphal
2015-09-11 11:09             ` D.S. Ljungmark

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150910094037.GB22575@bistromath.redhat.com \
    --to=sd@queasysnail.net \
    --cc=davem@davemloft.net \
    --cc=fw@strlen.de \
    --cc=hideaki.yoshifuji@miraclelinux.com \
    --cc=liuhangbin@gmail.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).