pynfs patch

pynfs patch

[Tigran Mkrtchyan]

Hi Bruce, 

this patch for pynfs I have received as github pull request

Tigran.

[PATCH] Reduce the probability to get port collision when asking for secure port. port collision = same port allocated to 2 different clients trying to connect to the same address.

[Tigran Mkrtchyan]

From: Gil Amsalem <gil.amsalem@primarydata.com>

Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
---
 rpc/rpc.py | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/rpc/rpc.py b/rpc/rpc.py
index 1a3ca38..8d88c62 100644
--- a/rpc/rpc.py
+++ b/rpc/rpc.py
@@ -824,23 +824,19 @@ class ConnectionHandler(object):
         defer.wait()
         return pipe
 
-    def bindsocket(self, s, port=1):
+    def bindsocket(self, s, start_port=1):
         """Scan up through ports, looking for one we can bind to"""
         # This is necessary when we need to use a 'secure' port
-        using = port
-        while 1:
+        valid_ports = range(start_port, 1024)
+        random.shuffle(valid_ports)
+        for port in valid_ports:
             try:
-                s.bind(('', using))
+                s.bind(('', port))
                 return
             except socket.error, why:
-                if why[0] == errno.EADDRINUSE:
-                    using += 1
-                    if port < 1024 <= using:
-                        # If we ask for a secure port, make sure we don't
-                        # silently bind to a non-secure one
-                        raise
-                else:
+                if why[0] != errno.EADDRINUSE:
                     raise
+        raise Exception('failed to find available secure port')
 
 
     def expose(self, address, af, safe=True):
-- 
2.5.0

Re: [PATCH] Reduce the probability to get port collision when asking for secure port. port collision = same port allocated to 2 different clients trying to connect to the same address.

[J. Bruce Fields]

Sorry for a slow response.

I'm confused.  You're worried about two pynfs instances binding to the
same local port at the same time?  The kernel should prevent that,
shouldn't it?

--b.

On Tue, Dec 08, 2015 at 08:27:34AM +0100, Tigran Mkrtchyan wrote:
> From: Gil Amsalem <gil.amsalem@primarydata.com>
> 
> Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
> ---
>  rpc/rpc.py | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/rpc/rpc.py b/rpc/rpc.py
> index 1a3ca38..8d88c62 100644
> --- a/rpc/rpc.py
> +++ b/rpc/rpc.py
> @@ -824,23 +824,19 @@ class ConnectionHandler(object):
>          defer.wait()
>          return pipe
>  
> -    def bindsocket(self, s, port=1):
> +    def bindsocket(self, s, start_port=1):
>          """Scan up through ports, looking for one we can bind to"""
>          # This is necessary when we need to use a 'secure' port
> -        using = port
> -        while 1:
> +        valid_ports = range(start_port, 1024)
> +        random.shuffle(valid_ports)
> +        for port in valid_ports:
>              try:
> -                s.bind(('', using))
> +                s.bind(('', port))
>                  return
>              except socket.error, why:
> -                if why[0] == errno.EADDRINUSE:
> -                    using += 1
> -                    if port < 1024 <= using:
> -                        # If we ask for a secure port, make sure we don't
> -                        # silently bind to a non-secure one
> -                        raise
> -                else:
> +                if why[0] != errno.EADDRINUSE:
>                      raise
> +        raise Exception('failed to find available secure port')
>  
>  
>      def expose(self, address, af, safe=True):
> -- 
> 2.5.0

Re: [PATCH] Reduce the probability to get port collision when asking for secure port. port collision = same port allocated to 2 different clients trying to connect to the same address.

[J. Bruce Fields]

On Wed, Jan 13, 2016 at 10:59:25AM +0200, Gil Amsalem wrote:
> Hi,
> 
> I don't know how, but I faced this issue where the *bindsocket* method
> bounded port 1 for two different threads.
> Not sure how could it happen.
> After my change, I got random ports of course, and the problem was solved.

Huh.  OK, well I'd like to see how to reproduce the problem and
understand what was going on.  Maybe I'm just missing something obvious
but I didn't think it should be possible for two bind()s to the same
port to succeed simultaneously.

--b.

Re: [PATCH] Reduce the probability to get port collision when asking for secure port. port collision = same port allocated to 2 different clients trying to connect to the same address.

[Jeff Layton]

On Wed, 13 Jan 2016 11:34:25 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jan 13, 2016 at 10:59:25AM +0200, Gil Amsalem wrote:
> > Hi,
> > 
> > I don't know how, but I faced this issue where the *bindsocket* method
> > bounded port 1 for two different threads.
> > Not sure how could it happen.
> > After my change, I got random ports of course, and the problem was solved.  
> 
> Huh.  OK, well I'd like to see how to reproduce the problem and
> understand what was going on.  Maybe I'm just missing something obvious
> but I didn't think it should be possible for two bind()s to the same
> port to succeed simultaneously.
> 

It is possible if you set SO_REUSEPORT on the socket. Does pynfs do
that? Might be interesting to strace the program and see if it sets
that option on the socket to confirm...

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] Reduce the probability to get port collision when asking for secure port. port collision = same port allocated to 2 different clients trying to connect to the same address.

[J. Bruce Fields]

On Wed, Jan 13, 2016 at 09:00:08PM -0500, Jeff Layton wrote:
> On Wed, 13 Jan 2016 11:34:25 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Wed, Jan 13, 2016 at 10:59:25AM +0200, Gil Amsalem wrote:
> > > Hi,
> > > 
> > > I don't know how, but I faced this issue where the *bindsocket* method
> > > bounded port 1 for two different threads.
> > > Not sure how could it happen.
> > > After my change, I got random ports of course, and the problem was solved.  
> > 
> > Huh.  OK, well I'd like to see how to reproduce the problem and
> > understand what was going on.  Maybe I'm just missing something obvious
> > but I didn't think it should be possible for two bind()s to the same
> > port to succeed simultaneously.
> > 
> 
> It is possible if you set SO_REUSEPORT on the socket. Does pynfs do
> that? Might be interesting to strace the program and see if it sets
> that option on the socket to confirm...

This is just socket.bind() with no special options so it'd be pretty
weird if it was doing an SO_REUSEPORT.  Not a bad idea to check that
with an strace if the problem shows up again, though.

Anyway, dropping the patch for now.

--b.

[PATCH] Reduce the probability to get port collision when asking for secure port. port collision = same port allocated to 2 different clients trying to connect to the same address.

[Tigran Mkrtchyan]

From: Gil Amsalem <gil.amsalem@primarydata.com>

Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
---
 rpc/rpc.py | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/rpc/rpc.py b/rpc/rpc.py
index 1a3ca38..8d88c62 100644
--- a/rpc/rpc.py
+++ b/rpc/rpc.py
@@ -824,23 +824,19 @@ class ConnectionHandler(object):
         defer.wait()
         return pipe
 
-    def bindsocket(self, s, port=1):
+    def bindsocket(self, s, start_port=1):
         """Scan up through ports, looking for one we can bind to"""
         # This is necessary when we need to use a 'secure' port
-        using = port
-        while 1:
+        valid_ports = range(start_port, 1024)
+        random.shuffle(valid_ports)
+        for port in valid_ports:
             try:
-                s.bind(('', using))
+                s.bind(('', port))
                 return
             except socket.error, why:
-                if why[0] == errno.EADDRINUSE:
-                    using += 1
-                    if port < 1024 <= using:
-                        # If we ask for a secure port, make sure we don't
-                        # silently bind to a non-secure one
-                        raise
-                else:
+                if why[0] != errno.EADDRINUSE:
                     raise
+        raise Exception('failed to find available secure port')
 
 
     def expose(self, address, af, safe=True):
-- 
2.5.0