[patch] fix statd -n

[patch] fix statd -n

[Janne Karhunen]

[-- Attachment #1: Type: text/plain, Size: 204 bytes --]

Hi all,

Apparently lockd does not expect statd to be used with -n
switch: statd is expected to bind loopback, always. Attached
patches show one (IPv4 specific) way of fixing it. Comments?


-- 
// Janne

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: statd-rmtcall.patch --]
[-- Type: text/x-patch; name=statd-rmtcall.patch, Size: 1404 bytes --]

--- rmtcall.c.org	2008-04-14 10:53:30.000000000 -0400
+++ rmtcall.c	2008-04-14 13:01:27.000000000 -0400
@@ -37,6 +37,7 @@
 #include <netdb.h>
 #include <string.h>
 #include <unistd.h>
+#include <errno.h>
 #ifdef HAVE_IFADDRS_H
 #include <ifaddrs.h>
 #endif /* HAVE_IFADDRS_H */
@@ -54,6 +55,34 @@
 
 static unsigned long	xid = 0;	/* RPC XID counter */
 static int		sockfd = -1;	/* notify socket */
+static int              ifset = 0;
+
+/*
+ * Notify lockd of non-standard binding 
+ */
+inline void
+nlm_nsm_set(unsigned int addr)
+{
+        ssize_t sz = 0;
+        char buf[20];
+        int fd;
+
+        if ( ifset )
+                return ;
+
+        sprintf (buf,"%u",addr);
+
+        fd = open ("/proc/sys/fs/nfs/nlm_nsm_interface", O_RDWR);
+        if (fd > 0) {
+                sz = write (fd,buf,strlen((char*)buf));
+                if ( sz == -1 )
+                        note(N_CRIT, "statd: write: %s\n", strerror(errno));
+                close (fd);
+        } else
+                note(N_CRIT, "statd: -n was specified with with no kernel support?\n");
+
+        ifset = 1;
+}
 
 /*
  * Initialize callback socket
@@ -85,6 +114,7 @@ statd_get_socket(int port)
 		struct hostent *hp = gethostbyname(MY_NAME);
 		if (hp)
 			sin.sin_addr = *(struct in_addr *) hp->h_addr;
+		nlm_nsm_set ((unsigned int)sin.sin_addr.s_addr);
 	}
 	if (port != 0) {
 		sin.sin_port = htons(port);

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: lockd-nsmif.patch --]
[-- Type: text/x-patch; name=lockd-nsmif.patch, Size: 2713 bytes --]

diff -Naurp lockd.org/svc4proc.c lockd/svc4proc.c
--- lockd.org/svc4proc.c	2008-04-14 10:58:29.000000000 -0400
+++ lockd/svc4proc.c	2008-04-14 12:19:04.000000000 -0400
@@ -21,6 +21,8 @@
 
 #define NLMDBG_FACILITY		NLMDBG_CLIENT
 
+extern unsigned int nlm_nsm_interface;
+
 /*
  * Obtain client and file from arguments
  */
@@ -430,8 +432,9 @@ nlm4svc_proc_sm_notify(struct svc_rqst *
 	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
 
 	dprintk("lockd: SM_NOTIFY     called\n");
-	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
-	 || ntohs(saddr.sin_port) >= 1024) {
+	if (((saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK))
+	 && (nlm_nsm_interface && (saddr.sin_addr.s_addr != htonl(nlm_nsm_interface))))
+	 || (ntohs(saddr.sin_port) >= 1024)) {
 		char buf[RPC_MAX_ADDRBUFLEN];
 		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
 				svc_print_addr(rqstp, buf, sizeof(buf)));
diff -Naurp lockd.org/svc.c lockd/svc.c
--- lockd.org/svc.c	2008-04-14 10:58:29.000000000 -0400
+++ lockd/svc.c	2008-04-14 12:19:04.000000000 -0400
@@ -64,6 +64,7 @@ static unsigned long		nlm_grace_period;
 static unsigned long		nlm_timeout = LOCKD_DFLT_TIMEO;
 static int			nlm_udpport, nlm_tcpport;
 int				nsm_use_hostnames = 0;
+unsigned int			nlm_nsm_interface = 0;
 
 /*
  * Constants needed for the sysctl interface.
@@ -425,6 +426,14 @@ static ctl_table nlm_sysctls[] = {
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec,
 	},
+        {
+                .ctl_name       = CTL_UNNUMBERED,
+                .procname       = "nlm_nsm_interface",
+                .data           = &nlm_nsm_interface,
+                .maxlen         = sizeof(int),
+                .mode           = 0644,
+                .proc_handler   = &proc_dointvec,
+        },
 	{ .ctl_name = 0 }
 };
 
diff -Naurp lockd.org/svcproc.c lockd/svcproc.c
--- lockd.org/svcproc.c	2008-04-14 10:58:29.000000000 -0400
+++ lockd/svcproc.c	2008-04-14 12:19:04.000000000 -0400
@@ -21,6 +21,8 @@
 
 #define NLMDBG_FACILITY		NLMDBG_CLIENT
 
+extern unsigned int nlm_nsm_interface;
+
 #ifdef CONFIG_LOCKD_V4
 static __be32
 cast_to_nlm(__be32 status, u32 vers)
@@ -462,8 +464,9 @@ nlmsvc_proc_sm_notify(struct svc_rqst *r
 	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
 
 	dprintk("lockd: SM_NOTIFY     called\n");
-	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
-	 || ntohs(saddr.sin_port) >= 1024) {
+	if (((saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK))
+	 && (nlm_nsm_interface && (saddr.sin_addr.s_addr != htonl(nlm_nsm_interface))))
+	 || (ntohs(saddr.sin_port) >= 1024)) {
 		char buf[RPC_MAX_ADDRBUFLEN];
 		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
 				svc_print_addr(rqstp, buf, sizeof(buf)));

Re: [patch] fix statd -n

[J. Bruce Fields]

On Thu, Apr 17, 2008 at 12:38:43PM -0400, Janne Karhunen wrote:
> Apparently lockd does not expect statd to be used with -n
> switch: statd is expected to bind loopback, always. Attached
> patches show one (IPv4 specific) way of fixing it. Comments?

Maybe statd really should always bind to the loopback interface?  Is
there any reason not to?

>From a quick look at the current nfs-utils code: it looks like the -n
option only affects the operation of the sm-notify program that's called
on boot to notify peer statd's?  I'm a little confused.  (What version
of nfs-utils are you working from?)

--b.

> 
> 
> -- 
> // Janne

> --- rmtcall.c.org	2008-04-14 10:53:30.000000000 -0400
> +++ rmtcall.c	2008-04-14 13:01:27.000000000 -0400
> @@ -37,6 +37,7 @@
>  #include <netdb.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <errno.h>
>  #ifdef HAVE_IFADDRS_H
>  #include <ifaddrs.h>
>  #endif /* HAVE_IFADDRS_H */
> @@ -54,6 +55,34 @@
>  
>  static unsigned long	xid = 0;	/* RPC XID counter */
>  static int		sockfd = -1;	/* notify socket */
> +static int              ifset = 0;
> +
> +/*
> + * Notify lockd of non-standard binding 
> + */
> +inline void
> +nlm_nsm_set(unsigned int addr)
> +{
> +        ssize_t sz = 0;
> +        char buf[20];
> +        int fd;
> +
> +        if ( ifset )
> +                return ;
> +
> +        sprintf (buf,"%u",addr);
> +
> +        fd = open ("/proc/sys/fs/nfs/nlm_nsm_interface", O_RDWR);
> +        if (fd > 0) {
> +                sz = write (fd,buf,strlen((char*)buf));
> +                if ( sz == -1 )
> +                        note(N_CRIT, "statd: write: %s\n", strerror(errno));
> +                close (fd);
> +        } else
> +                note(N_CRIT, "statd: -n was specified with with no kernel support?\n");
> +
> +        ifset = 1;
> +}
>  
>  /*
>   * Initialize callback socket
> @@ -85,6 +114,7 @@ statd_get_socket(int port)
>  		struct hostent *hp = gethostbyname(MY_NAME);
>  		if (hp)
>  			sin.sin_addr = *(struct in_addr *) hp->h_addr;
> +		nlm_nsm_set ((unsigned int)sin.sin_addr.s_addr);
>  	}
>  	if (port != 0) {
>  		sin.sin_port = htons(port);

> diff -Naurp lockd.org/svc4proc.c lockd/svc4proc.c
> --- lockd.org/svc4proc.c	2008-04-14 10:58:29.000000000 -0400
> +++ lockd/svc4proc.c	2008-04-14 12:19:04.000000000 -0400
> @@ -21,6 +21,8 @@
>  
>  #define NLMDBG_FACILITY		NLMDBG_CLIENT
>  
> +extern unsigned int nlm_nsm_interface;
> +
>  /*
>   * Obtain client and file from arguments
>   */
> @@ -430,8 +432,9 @@ nlm4svc_proc_sm_notify(struct svc_rqst *
>  	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>  
>  	dprintk("lockd: SM_NOTIFY     called\n");
> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
> -	 || ntohs(saddr.sin_port) >= 1024) {
> +	if (((saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK))
> +	 && (nlm_nsm_interface && (saddr.sin_addr.s_addr != htonl(nlm_nsm_interface))))
> +	 || (ntohs(saddr.sin_port) >= 1024)) {
>  		char buf[RPC_MAX_ADDRBUFLEN];
>  		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>  				svc_print_addr(rqstp, buf, sizeof(buf)));
> diff -Naurp lockd.org/svc.c lockd/svc.c
> --- lockd.org/svc.c	2008-04-14 10:58:29.000000000 -0400
> +++ lockd/svc.c	2008-04-14 12:19:04.000000000 -0400
> @@ -64,6 +64,7 @@ static unsigned long		nlm_grace_period;
>  static unsigned long		nlm_timeout = LOCKD_DFLT_TIMEO;
>  static int			nlm_udpport, nlm_tcpport;
>  int				nsm_use_hostnames = 0;
> +unsigned int			nlm_nsm_interface = 0;
>  
>  /*
>   * Constants needed for the sysctl interface.
> @@ -425,6 +426,14 @@ static ctl_table nlm_sysctls[] = {
>  		.mode		= 0644,
>  		.proc_handler	= &proc_dointvec,
>  	},
> +        {
> +                .ctl_name       = CTL_UNNUMBERED,
> +                .procname       = "nlm_nsm_interface",
> +                .data           = &nlm_nsm_interface,
> +                .maxlen         = sizeof(int),
> +                .mode           = 0644,
> +                .proc_handler   = &proc_dointvec,
> +        },
>  	{ .ctl_name = 0 }
>  };
>  
> diff -Naurp lockd.org/svcproc.c lockd/svcproc.c
> --- lockd.org/svcproc.c	2008-04-14 10:58:29.000000000 -0400
> +++ lockd/svcproc.c	2008-04-14 12:19:04.000000000 -0400
> @@ -21,6 +21,8 @@
>  
>  #define NLMDBG_FACILITY		NLMDBG_CLIENT
>  
> +extern unsigned int nlm_nsm_interface;
> +
>  #ifdef CONFIG_LOCKD_V4
>  static __be32
>  cast_to_nlm(__be32 status, u32 vers)
> @@ -462,8 +464,9 @@ nlmsvc_proc_sm_notify(struct svc_rqst *r
>  	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>  
>  	dprintk("lockd: SM_NOTIFY     called\n");
> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
> -	 || ntohs(saddr.sin_port) >= 1024) {
> +	if (((saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK))
> +	 && (nlm_nsm_interface && (saddr.sin_addr.s_addr != htonl(nlm_nsm_interface))))
> +	 || (ntohs(saddr.sin_port) >= 1024)) {
>  		char buf[RPC_MAX_ADDRBUFLEN];
>  		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>  				svc_print_addr(rqstp, buf, sizeof(buf)));

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, Apr 18, 2008 at 1:36 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > Apparently lockd does not expect statd to be used with -n
>  > switch: statd is expected to bind loopback, always. Attached
>  > patches show one (IPv4 specific) way of fixing it. Comments?
>
>  Maybe statd really should always bind to the loopback interface?  Is
>  there any reason not to?

In my case I used -n to push NSM/NLM comms to
clusters specific NFS service address (that just
happens to migrate all around). -n is the only way
to do this: otherwise it will bind to INADDR_ANY
on node that has gazillion interfaces/addresses..

That, and the original reasoning for the option is
said in statd code:

        /*
         * If a local hostname is given (-n option to statd), bind to
the address
         * specified. This is required to support clients that ignore
the mon_name in
         * the statd protocol but use the source address from the
request packet.
         */


>  From a quick look at the current nfs-utils code: it looks like the -n
>  option only affects the operation of the sm-notify program that's called
>  on boot to notify peer statd's?  I'm a little confused.  (What version
>  of nfs-utils are you working from?)

Right. 1.0.8 -> 1.0.12, it's valid for all.


-- 
// Janne

Re: [patch] fix statd -n

[Wendy Cheng]

J. Bruce Fields wrote:
> On Thu, Apr 17, 2008 at 12:38:43PM -0400, Janne Karhunen wrote:
>   
>> Apparently lockd does not expect statd to be used with -n
>> switch: statd is expected to bind loopback, always. Attached
>> patches show one (IPv4 specific) way of fixing it. Comments?
>>     
>
> Maybe statd really should always bind to the loopback interface?  Is
> there any reason not to?
>   

This patch is very similar to what we had about one year ago for cluster 
lock failover... Maybe it is about time to revisit the issues ?

-- Wendy

Re: [patch] fix statd -n

[J. Bruce Fields]

On Fri, Apr 18, 2008 at 02:11:55PM -0400, Janne Karhunen wrote:
> On Fri, Apr 18, 2008 at 1:36 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> >  > Apparently lockd does not expect statd to be used with -n
> >  > switch: statd is expected to bind loopback, always. Attached
> >  > patches show one (IPv4 specific) way of fixing it. Comments?
> >
> >  Maybe statd really should always bind to the loopback interface?  Is
> >  there any reason not to?
> 
> In my case I used -n to push NSM/NLM comms to
> clusters specific NFS service address (that just
> happens to migrate all around). -n is the only way
> to do this: otherwise it will bind to INADDR_ANY
> on node that has gazillion interfaces/addresses..
> 
> That, and the original reasoning for the option is
> said in statd code:
> 
>         /*
>          * If a local hostname is given (-n option to statd), bind to
> the address
>          * specified. This is required to support clients that ignore
> the mon_name in
>          * the statd protocol but use the source address from the
> request packet.
>          */

Weird--I dont' see that comment.

> >  From a quick look at the current nfs-utils code: it looks like the -n
> >  option only affects the operation of the sm-notify program that's called
> >  on boot to notify peer statd's?  I'm a little confused.  (What version
> >  of nfs-utils are you working from?)
> 
> Right. 1.0.8 -> 1.0.12, it's valid for all.

Oh, OK.  Looks like the change was made for 1.1.0; do nfs-utils more
recent than that work for you?

--b.

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, Apr 18, 2008 at 2:25 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  >         /*
>  >          * If a local hostname is given (-n option to statd), bind to
>  > the address
>  >          * specified. This is required to support clients that ignore
>  > the mon_name in
>  >          * the statd protocol but use the source address from the
>  > request packet.
>  >          */
>
>  Weird--I dont' see that comment.

All original nfs-utils-1.0.12 tarball; indeed strange.

But to be honest, I don't instantly understand how
this would work without -n at all (apart blind luck).
INADDR_ANY has no guarantees..


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Fri, Apr 18, 2008 at 02:31:40PM -0400, Janne Karhunen wrote:
> On Fri, Apr 18, 2008 at 2:25 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> >  >         /*
> >  >          * If a local hostname is given (-n option to statd), bind to
> >  > the address
> >  >          * specified. This is required to support clients that ignore
> >  > the mon_name in
> >  >          * the statd protocol but use the source address from the
> >  > request packet.
> >  >          */
> >
> >  Weird--I dont' see that comment.
> 
> All original nfs-utils-1.0.12 tarball; indeed strange.

I was looking at recent git.  Note the most recent tarball on
sourceforge.net is 1.1.2, and that has the relevant changes.

--b.

> 
> But to be honest, I don't instantly understand how
> this would work without -n at all (apart blind luck).
> INADDR_ANY has no guarantees..
> 
> 
> -- 
> // Janne

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, Apr 18, 2008 at 2:34 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > >  Weird--I dont' see that comment.
>  >
>  > All original nfs-utils-1.0.12 tarball; indeed strange.
>
>  I was looking at recent git.  Note the most recent tarball on
>  sourceforge.net is 1.1.2, and that has the relevant changes.

So what has been done in 1.1.2 is that the
-n binding has been removed completely.
It would _probably_ work, but only if I'm lucky.

This way it will not only try to guess the interface
to be used (and may or may not get it right). That,
and it will expose NFS services on interfaces
that should not need them.

I'll rework the patch to 1.1.2 to see how well
it fits in.


-- 
// Janne

Re: [patch] fix statd -n

[Janne Karhunen]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

On Fri, Apr 18, 2008 at 2:34 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  I was looking at recent git.  Note the most recent tarball on
>  sourceforge.net is 1.1.2, and that has the relevant changes.

There you go, against 1.1.2.


-- 
// Janne

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rmtcall-nsmif.patch --]
[-- Type: text/x-patch; name=rmtcall-nsmif.patch, Size: 1820 bytes --]

--- rmtcall.c.org	2008-04-18 15:41:16.000000000 -0400
+++ rmtcall.c	2008-04-18 15:40:12.000000000 -0400
@@ -27,6 +27,7 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <sys/time.h>
+#include <sys/stat.h>
 #include <netinet/in.h>
 #include <net/if.h>
 #include <arpa/inet.h>
@@ -37,6 +38,9 @@
 #include <netdb.h>
 #include <string.h>
 #include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <netdb.h>
 #ifdef HAVE_IFADDRS_H
 #include <ifaddrs.h>
 #endif /* HAVE_IFADDRS_H */
@@ -54,6 +58,34 @@
 
 static unsigned long	xid = 0;	/* RPC XID counter */
 static int		sockfd = -1;	/* notify socket */
+static int		ifset = 0;
+
+/*
+ * Notify lockd of non-standard binding 
+ */
+void
+nlm_nsm_set(unsigned int addr)
+{
+	ssize_t sz = 0;
+	char buf[20];
+	int fd;
+
+	if ( ifset )
+		return ;
+
+	sprintf (buf,"%u",addr);
+
+	fd = open ("/proc/sys/fs/nfs/nlm_nsm_interface", O_RDWR);
+	if (fd > 0) {
+		sz = write (fd,buf,strlen((char*)buf));
+		if ( sz == -1 )
+			note(N_CRIT, "statd: write: %s\n", strerror(errno));
+		close (fd);
+	} else
+		note(N_CRIT, "statd: -n was specified with with no kernel support?\n");
+
+	ifset = 1;
+}
 
 /*
  * Initialize callback socket
@@ -63,6 +95,7 @@ statd_get_socket(void)
 {
 	struct sockaddr_in	sin;
 	struct servent *se;
+	struct hostent *hp;
 	int loopcnt = 100;
 
 	if (sockfd >= 0)
@@ -77,11 +110,18 @@ statd_get_socket(void)
 			return -1;
 		}
 
-
 		memset(&sin, 0, sizeof(sin));
 		sin.sin_family = AF_INET;
 		sin.sin_addr.s_addr = INADDR_ANY;
 
+		if (MY_NAME) {
+			hp = gethostbyname(MY_NAME);
+			if (hp) {
+				sin.sin_addr = *(struct in_addr *) hp->h_addr;
+				nlm_nsm_set ((unsigned int)sin.sin_addr.s_addr);
+			}
+		} 
+
 		if (bindresvport(sockfd, &sin) < 0) {
 			dprintf(N_WARNING,
 				"process_hosts: can't bind to reserved port\n");

Re: [patch] fix statd -n

[Peter Staubach]

J. Bruce Fields wrote:
> On Thu, Apr 17, 2008 at 12:38:43PM -0400, Janne Karhunen wrote:
>   
>> Apparently lockd does not expect statd to be used with -n
>> switch: statd is expected to bind loopback, always. Attached
>> patches show one (IPv4 specific) way of fixing it. Comments?
>>     
>
> Maybe statd really should always bind to the loopback interface?  Is
> there any reason not to?
>
>   

I think that statd needs to be reachable from clients
and/or servers when the state changes on the other end.

This "-n" option really assumes that the system is single-homed
though, doesn't it?  Binding to one particular interface will
make it so that statd will not be reachable via any of the other
interfaces on the system.

Perhaps statd should always bind to the loopback interface
if it doesn't bind to INADDR_ANY?

    Thanx...

       ps

> From a quick look at the current nfs-utils code: it looks like the -n
> option only affects the operation of the sm-notify program that's called
> on boot to notify peer statd's?  I'm a little confused.  (What version
> of nfs-utils are you working from?)
>
> --b.
>
>   
>> -- 
>> // Janne
>>     
>
>   
>> --- rmtcall.c.org	2008-04-14 10:53:30.000000000 -0400
>> +++ rmtcall.c	2008-04-14 13:01:27.000000000 -0400
>> @@ -37,6 +37,7 @@
>>  #include <netdb.h>
>>  #include <string.h>
>>  #include <unistd.h>
>> +#include <errno.h>
>>  #ifdef HAVE_IFADDRS_H
>>  #include <ifaddrs.h>
>>  #endif /* HAVE_IFADDRS_H */
>> @@ -54,6 +55,34 @@
>>  
>>  static unsigned long	xid = 0;	/* RPC XID counter */
>>  static int		sockfd = -1;	/* notify socket */
>> +static int              ifset = 0;
>> +
>> +/*
>> + * Notify lockd of non-standard binding 
>> + */
>> +inline void
>> +nlm_nsm_set(unsigned int addr)
>> +{
>> +        ssize_t sz = 0;
>> +        char buf[20];
>> +        int fd;
>> +
>> +        if ( ifset )
>> +                return ;
>> +
>> +        sprintf (buf,"%u",addr);
>> +
>> +        fd = open ("/proc/sys/fs/nfs/nlm_nsm_interface", O_RDWR);
>> +        if (fd > 0) {
>> +                sz = write (fd,buf,strlen((char*)buf));
>> +                if ( sz == -1 )
>> +                        note(N_CRIT, "statd: write: %s\n", strerror(errno));
>> +                close (fd);
>> +        } else
>> +                note(N_CRIT, "statd: -n was specified with with no kernel support?\n");
>> +
>> +        ifset = 1;
>> +}
>>  
>>  /*
>>   * Initialize callback socket
>> @@ -85,6 +114,7 @@ statd_get_socket(int port)
>>  		struct hostent *hp = gethostbyname(MY_NAME);
>>  		if (hp)
>>  			sin.sin_addr = *(struct in_addr *) hp->h_addr;
>> +		nlm_nsm_set ((unsigned int)sin.sin_addr.s_addr);
>>  	}
>>  	if (port != 0) {
>>  		sin.sin_port = htons(port);
>>     
>
>   
>> diff -Naurp lockd.org/svc4proc.c lockd/svc4proc.c
>> --- lockd.org/svc4proc.c	2008-04-14 10:58:29.000000000 -0400
>> +++ lockd/svc4proc.c	2008-04-14 12:19:04.000000000 -0400
>> @@ -21,6 +21,8 @@
>>  
>>  #define NLMDBG_FACILITY		NLMDBG_CLIENT
>>  
>> +extern unsigned int nlm_nsm_interface;
>> +
>>  /*
>>   * Obtain client and file from arguments
>>   */
>> @@ -430,8 +432,9 @@ nlm4svc_proc_sm_notify(struct svc_rqst *
>>  	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>>  
>>  	dprintk("lockd: SM_NOTIFY     called\n");
>> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
>> -	 || ntohs(saddr.sin_port) >= 1024) {
>> +	if (((saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK))
>> +	 && (nlm_nsm_interface && (saddr.sin_addr.s_addr != htonl(nlm_nsm_interface))))
>> +	 || (ntohs(saddr.sin_port) >= 1024)) {
>>  		char buf[RPC_MAX_ADDRBUFLEN];
>>  		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>>  				svc_print_addr(rqstp, buf, sizeof(buf)));
>> diff -Naurp lockd.org/svc.c lockd/svc.c
>> --- lockd.org/svc.c	2008-04-14 10:58:29.000000000 -0400
>> +++ lockd/svc.c	2008-04-14 12:19:04.000000000 -0400
>> @@ -64,6 +64,7 @@ static unsigned long		nlm_grace_period;
>>  static unsigned long		nlm_timeout = LOCKD_DFLT_TIMEO;
>>  static int			nlm_udpport, nlm_tcpport;
>>  int				nsm_use_hostnames = 0;
>> +unsigned int			nlm_nsm_interface = 0;
>>  
>>  /*
>>   * Constants needed for the sysctl interface.
>> @@ -425,6 +426,14 @@ static ctl_table nlm_sysctls[] = {
>>  		.mode		= 0644,
>>  		.proc_handler	= &proc_dointvec,
>>  	},
>> +        {
>> +                .ctl_name       = CTL_UNNUMBERED,
>> +                .procname       = "nlm_nsm_interface",
>> +                .data           = &nlm_nsm_interface,
>> +                .maxlen         = sizeof(int),
>> +                .mode           = 0644,
>> +                .proc_handler   = &proc_dointvec,
>> +        },
>>  	{ .ctl_name = 0 }
>>  };
>>  
>> diff -Naurp lockd.org/svcproc.c lockd/svcproc.c
>> --- lockd.org/svcproc.c	2008-04-14 10:58:29.000000000 -0400
>> +++ lockd/svcproc.c	2008-04-14 12:19:04.000000000 -0400
>> @@ -21,6 +21,8 @@
>>  
>>  #define NLMDBG_FACILITY		NLMDBG_CLIENT
>>  
>> +extern unsigned int nlm_nsm_interface;
>> +
>>  #ifdef CONFIG_LOCKD_V4
>>  static __be32
>>  cast_to_nlm(__be32 status, u32 vers)
>> @@ -462,8 +464,9 @@ nlmsvc_proc_sm_notify(struct svc_rqst *r
>>  	memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>>  
>>  	dprintk("lockd: SM_NOTIFY     called\n");
>> -	if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
>> -	 || ntohs(saddr.sin_port) >= 1024) {
>> +	if (((saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK))
>> +	 && (nlm_nsm_interface && (saddr.sin_addr.s_addr != htonl(nlm_nsm_interface))))
>> +	 || (ntohs(saddr.sin_port) >= 1024)) {
>>  		char buf[RPC_MAX_ADDRBUFLEN];
>>  		printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>>  				svc_print_addr(rqstp, buf, sizeof(buf)));
>>     
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [patch] fix statd -n

[Peter Staubach]

Janne Karhunen wrote:
> On Fri, Apr 18, 2008 at 1:36 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
>   
>>  > Apparently lockd does not expect statd to be used with -n
>>  > switch: statd is expected to bind loopback, always. Attached
>>  > patches show one (IPv4 specific) way of fixing it. Comments?
>>
>>  Maybe statd really should always bind to the loopback interface?  Is
>>  there any reason not to?
>>     
>
> In my case I used -n to push NSM/NLM comms to
> clusters specific NFS service address (that just
> happens to migrate all around). -n is the only way
> to do this: otherwise it will bind to INADDR_ANY
> on node that has gazillion interfaces/addresses..
>
>   

What happens on systems with more than one network interface?

    Thanx...

       ps

> That, and the original reasoning for the option is
> said in statd code:
>
>         /*
>          * If a local hostname is given (-n option to statd), bind to
> the address
>          * specified. This is required to support clients that ignore
> the mon_name in
>          * the statd protocol but use the source address from the
> request packet.
>          */
>
>
>   
>>  From a quick look at the current nfs-utils code: it looks like the -n
>>  option only affects the operation of the sm-notify program that's called
>>  on boot to notify peer statd's?  I'm a little confused.  (What version
>>  of nfs-utils are you working from?)
>>     
>
> Right. 1.0.8 -> 1.0.12, it's valid for all.
>
>
>   

Re: [patch] fix statd -n

[Peter Staubach]

Peter Staubach wrote:
> J. Bruce Fields wrote:
>> On Thu, Apr 17, 2008 at 12:38:43PM -0400, Janne Karhunen wrote:
>>  
>>> Apparently lockd does not expect statd to be used with -n
>>> switch: statd is expected to bind loopback, always. Attached
>>> patches show one (IPv4 specific) way of fixing it. Comments?
>>>     
>>
>> Maybe statd really should always bind to the loopback interface?  Is
>> there any reason not to?
>>
>>   
>
> I think that statd needs to be reachable from clients
> and/or servers when the state changes on the other end.
>
> This "-n" option really assumes that the system is single-homed
> though, doesn't it?  Binding to one particular interface will
> make it so that statd will not be reachable via any of the other
> interfaces on the system.
>
> Perhaps statd should always bind to the loopback interface
> if it doesn't bind to INADDR_ANY?
>

Sorry, not very clear.  Perhaps statd should bind to the loopback
interface in addition to any other interfaces if it doesn't bind
to INADDR_ANY.

    Thanx...

       ps

>    Thanx...
>
>       ps
>
>> From a quick look at the current nfs-utils code: it looks like the -n
>> option only affects the operation of the sm-notify program that's called
>> on boot to notify peer statd's?  I'm a little confused.  (What version
>> of nfs-utils are you working from?)
>>
>> --b.
>>
>>  
>>> -- 
>>> // Janne
>>>     
>>
>>  
>>> --- rmtcall.c.org    2008-04-14 10:53:30.000000000 -0400
>>> +++ rmtcall.c    2008-04-14 13:01:27.000000000 -0400
>>> @@ -37,6 +37,7 @@
>>>  #include <netdb.h>
>>>  #include <string.h>
>>>  #include <unistd.h>
>>> +#include <errno.h>
>>>  #ifdef HAVE_IFADDRS_H
>>>  #include <ifaddrs.h>
>>>  #endif /* HAVE_IFADDRS_H */
>>> @@ -54,6 +55,34 @@
>>>  
>>>  static unsigned long    xid = 0;    /* RPC XID counter */
>>>  static int        sockfd = -1;    /* notify socket */
>>> +static int              ifset = 0;
>>> +
>>> +/*
>>> + * Notify lockd of non-standard binding + */
>>> +inline void
>>> +nlm_nsm_set(unsigned int addr)
>>> +{
>>> +        ssize_t sz = 0;
>>> +        char buf[20];
>>> +        int fd;
>>> +
>>> +        if ( ifset )
>>> +                return ;
>>> +
>>> +        sprintf (buf,"%u",addr);
>>> +
>>> +        fd = open ("/proc/sys/fs/nfs/nlm_nsm_interface", O_RDWR);
>>> +        if (fd > 0) {
>>> +                sz = write (fd,buf,strlen((char*)buf));
>>> +                if ( sz == -1 )
>>> +                        note(N_CRIT, "statd: write: %s\n", 
>>> strerror(errno));
>>> +                close (fd);
>>> +        } else
>>> +                note(N_CRIT, "statd: -n was specified with with no 
>>> kernel support?\n");
>>> +
>>> +        ifset = 1;
>>> +}
>>>  
>>>  /*
>>>   * Initialize callback socket
>>> @@ -85,6 +114,7 @@ statd_get_socket(int port)
>>>          struct hostent *hp = gethostbyname(MY_NAME);
>>>          if (hp)
>>>              sin.sin_addr = *(struct in_addr *) hp->h_addr;
>>> +        nlm_nsm_set ((unsigned int)sin.sin_addr.s_addr);
>>>      }
>>>      if (port != 0) {
>>>          sin.sin_port = htons(port);
>>>     
>>
>>  
>>> diff -Naurp lockd.org/svc4proc.c lockd/svc4proc.c
>>> --- lockd.org/svc4proc.c    2008-04-14 10:58:29.000000000 -0400
>>> +++ lockd/svc4proc.c    2008-04-14 12:19:04.000000000 -0400
>>> @@ -21,6 +21,8 @@
>>>  
>>>  #define NLMDBG_FACILITY        NLMDBG_CLIENT
>>>  
>>> +extern unsigned int nlm_nsm_interface;
>>> +
>>>  /*
>>>   * Obtain client and file from arguments
>>>   */
>>> @@ -430,8 +432,9 @@ nlm4svc_proc_sm_notify(struct svc_rqst *
>>>      memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>>>  
>>>      dprintk("lockd: SM_NOTIFY     called\n");
>>> -    if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
>>> -     || ntohs(saddr.sin_port) >= 1024) {
>>> +    if (((saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK))
>>> +     && (nlm_nsm_interface && (saddr.sin_addr.s_addr != 
>>> htonl(nlm_nsm_interface))))
>>> +     || (ntohs(saddr.sin_port) >= 1024)) {
>>>          char buf[RPC_MAX_ADDRBUFLEN];
>>>          printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>>>                  svc_print_addr(rqstp, buf, sizeof(buf)));
>>> diff -Naurp lockd.org/svc.c lockd/svc.c
>>> --- lockd.org/svc.c    2008-04-14 10:58:29.000000000 -0400
>>> +++ lockd/svc.c    2008-04-14 12:19:04.000000000 -0400
>>> @@ -64,6 +64,7 @@ static unsigned long        nlm_grace_period;
>>>  static unsigned long        nlm_timeout = LOCKD_DFLT_TIMEO;
>>>  static int            nlm_udpport, nlm_tcpport;
>>>  int                nsm_use_hostnames = 0;
>>> +unsigned int            nlm_nsm_interface = 0;
>>>  
>>>  /*
>>>   * Constants needed for the sysctl interface.
>>> @@ -425,6 +426,14 @@ static ctl_table nlm_sysctls[] = {
>>>          .mode        = 0644,
>>>          .proc_handler    = &proc_dointvec,
>>>      },
>>> +        {
>>> +                .ctl_name       = CTL_UNNUMBERED,
>>> +                .procname       = "nlm_nsm_interface",
>>> +                .data           = &nlm_nsm_interface,
>>> +                .maxlen         = sizeof(int),
>>> +                .mode           = 0644,
>>> +                .proc_handler   = &proc_dointvec,
>>> +        },
>>>      { .ctl_name = 0 }
>>>  };
>>>  
>>> diff -Naurp lockd.org/svcproc.c lockd/svcproc.c
>>> --- lockd.org/svcproc.c    2008-04-14 10:58:29.000000000 -0400
>>> +++ lockd/svcproc.c    2008-04-14 12:19:04.000000000 -0400
>>> @@ -21,6 +21,8 @@
>>>  
>>>  #define NLMDBG_FACILITY        NLMDBG_CLIENT
>>>  
>>> +extern unsigned int nlm_nsm_interface;
>>> +
>>>  #ifdef CONFIG_LOCKD_V4
>>>  static __be32
>>>  cast_to_nlm(__be32 status, u32 vers)
>>> @@ -462,8 +464,9 @@ nlmsvc_proc_sm_notify(struct svc_rqst *r
>>>      memcpy(&saddr, svc_addr_in(rqstp), sizeof(saddr));
>>>  
>>>      dprintk("lockd: SM_NOTIFY     called\n");
>>> -    if (saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK)
>>> -     || ntohs(saddr.sin_port) >= 1024) {
>>> +    if (((saddr.sin_addr.s_addr != htonl(INADDR_LOOPBACK))
>>> +     && (nlm_nsm_interface && (saddr.sin_addr.s_addr != 
>>> htonl(nlm_nsm_interface))))
>>> +     || (ntohs(saddr.sin_port) >= 1024)) {
>>>          char buf[RPC_MAX_ADDRBUFLEN];
>>>          printk(KERN_WARNING "lockd: rejected NSM callback from %s\n",
>>>                  svc_print_addr(rqstp, buf, sizeof(buf)));
>>>     
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>   
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] fix statd -n

[J. Bruce Fields]

On Fri, Apr 18, 2008 at 04:23:50PM -0400, Peter Staubach wrote:
> Peter Staubach wrote:
>> J. Bruce Fields wrote:
>>> On Thu, Apr 17, 2008 at 12:38:43PM -0400, Janne Karhunen wrote:
>>>  
>>>> Apparently lockd does not expect statd to be used with -n
>>>> switch: statd is expected to bind loopback, always. Attached
>>>> patches show one (IPv4 specific) way of fixing it. Comments?
>>>>     
>>>
>>> Maybe statd really should always bind to the loopback interface?  Is
>>> there any reason not to?
>>>
>>>   
>>
>> I think that statd needs to be reachable from clients
>> and/or servers when the state changes on the other end.
>>
>> This "-n" option really assumes that the system is single-homed
>> though, doesn't it?  Binding to one particular interface will
>> make it so that statd will not be reachable via any of the other
>> interfaces on the system.
>>
>> Perhaps statd should always bind to the loopback interface
>> if it doesn't bind to INADDR_ANY?
>>
>
> Sorry, not very clear.  Perhaps statd should bind to the loopback
> interface in addition to any other interfaces if it doesn't bind
> to INADDR_ANY.

Right, that's what would make the most sense to me.  Janne, is there any
reason that wouldn't solve your problem?

--b.

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, Apr 18, 2008 at 4:22 PM, Peter Staubach <staubach@redhat.com> wrote:

> > In my case I used -n to push NSM/NLM comms to
> > clusters specific NFS service address (that just
> > happens to migrate all around). -n is the only way
> > to do this: otherwise it will bind to INADDR_ANY
> > on node that has gazillion interfaces/addresses..
> >
>
>  What happens on systems with more than one network interface?

Using -n breaks and without it it uses loopback
at least on this system - but that's just luck I
guess. And, of course, without -n it's bound to
ANY which leaves ports wide open all around.


-- 
// Janne

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, Apr 18, 2008 at 4:32 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > Sorry, not very clear.  Perhaps statd should bind to the loopback
>  > interface in addition to any other interfaces if it doesn't bind
>  > to INADDR_ANY.
>
>  Right, that's what would make the most sense to me.  Janne, is there any
>  reason that wouldn't solve your problem?

I didn't get the idea. So the idea is to use multiple sockets,
one bound to LOOPBACK and one to external interface?
Complicated and unclean in my opinion: one address
should suffice.


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Fri, Apr 18, 2008 at 04:46:02PM -0400, Janne Karhunen wrote:
> On Fri, Apr 18, 2008 at 4:32 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> >  > Sorry, not very clear.  Perhaps statd should bind to the loopback
> >  > interface in addition to any other interfaces if it doesn't bind
> >  > to INADDR_ANY.
> >
> >  Right, that's what would make the most sense to me.  Janne, is there any
> >  reason that wouldn't solve your problem?
> 
> I didn't get the idea. So the idea is to use multiple sockets,
> one bound to LOOPBACK and one to external interface?

I suppose so.  One socket would be for communication for the local
kernel nfsd, one for communication with statd peers.

> Complicated and unclean in my opinion: one address
> should suffice.

The advantage is that it would require no changes to the kernel or
kernel interfaces, and would also solve the problem for people that
don't want to upgrade their kernels.

The "rpc over lo" interface to the kernel's lockd is simple enough, and
I'd rather not replace it with "rpc over either lo or the interface
specified via sysctl" unless there's a really clear advantage.

(Also, would your patch mean lockd could accept requests that could have
spoofed source addresses?)

--b.

Re: [patch] fix statd -n

[Janne Karhunen]

On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > I didn't get the idea. So the idea is to use multiple sockets,
>  > one bound to LOOPBACK and one to external interface?
>
>  I suppose so.  One socket would be for communication for the local
>  kernel nfsd, one for communication with statd peers.

Ok, but that's really quite intrusive - my goal with that
patch was to minimize the amount of changes. Sure,
we can rework larger part of it if you think is better
that way.


>  > Complicated and unclean in my opinion: one address
>  > should suffice.
>
>  The advantage is that it would require no changes to the kernel or
>  kernel interfaces, and would also solve the problem for people that
>  don't want to upgrade their kernels.

Right, but that's hardly an issue with Linux. You need
to do that twice per week anyway ;)


>  The "rpc over lo" interface to the kernel's lockd is simple enough, and
>  I'd rather not replace it with "rpc over either lo or the interface
>  specified via sysctl" unless there's a really clear advantage.
>
>  (Also, would your patch mean lockd could accept requests that could have
>  spoofed source addresses?)

Yes, but loopback can also be spoofed. And it
does already improve things by making it bind
specific interface/address instead of ANY (ports
open all around).


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Sun, Apr 20, 2008 at 08:49:52PM -0400, Janne Karhunen wrote:
> On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> >  > I didn't get the idea. So the idea is to use multiple sockets,
> >  > one bound to LOOPBACK and one to external interface?
> >
> >  I suppose so.  One socket would be for communication for the local
> >  kernel nfsd, one for communication with statd peers.
> 
> Ok, but that's really quite intrusive - my goal with that
> patch was to minimize the amount of changes. Sure,
> we can rework larger part of it if you think is better
> that way.

Yes.  Hopefully it's not too bad....

> >  > Complicated and unclean in my opinion: one address
> >  > should suffice.
> >
> >  The advantage is that it would require no changes to the kernel or
> >  kernel interfaces, and would also solve the problem for people that
> >  don't want to upgrade their kernels.
> 
> Right, but that's hardly an issue with Linux. You need
> to do that twice per week anyway ;)
> 
> 
> >  The "rpc over lo" interface to the kernel's lockd is simple enough, and
> >  I'd rather not replace it with "rpc over either lo or the interface
> >  specified via sysctl" unless there's a really clear advantage.
> >
> >  (Also, would your patch mean lockd could accept requests that could have
> >  spoofed source addresses?)
> 
> Yes, but loopback can also be spoofed.

Is that true?  I thought the kernel discarded packets from interfaces
other than lo claiming to be from 127.*.*.*.

--b.

Re: [patch] fix statd -n

[Jeff Layton]

On Sun, 20 Apr 2008 22:11:53 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Sun, Apr 20, 2008 at 08:49:52PM -0400, Janne Karhunen wrote:
> > On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > >  > I didn't get the idea. So the idea is to use multiple sockets,
> > >  > one bound to LOOPBACK and one to external interface?
> > >
> > >  I suppose so.  One socket would be for communication for the local
> > >  kernel nfsd, one for communication with statd peers.
> > 
> > Ok, but that's really quite intrusive - my goal with that
> > patch was to minimize the amount of changes. Sure,
> > we can rework larger part of it if you think is better
> > that way.
> 
> Yes.  Hopefully it's not too bad....
> 
> > >  > Complicated and unclean in my opinion: one address
> > >  > should suffice.
> > >
> > >  The advantage is that it would require no changes to the kernel or
> > >  kernel interfaces, and would also solve the problem for people that
> > >  don't want to upgrade their kernels.
> > 
> > Right, but that's hardly an issue with Linux. You need
> > to do that twice per week anyway ;)
> > 
> > 
> > >  The "rpc over lo" interface to the kernel's lockd is simple enough, and
> > >  I'd rather not replace it with "rpc over either lo or the interface
> > >  specified via sysctl" unless there's a really clear advantage.
> > >
> > >  (Also, would your patch mean lockd could accept requests that could have
> > >  spoofed source addresses?)
> > 
> > Yes, but loopback can also be spoofed.
> 
> Is that true?  I thought the kernel discarded packets from interfaces
> other than lo claiming to be from 127.*.*.*.
> 

I think that's the case only if you have rp_filter turned on. It
usually is these days, but there are some situations where it doesn't
do what's expected (vlans, for instance), and has to be disabled.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [patch] fix statd -n

[J. Bruce Fields]

On Mon, Apr 21, 2008 at 07:01:07AM -0400, Jeff Layton wrote:
> On Sun, 20 Apr 2008 22:11:53 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Sun, Apr 20, 2008 at 08:49:52PM -0400, Janne Karhunen wrote:
> > > Yes, but loopback can also be spoofed.
> > 
> > Is that true?  I thought the kernel discarded packets from interfaces
> > other than lo claiming to be from 127.*.*.*.
> > 
> 
> I think that's the case only if you have rp_filter turned on. It
> usually is these days, but there are some situations where it doesn't
> do what's expected (vlans, for instance), and has to be disabled.

Well, if you believe Documentation/filesystems/proc.txt on rp_filter:

	"Integer value determines if a source validation should be made.
	1 means yes, 0 means no.  Disabled by default, but
	local/broadcast address spoofing is always on."

But I haven't tested this or looked at the code.

--b.

Re: [patch] fix statd -n

[Jeff Layton]

On Mon, 21 Apr 2008 09:39:40 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Apr 21, 2008 at 07:01:07AM -0400, Jeff Layton wrote:
> > On Sun, 20 Apr 2008 22:11:53 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Sun, Apr 20, 2008 at 08:49:52PM -0400, Janne Karhunen wrote:
> > > > Yes, but loopback can also be spoofed.
> > > 
> > > Is that true?  I thought the kernel discarded packets from interfaces
> > > other than lo claiming to be from 127.*.*.*.
> > > 
> > 
> > I think that's the case only if you have rp_filter turned on. It
> > usually is these days, but there are some situations where it doesn't
> > do what's expected (vlans, for instance), and has to be disabled.
> 
> Well, if you believe Documentation/filesystems/proc.txt on rp_filter:
> 
> 	"Integer value determines if a source validation should be made.
> 	1 means yes, 0 means no.  Disabled by default, but
> 	local/broadcast address spoofing is always on."
> 
> But I haven't tested this or looked at the code.
> 
> --b.

I think that's basically correct, but most modern distros turn it on by
default. From the default /etc/sysctl.conf on my fedora box:

net.ipv4.conf.default.rp_filter = 1

...it's generally a good thing to enable, but there are places where it
needs to be disabled. For instance, my Linksys WRT54g is doing firewall
duties and has it disabled because the switch ports on it are segmented
with VLANs and rp_filter interferes with that.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [patch] fix statd -n

[Janne Karhunen]

On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > >  > Sorry, not very clear.  Perhaps statd should bind to the loopback
>  > >  > interface in addition to any other interfaces if it doesn't bind
>  > >  > to INADDR_ANY.
>  > >
>  > >  Right, that's what would make the most sense to me.  Janne, is there any
>  > >  reason that wouldn't solve your problem?
>  >
>  > I didn't get the idea. So the idea is to use multiple sockets,
>  > one bound to LOOPBACK and one to external interface?
>
>  I suppose so.  One socket would be for communication for the local
>  kernel nfsd, one for communication with statd peers.

So shall we add yet another port option for statd or talk
to portmap about the port assignment?  It's ugly any way
you put it.



-- 
// Janne

Re: [patch] fix statd -n

[Chuck Lever]

On Apr 20, 2008, at 10:11 PM, J. Bruce Fields wrote:
> On Sun, Apr 20, 2008 at 08:49:52PM -0400, Janne Karhunen wrote:
>> On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org 
>> > wrote:
>>
>>>> I didn't get the idea. So the idea is to use multiple sockets,
>>>> one bound to LOOPBACK and one to external interface?
>>>
>>> I suppose so.  One socket would be for communication for the local
>>> kernel nfsd, one for communication with statd peers.
>>
>> Ok, but that's really quite intrusive - my goal with that
>> patch was to minimize the amount of changes. Sure,
>> we can rework larger part of it if you think is better
>> that way.
>
> Yes.  Hopefully it's not too bad....

My two pfennigs worth:

I think creating a separate loopback listener in the "-n" case is a  
reasonable thing to do, and probably won't be terribly complex.  This  
isn't a performance path, so you can probably get away with a select  
on multiple sockets.

Alternately, you could create separate rpc.statd daemons -- one would  
listen only for loopback.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, Apr 21, 2008 at 11:01 AM, Chuck Lever <chuck.lever@oracle.com> wrote:

>  I think creating a separate loopback listener in the "-n" case is a
> reasonable thing to do, and probably won't be terribly complex.  This isn't
> a performance path, so you can probably get away with a select on multiple
> sockets.

I'm currently experimenting this by adding it a
statd_get_nlm_socket() and going through the
code to catch which socket should be used
and when..


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Mon, Apr 21, 2008 at 10:46:00AM -0400, Janne Karhunen wrote:
> On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> >  > >  > Sorry, not very clear.  Perhaps statd should bind to the loopback
> >  > >  > interface in addition to any other interfaces if it doesn't bind
> >  > >  > to INADDR_ANY.
> >  > >
> >  > >  Right, that's what would make the most sense to me.  Janne, is there any
> >  > >  reason that wouldn't solve your problem?
> >  >
> >  > I didn't get the idea. So the idea is to use multiple sockets,
> >  > one bound to LOOPBACK and one to external interface?
> >
> >  I suppose so.  One socket would be for communication for the local
> >  kernel nfsd, one for communication with statd peers.
> 
> So shall we add yet another port option for statd or talk
> to portmap about the port assignment?  It's ugly any way
> you put it.

I'm confused--why is either needed?

--b.

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, Apr 21, 2008 at 12:59 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > So shall we add yet another port option for statd or talk
>  > to portmap about the port assignment?  It's ugly any way
>  > you put it.
>
>  I'm confused--why is either needed?

Right, it wasn't. Let me experiment with the patch
first if it does anything sane..


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Mon, Apr 21, 2008 at 10:10:03AM -0400, Jeff Layton wrote:
> On Mon, 21 Apr 2008 09:39:40 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Apr 21, 2008 at 07:01:07AM -0400, Jeff Layton wrote:
> > > On Sun, 20 Apr 2008 22:11:53 -0400
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Sun, Apr 20, 2008 at 08:49:52PM -0400, Janne Karhunen wrote:
> > > > > Yes, but loopback can also be spoofed.
> > > > 
> > > > Is that true?  I thought the kernel discarded packets from interfaces
> > > > other than lo claiming to be from 127.*.*.*.
> > > > 
> > > 
> > > I think that's the case only if you have rp_filter turned on. It
> > > usually is these days, but there are some situations where it doesn't
> > > do what's expected (vlans, for instance), and has to be disabled.
> > 
> > Well, if you believe Documentation/filesystems/proc.txt on rp_filter:
> > 
> > 	"Integer value determines if a source validation should be made.
> > 	1 means yes, 0 means no.  Disabled by default, but
> > 	local/broadcast address spoofing is always on."
> > 
> > But I haven't tested this or looked at the code.
> > 
> > --b.
> 
> I think that's basically correct, but most modern distros turn it on by
> default. From the default /etc/sysctl.conf on my fedora box:
> 
> net.ipv4.conf.default.rp_filter = 1
> 
> ...it's generally a good thing to enable, but there are places where it
> needs to be disabled. For instance, my Linksys WRT54g is doing firewall
> duties and has it disabled because the switch ports on it are segmented
> with VLANs and rp_filter interferes with that.

Actually, the specific question here is: say you have an ethernet
interface 192.168.0.1.  Will the kernel deliver a packet that comes from
the network and has source address 192.168.0.1?

--b.

Re: [patch] fix statd -n

[Jeff Layton]

On Mon, 21 Apr 2008 13:32:27 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> > I think that's basically correct, but most modern distros turn it on by
> > default. From the default /etc/sysctl.conf on my fedora box:
> > 
> > net.ipv4.conf.default.rp_filter = 1
> > 
> > ...it's generally a good thing to enable, but there are places where it
> > needs to be disabled. For instance, my Linksys WRT54g is doing firewall
> > duties and has it disabled because the switch ports on it are segmented
> > with VLANs and rp_filter interferes with that.
> 
> Actually, the specific question here is: say you have an ethernet
> interface 192.168.0.1.  Will the kernel deliver a packet that comes from
> the network and has source address 192.168.0.1?
> 

Ahh, I misunderstood then. I'm not sure about that specific situation.
I doubt that rp_filter would prevent that, but there may be some other
mechanism that would.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [patch] fix statd -n

[Wendy Cheng]

J. Bruce Fields wrote:
> On Mon, Apr 21, 2008 at 10:10:03AM -0400, Jeff Layton wrote:
>   
>> On Mon, 21 Apr 2008 09:39:40 -0400
>> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>
>>     
>>> On Mon, Apr 21, 2008 at 07:01:07AM -0400, Jeff Layton wrote:
>>>       
>>>> On Sun, 20 Apr 2008 22:11:53 -0400
>>>> "J. Bruce Fields" <bfields@fieldses.org> wrote:
>>>>
>>>>         
>>>>> On Sun, Apr 20, 2008 at 08:49:52PM -0400, Janne Karhunen wrote:
>>>>>           
>>>>>> Yes, but loopback can also be spoofed.
>>>>>>             
>>>>> Is that true?  I thought the kernel discarded packets from interfaces
>>>>> other than lo claiming to be from 127.*.*.*.
>>>>>
>>>>>           
>>>> I think that's the case only if you have rp_filter turned on. It
>>>> usually is these days, but there are some situations where it doesn't
>>>> do what's expected (vlans, for instance), and has to be disabled.
>>>>         
>>> Well, if you believe Documentation/filesystems/proc.txt on rp_filter:
>>>
>>> 	"Integer value determines if a source validation should be made.
>>> 	1 means yes, 0 means no.  Disabled by default, but
>>> 	local/broadcast address spoofing is always on."
>>>
>>> But I haven't tested this or looked at the code.
>>>
>>> --b.
>>>       
>> I think that's basically correct, but most modern distros turn it on by
>> default. From the default /etc/sysctl.conf on my fedora box:
>>
>> net.ipv4.conf.default.rp_filter = 1
>>
>> ...it's generally a good thing to enable, but there are places where it
>> needs to be disabled. For instance, my Linksys WRT54g is doing firewall
>> duties and has it disabled because the switch ports on it are segmented
>> with VLANs and rp_filter interferes with that.
>>     
>
> Actually, the specific question here is: say you have an ethernet
> interface 192.168.0.1.  Will the kernel deliver a packet that comes from
> the network and has source address 192.168.0.1?
>   

I doubt it will. Remember one of my old patches (patch 3 & 4) ?
https://www.redhat.com/archives/cluster-devel/2007-April/msg00028.html
https://www.redhat.com/archives/cluster-devel/2007-April/msg00032.html 
(patch 3)
https://www.redhat.com/archives/cluster-devel/2007-April/msg00031.html 
(patch 4)

I think you have to specifically hack the kernel (as I did) but I don't 
have linux source code in front of me at this moment.

-- Wendy

Re: [patch] fix statd -n

[Janne Karhunen]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > >  Right, that's what would make the most sense to me.  Janne, is there any
>  > >  reason that wouldn't solve your problem?
>  >
>  > I didn't get the idea. So the idea is to use multiple sockets,
>  > one bound to LOOPBACK and one to external interface?
>
>  I suppose so.  One socket would be for communication for the local
>  kernel nfsd, one for communication with statd peers.

Finally got around to it again. Attached patch takes a
shot at the two socket approach. Patch is a draft to
see what you guys would really think about this
approach.


-- 
// Janne

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: statd-two-sockets.patch --]
[-- Type: text/x-patch; name=statd-two-sockets.patch, Size: 7271 bytes --]

diff -Naurp nfs-utils-1.1.2.orig/utils/statd/rmtcall.c nfs-utils-1.1.2/utils/statd/rmtcall.c
--- nfs-utils-1.1.2.orig/utils/statd/rmtcall.c	2008-03-14 11:46:29.000000000 -0400
+++ nfs-utils-1.1.2/utils/statd/rmtcall.c	2008-04-28 16:16:02.000000000 -0400
@@ -54,35 +54,34 @@
 
 static unsigned long	xid = 0;	/* RPC XID counter */
 static int		sockfd = -1;	/* notify socket */
+static int              nlmfd = -1;     /* lockd socket */
 
 /*
- * Initialize callback socket
+ * Initialize callback socket and bind it to addr
  */
 int
-statd_get_socket(void)
+statd_reserve_socket(struct in_addr *addr)
 {
-	struct sockaddr_in	sin;
+	struct sockaddr_in      sin;
 	struct servent *se;
 	int loopcnt = 100;
+	int fd = -1;
 
-	if (sockfd >= 0)
-		return sockfd;
-
-	while (loopcnt-- > 0) {
+	memset(&sin, 0, sizeof(sin));
+	sin.sin_family = AF_INET;
+	memcpy(&sin.sin_addr, addr, sizeof(struct in_addr));
 
-		if (sockfd >= 0) close(sockfd);
+	while (loopcnt-- > 0)
+	{
+		if (fd >= 0)
+			close(fd);
 
-		if ((sockfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP)) < 0) {
+		if ((fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP)) < 0) {
 			note(N_CRIT, "Can't create socket: %m");
 			return -1;
 		}
 
-
-		memset(&sin, 0, sizeof(sin));
-		sin.sin_family = AF_INET;
-		sin.sin_addr.s_addr = INADDR_ANY;
-
-		if (bindresvport(sockfd, &sin) < 0) {
+		if (bindresvport(fd, &sin) < 0) {
 			dprintf(N_WARNING,
 				"process_hosts: can't bind to reserved port\n");
 			break;
@@ -92,12 +91,75 @@ statd_get_socket(void)
 			break;
 		/* rather not use that port, try again */
 	}
-	FD_SET(sockfd, &SVC_FDSET);
-	return sockfd;
+	FD_SET(fd, &SVC_FDSET);
+	return fd;
+}
+
+/*
+ * Check required sockets and init
+ */
+int
+statd_init_sockets(void)
+{
+	struct hostent *hp = 0;
+	struct in_addr addr;
+
+	if (MY_NAME)
+		hp = gethostbyname(MY_NAME);
+
+	if (hp) {
+		memcpy (&addr,hp->h_addr,sizeof(struct in_addr));
+		sockfd = statd_reserve_socket(&addr);
+
+		addr.s_addr = htonl(INADDR_LOOPBACK);
+		nlmfd  = statd_reserve_socket(&addr);
+	} else {
+		addr.s_addr = INADDR_ANY;
+		sockfd = statd_reserve_socket(&addr);
+		nlmfd  = sockfd;
+	}
+	if (sockfd < 0 || nlmfd < 0) {
+		close(sockfd);
+		close(nlmfd);
+		sockfd = 0;
+		nlmfd  = 0;
+		return -1;
+	}
+	return 1;
+}
+
+/*
+ *  Get socket to destination. Local communications should
+ *  use specific socket bound to loopback if we're running
+ *  with -n.
+ */
+int
+statd_get_socket(struct in_addr *dest)
+{
+	struct in_addr addr;
+	int res;
+
+	if (!dest)
+		return -1;
+
+	addr.s_addr = ntohl(dest->s_addr);
+
+	if ((nlmfd > 0) && (addr.s_addr == INADDR_LOOPBACK))
+		return nlmfd;
+
+	if ((sockfd > 0) && (addr.s_addr != INADDR_LOOPBACK))
+		return sockfd;
+
+	res = statd_init_sockets();
+	if (res < 0) {
+		note(N_WARNING, "statd_get_socket: could not initilize sockets\n");
+		return -1;
+	}
+	return (addr.s_addr == INADDR_LOOPBACK) ? nlmfd : sockfd;
 }
 
 static unsigned long
-xmit_call(int sockfd, struct sockaddr_in *sin,
+xmit_call(int fd, struct sockaddr_in *sin,
 	  u_int32_t prog, u_int32_t vers, u_int32_t proc,
 	  xdrproc_t func, void *obj)
 /* 		__u32 prog, __u32 vers, __u32 proc, xdrproc_t func, void *obj) */
@@ -150,7 +212,7 @@ xmit_call(int sockfd, struct sockaddr_in
 	/* Get overall length of datagram */
 	msglen = xdr_getpos(xdrs);
 
-	if ((err = sendto(sockfd, msgbuf, msglen, 0,
+	if ((err = sendto(fd, msgbuf, msglen, 0,
 			(struct sockaddr *) sin, sizeof(*sin))) < 0) {
 		dprintf(N_WARNING, "xmit_mesg: sendto failed: %m");
 	} else if (err != msglen) {
@@ -163,7 +225,7 @@ xmit_call(int sockfd, struct sockaddr_in
 }
 
 static notify_list *
-recv_rply(int sockfd, struct sockaddr_in *sin, u_long *portp)
+recv_rply(int fd, struct sockaddr_in *sin, u_long *portp)
 {
 	unsigned int		msgbuf[MAXMSGSIZE], msglen;
 	struct rpc_msg		mesg;
@@ -172,7 +234,7 @@ recv_rply(int sockfd, struct sockaddr_in
 	socklen_t		alen = sizeof(*sin);
 
 	/* Receive message */
-	if ((msglen = recvfrom(sockfd, msgbuf, sizeof(msgbuf), 0,
+	if ((msglen = recvfrom(fd, msgbuf, sizeof(msgbuf), 0,
 			(struct sockaddr *) sin, &alen)) < 0) {
 		dprintf(N_WARNING, "recv_rply: recvfrom failed: %m");
 		return NULL;
@@ -239,7 +301,7 @@ done:
  * Notify operation for a single list entry
  */
 static int
-process_entry(int sockfd, notify_list *lp)
+process_entry(int fd, notify_list *lp)
 {
 	struct sockaddr_in	sin;
 	struct status		new_status;
@@ -273,7 +335,7 @@ process_entry(int sockfd, notify_list *l
 	new_status.state    = NL_STATE(lp);
 	memcpy(new_status.priv, NL_PRIV(lp), SM_PRIV_SIZE);
 
-	lp->xid = xmit_call(sockfd, &sin, prog, vers, proc, func, objp);
+	lp->xid = xmit_call(fd, &sin, prog, vers, proc, func, objp);
 	if (!lp->xid) {
 		note(N_WARNING, "notify_host: failed to notify port %d\n",
 				ntohs(lp->port));
@@ -283,26 +345,20 @@ process_entry(int sockfd, notify_list *l
 	return 1;
 }
 
-/*
- * Process a datagram received on the notify socket
- */
 int
-process_reply(FD_SET_TYPE *rfds)
+process_sock(int fd)
 {
-	struct sockaddr_in	sin;
-	notify_list		*lp;
-	u_long			port;
+	struct sockaddr_in      sin;
+	notify_list             *lp;
+	u_long                  port;
 
-	if (sockfd == -1 || !FD_ISSET(sockfd, rfds))
-		return 0;
-
-	if (!(lp = recv_rply(sockfd, &sin, &port)))
+	if (!(lp = recv_rply(fd, &sin, &port)))
 		return 1;
 
 	if (lp->port == 0) {
 		if (port != 0) {
 			lp->port = htons((unsigned short) port);
-			process_entry(sockfd, lp);
+			process_entry(fd, lp);
 			NL_WHEN(lp) = time(NULL) + NOTIFY_TIMEOUT;
 			nlist_remove(&notify, lp);
 			nlist_insert_timer(&notify, lp);
@@ -319,6 +375,23 @@ process_reply(FD_SET_TYPE *rfds)
 }
 
 /*
+ * Process a datagram received on the notify socket
+ */
+int
+process_reply(FD_SET_TYPE *rfds)
+{
+	int ret1 = 0, ret2 = 0;
+
+	if (FD_ISSET(sockfd,rfds)) {
+		ret1 = process_sock(sockfd);
+	}
+	if ((nlmfd != sockfd) && (FD_ISSET(nlmfd,rfds))) {
+		ret2 = process_sock(nlmfd);
+	}
+	return ret1+ret2;
+}
+
+/*
  * Process a notify list, either for notifying remote hosts after reboot
  * or for calling back (local) statd clients when the remote has notified
  * us of a crash. 
@@ -330,10 +403,9 @@ process_notify_list(void)
 	time_t		now;
 	int		fd;
 
-	if ((fd = statd_get_socket()) < 0)
-		return 0;
-
 	while ((entry = notify) != NULL && NL_WHEN(entry) < time(&now)) {
+		fd = statd_get_socket(&entry->addr);
+
 		if (process_entry(fd, entry)) {
 			NL_WHEN(entry) = time(NULL) + NOTIFY_TIMEOUT;
 			nlist_remove(&notify, entry);
diff -Naurp nfs-utils-1.1.2.orig/utils/statd/statd.c nfs-utils-1.1.2/utils/statd/statd.c
--- nfs-utils-1.1.2.orig/utils/statd/statd.c	2008-03-14 11:46:29.000000000 -0400
+++ nfs-utils-1.1.2/utils/statd/statd.c	2008-04-28 14:47:15.000000000 -0400
@@ -75,7 +75,7 @@ static struct option longopts[] =
 };
 
 extern void sm_prog_1 (struct svc_req *, register SVCXPRT *);
-extern int statd_get_socket(void);
+extern int  statd_init_sockets(void);
 static void load_state_number(void);
 
 #ifdef SIMULATIONS
@@ -477,7 +477,7 @@ int main (int argc, char **argv)
 		}
 
 	/* Make sure we have a privilege port for calling into the kernel */
-	statd_get_socket();
+	statd_init_sockets();
 
 	/* If sm-notify didn't take all the state files, load
 	 * state information into our notify-list so we can

Re: [patch] fix statd -n

[Wendy Cheng]

Janne Karhunen wrote:
> On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
>   
>>  > >  Right, that's what would make the most sense to me.  Janne, is there any
>>  > >  reason that wouldn't solve your problem?
>>  >
>>  > I didn't get the idea. So the idea is to use multiple sockets,
>>  > one bound to LOOPBACK and one to external interface?
>>
>>  I suppose so.  One socket would be for communication for the local
>>  kernel nfsd, one for communication with statd peers.
>>     
>
> Finally got around to it again. Attached patch takes a
> shot at the two socket approach. Patch is a draft to
> see what you guys would really think about this
> approach.
>
>   
Do we really have to add so many lines of the code just to fix "statd 
-n" ? Maybe we should go back to the basics by understanding the 
requirement of this command ? So why do we need it (i.e. what kind of 
bad things we'll see if we don't fix this) ? Some short description 
would help. 

-- Wendy

Re: [patch] fix statd -n

[J. Bruce Fields]

On Tue, Apr 29, 2008 at 10:45:03AM -0400, Wendy Cheng wrote:
> Janne Karhunen wrote:
>> On Sun, Apr 20, 2008 at 8:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>>
>>   
>>>  > >  Right, that's what would make the most sense to me.  Janne, is there any
>>>  > >  reason that wouldn't solve your problem?
>>>  >
>>>  > I didn't get the idea. So the idea is to use multiple sockets,
>>>  > one bound to LOOPBACK and one to external interface?
>>>
>>>  I suppose so.  One socket would be for communication for the local
>>>  kernel nfsd, one for communication with statd peers.
>>>     
>>
>> Finally got around to it again. Attached patch takes a
>> shot at the two socket approach. Patch is a draft to
>> see what you guys would really think about this
>> approach.
>>
>>   
> Do we really have to add so many lines of the code just to fix "statd  
> -n" ? Maybe we should go back to the basics by understanding the  
> requirement of this command ? So why do we need it (i.e. what kind of  
> bad things we'll see if we don't fix this) ? Some short description  
> would help. 

I recall two reasons for -n given in this thread; I think one was just
security (maybe you don't want statd listening on some ports, for
whatever reason.  The other was a code comment quoted here:

	http://marc.info/?l=linux-nfs&m=120854237320424&w=2

	"This is required to support clients that ignore the mon_name in
	the statd protocol but use the source address from the request
	packet."

Which I don't completely understand.  I guess it was meant as a way to
ensure that *outgoing* packets are sent from the correct (floating) ip
address?

--b.

Re: [patch] fix statd -n

[Janne Karhunen]

On Tue, Apr 29, 2008 at 12:16 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > Do we really have to add so many lines of the code just to fix "statd
>  > -n"

Which is why I offered the small patch initially; it was
mentioned that intrusiveness does not matter if it
can be kept in userspace.


> >  ? Maybe we should go back to the basics by understanding the
>  > requirement of this command ? So why do we need it (i.e. what kind of
>  > bad things we'll see if we don't fix this) ? Some short description
>  > would help.
>
>  I recall two reasons for -n given in this thread; I think one was just
>  security (maybe you don't want statd listening on some ports, for
>  whatever reason.  The other was a code comment quoted here:

That being one..


>         http://marc.info/?l=linux-nfs&m=120854237320424&w=2
>
>
>         "This is required to support clients that ignore the mon_name in
>         the statd protocol but use the source address from the request
>         packet."

This another, and the third the fact that this way mon_name
stays the same on server failover to node that has different
name. It identifies the server instance..


>  Which I don't completely understand.  I guess it was meant as a way to
>  ensure that *outgoing* packets are sent from the correct (floating) ip
>  address?

Right.


-- 
// Janne

Re: [patch] fix statd -n

[Janne Karhunen]

Hi,

So effectively, it makes me sleep better. With it:
- I can rely on clients identifying the server correctly,
- I'm not exposing out anything that is not needed,
- Can tell by the address what this traffic is,
- Can be sure that packets are sent out via right interface

It might be even better if it would exit if -n is used when
no such interface is actually available. As I did it, it still
gambles here just as before.


-- 
// Janne

On Thu, May 1, 2008 at 8:57 AM, Janne Karhunen <janne.karhunen@gmail.com> wrote:
> On Tue, Apr 29, 2008 at 12:16 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
>  >  > Do we really have to add so many lines of the code just to fix "statd
>  >  > -n"
>
>  Which is why I offered the small patch initially; it was
>  mentioned that intrusiveness does not matter if it
>  can be kept in userspace.
>
>
>
>  > >  ? Maybe we should go back to the basics by understanding the
>  >  > requirement of this command ? So why do we need it (i.e. what kind of
>  >  > bad things we'll see if we don't fix this) ? Some short description
>  >  > would help.
>  >
>  >  I recall two reasons for -n given in this thread; I think one was just
>  >  security (maybe you don't want statd listening on some ports, for
>  >  whatever reason.  The other was a code comment quoted here:
>
>  That being one..
>
>
>
>  >         http://marc.info/?l=linux-nfs&m=120854237320424&w=2
>  >
>  >
>  >         "This is required to support clients that ignore the mon_name in
>  >         the statd protocol but use the source address from the request
>  >         packet."
>
>  This another, and the third the fact that this way mon_name
>  stays the same on server failover to node that has different
>  name. It identifies the server instance..
>
>
>
>  >  Which I don't completely understand.  I guess it was meant as a way to
>  >  ensure that *outgoing* packets are sent from the correct (floating) ip
>  >  address?
>
>  Right.
>
>
>  --
>  // Janne
>

Re: [patch] fix statd -n

[Wendy Cheng]

Janne Karhunen wrote:
> Hi,
>
> So effectively, it makes me sleep better. With it:
> - I can rely on clients identifying the server correctly,
> - I'm not exposing out anything that is not needed,
> - Can tell by the address what this traffic is,
> - Can be sure that packets are sent out via right interface
>
> It might be even better if it would exit if -n is used when
> no such interface is actually available. As I did it, it still
> gambles here just as before.
>
>
>   
Yep, understood. Linux NFS server failover was one of my painful 
projects (actually it was "the" one - so much work and so little result).

But the flow you proposed confuses me. Reading the code right now (to 
refresh my memory).. Will get back to the list soon.

-- Wendy

Re: [patch] fix statd -n

[Janne Karhunen]

On Thu, May 1, 2008 at 9:50 AM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:

> > It might be even better if it would exit if -n is used when
> > no such interface is actually available. As I did it, it still
> > gambles here just as before.
> >
>  Yep, understood. Linux NFS server failover was one of my painful projects
> (actually it was "the" one - so much work and so little result).

Heh


>  But the flow you proposed confuses me. Reading the code right now (to
> refresh my memory).. Will get back to the list soon.

Patch no.1 was better imho..

To be frank, given that it has to be done this way
I would probably make it even more intrusive by
having 'clean looking' socket pool. Not that it
matters much in real life terms as there are only
two sockets, but at least it would look more
readable..


-- 
// Janne

Re: [patch] fix statd -n

[Wendy Cheng]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

Janne Karhunen wrote:
> Hi,
>
> So effectively, it makes me sleep better. With it:
> - I can rely on clients identifying the server correctly,
> - I'm not exposing out anything that is not needed,
> - Can tell by the address what this traffic is,
> - Can be sure that packets are sent out via right interface
>
> It might be even better if it would exit if -n is used when
> no such interface is actually available. As I did it, it still
> gambles here just as before.
>
>   

After browsing thru "statd -n" flow, it is still not clear what will 
happen if there are more than 2 interfaces used to export NFS shares ? 

Using "statd -H", together with patches described in:
https://www.redhat.com/archives/cluster-devel/2007-April/msg00028.html ,

our cluster failover (with 4 IP interfaces per server) seemed to run 
well without troubles. Note that 2/3 of the patch in 4-3 can be removed 
*now* since it deals with moving server address from network header into 
lockd internal structures - another similar patch (by Frank van 
Maarseveen) was accepted into mainline kernel after our patch that has 
the required functionality:
http://lkml.org/lkml/2007/7/10/553 .

So the following is our (-H) flow:
* Server dispatches statd with "-N" option that has a user mode script
   (sample program fotest.c enclosed). It is expected the user mode script
   could structure its nlm directory accordingly.
* Upon failover, the take-over server notifies clients with:
   "/usr/sbin/sm-notify -f -v floating_ip_address -P an_sm_directory"

The advantages of "-H" approach over "-n" are (I think ?):
* It can handle multiple NFS export network interfaces.
* It knows which clients coming from which interfaces to allow selective 
grace period for each interface.

In many ways, I would think "-n" should be obsolete ?

-- Wendy

[-- Attachment #2: fotest.c --]
[-- Type: text/x-csrc, Size: 1163 bytes --]

#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/stat.h>      /* for mode_t */

void usage() 
{

	printf("\n");
	printf("    check out \"man rpc.statd\":\n");
	printf("    This callout program should receive callouts for\n");
	printf("    NLM  client  monitor and unmonitor requests. It is\n");
	printf("    run with 3 arguments: \n"); 
	printf("    1st: either add-client or del-client\n");
	printf("    2nd: the name of the client\n");
	printf("    3rd: the name of the server as known to the client.\n");

	return;
}

int main (int argc, char* argv[]) 
{
        char* file = "/tmp/NLM/from-ha";
        mode_t rw_mode;
        int rc, debug=0, i;
	FILE *fd;
        struct flock lock;

        /* Open a file descriptor to the file.  */
        rw_mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;     /* 0644 */
        fd = fopen(file, "a+");

	fprintf(fd, "%s is invoked with %d arguments.\n", argv[0], argc - 1); 
	if (argc != 4) {
		fprintf (fd, "    error: invalid arguments\n");
		debug = 1;
	}

	for (i = 1; i < argc; ++i) 
		fprintf (fd, "    %s\n", argv[i]); 

	fclose(fd);

	return 0; 
} 

Re: [patch] fix statd -n

[Wendy Cheng]

Wendy Cheng wrote:
> Janne Karhunen wrote:
>> Hi,
>>
>> So effectively, it makes me sleep better. With it:
>> - I can rely on clients identifying the server correctly,
>> - I'm not exposing out anything that is not needed,
>> - Can tell by the address what this traffic is,
>> - Can be sure that packets are sent out via right interface
>>
>> It might be even better if it would exit if -n is used when
>> no such interface is actually available. As I did it, it still
>> gambles here just as before.
>>
>>   
>
> After browsing thru "statd -n" flow, it is still not clear what will 
> happen if there are more than 2 interfaces used to export NFS shares ?
> Using "statd -H", together with patches described in:
> https://www.redhat.com/archives/cluster-devel/2007-April/msg00028.html ,
>
> our cluster failover (with 4 IP interfaces per server) seemed to run 
> well without troubles. Note that 2/3 of the patch in 4-3 can be 
> removed *now* since it deals with moving server address from network 
> header into lockd internal structures - another similar patch (by 
> Frank van Maarseveen) was accepted into mainline kernel after our 
> patch that has the required functionality:
> http://lkml.org/lkml/2007/7/10/553 .
>
> So the following is our (-H) flow:
> * Server dispatches statd with "-N" option that has a user mode script
obvious typo here .. "Server dispatches statd with "-H" option that has 
a uwer mode script"

-- Wendy

 

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, May 2, 2008 at 11:21 AM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:

>  After browsing thru "statd -n" flow, it is still not clear what will happen
> if there are more than 2 interfaces used to export NFS shares ?

How come? It will use the interface specified with -n.


>  Using "statd -H", together with patches described in:
>  https://www.redhat.com/archives/cluster-devel/2007-April/msg00028.html ,

So this basically makes it a users problem, I don't
like it. Statd's standard notifications are just fine and
I don't want to have anything to do with the process.
It should work as is, out of the box, without writing
separate programs to handle stuff that statd doesn't
do.


>  our cluster failover (with 4 IP interfaces per server) seemed to run well
> without troubles.

Your idea was to serve traffic via all of these interfaces?
One specific segment is just enough for us. Our servers
can have anything from 5 - 100 floating addresses and
it would be just great if we could keep each service
bound in it's own address. It's just better that way.


> Note that 2/3 of the patch in 4-3 can be removed *now*
> since it deals with moving server address from network header into lockd
> internal structures - another similar patch (by Frank van Maarseveen) was
> accepted into mainline kernel after our patch that has the required
> functionality:
>  http://lkml.org/lkml/2007/7/10/553 .
>
>  So the following is our (-H) flow:
>  * Server dispatches statd with "-N" option that has a user mode script
>   (sample program fotest.c enclosed). It is expected the user mode script
>   could structure its nlm directory accordingly.
>  * Upon failover, the take-over server notifies clients with:
>   "/usr/sbin/sm-notify -f -v floating_ip_address -P an_sm_directory"
>
>  The advantages of "-H" approach over "-n" are (I think ?):
>  * It can handle multiple NFS export network interfaces.
>  * It knows which clients coming from which interfaces to allow selective
> grace period for each interface.
>
>  In many ways, I would think "-n" should be obsolete ?

To me these use cases are clearly different. You're
trying to serve traffic to multiple segments and need
stuff that 'user have to worry about' to accomplish
this.  -n works as is for just one segment. And how
many users really need interface specific selective
grace anyway?


-- 
// Janne

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, May 2, 2008 at 5:13 PM, Janne Karhunen <janne.karhunen@gmail.com> wrote:

>  >  After browsing thru "statd -n" flow, it is still not clear what will happen
>  > if there are more than 2 interfaces used to export NFS shares ?
>
>  How come? It will use the interface specified with -n.

Ah, you were talking about rest of the services. Yeah,
we would need to go this through one patch at a time.


-- 
// Janne

Re: [patch] fix statd -n

[Wendy Cheng]

Janne Karhunen wrote:
> On Fri, May 2, 2008 at 11:21 AM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:
>
>   
>>  After browsing thru "statd -n" flow, it is still not clear what will happen
>> if there are more than 2 interfaces used to export NFS shares ?
>>     
>
> How come? It will use the interface specified with -n.
>   

ok, just read your follow-on email .. so we can cross out this one.

>
>   
>>  Using "statd -H", together with patches described in:
>>  https://www.redhat.com/archives/cluster-devel/2007-April/msg00028.html ,
>>     
>
> So this basically makes it a users problem, I don't
> like it. Statd's standard notifications are just fine and
> I don't want to have anything to do with the process.
> It should work as is, out of the box, without writing
> separate programs to handle stuff that statd doesn't
> do.
>   
I think you mis-understand our approach. With our patch, you do *not* 
need to do anything if you have only one single (floating) interface to 
do nfs export (and you can have the machine configured to use other 
hostname). The statd's "my_name" is correctly filled (from kernel by our 
patch) - it is no longer bound to 127.0.0.1.

Maybe I mislead you in previous post. The "-H" is only an "example" - to 
show people how to selectively move an ip address around without 
affecting other co-existing nfs ip interface on the same server. I did 
plan to submit a complete user mode patch so the program after "-H" will 
have a default executable (but user still can use their own if they 
don't like our nlm directory structure).

>
>   
>>  our cluster failover (with 4 IP interfaces per server) seemed to run well
>> without troubles.
>>     
>
> Your idea was to serve traffic via all of these interfaces?
>   

yes ...

> One specific segment is just enough for us. 

I know .. but there *are* users and live systems *today* that have 
multiple nfs interfaces. For big-fat server, I normally saw 4.

> Our servers
> can have anything from 5 - 100 floating addresses and
> it would be just great if we could keep each service
> bound in it's own address. It's just better that way.
>
> To me these use cases are clearly different. You're
> trying to serve traffic to multiple segments and need
> stuff that 'user have to worry about' to accomplish
> this.  -n works as is for just one segment. And how
> many users really need interface specific selective
> grace anyway?
>
>
>   
I hope above clears the confusion ? Again, admin(s)  do *not* need to do 
anything if there is only one single (floating) nfs export IP address.

On the other hand,  we did see systems had many nfs interfaces (I was 
surprised too). And that was exactly the rationale to kick off this work 
three years ago . 

-- Wendy

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, May 2, 2008 at 6:33 PM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:

> > So this basically makes it a users problem, I don't
> > like it. Statd's standard notifications are just fine and
> > I don't want to have anything to do with the process.
> > It should work as is, out of the box, without writing
> > separate programs to handle stuff that statd doesn't
> > do.
> >
>  I think you mis-understand our approach. With our patch, you do *not* need
> to do anything if you have only one single (floating) interface to do nfs
> export (and you can have the machine configured to use other hostname). The
> statd's "my_name" is correctly filled (from kernel by our patch) - it is no
> longer bound to 127.0.0.1.

What happens when it fails over to node that has
different name?  Mon_name in NOTIFY changes
while address it's coming from does not. Which
one should client believe - the address or the
name?

But OK, I probably need to dig out the patch to
have a look.


> > One specific segment is just enough for us.
>
>  I know .. but there *are* users and live systems *today* that have multiple
> nfs interfaces. For big-fat server, I normally saw 4.

I'm still a bit confused - does your patch make it
sure that all the extra N+1 IP addresses in the
node have nothing to do with NFS?

If not, these still seem like two different use cases,
ie. making the service tightly bound to one service
address instead of making it work right with many.


-- 
// Janne

Re: [patch] fix statd -n

[Janne Karhunen]

On Fri, May 2, 2008 at 6:33 PM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:

>  Maybe I mislead you in previous post. The "-H" is only an "example" - to
> show people how to selectively move an ip address around without affecting
> other co-existing nfs ip interface on the same server. I did plan to submit
> a complete user mode patch so the program after "-H" will have a default
> executable (but user still can use their own if they don't like our nlm
> directory structure).

Even if you ship the default executable user would
still need to go and edit it (add the address). Well,
I guess it would not be that much harder than
specifying the -n, but somehow this does seem
more arcane (for simple setups).


-- 
// Janne

Re: [patch] fix statd -n

[Wendy Cheng]

Janne Karhunen wrote:
>
>>  I think you mis-understand our approach. With our patch, you do *not* need
>> to do anything if you have only one single (floating) interface to do nfs
>> export (and you can have the machine configured to use other hostname). The
>> statd's "my_name" is correctly filled (from kernel by our patch) - it is no
>> longer bound to 127.0.0.1.
>>     
>
> What happens when it fails over to node that has
> different name?  Mon_name in NOTIFY changes
> while address it's coming from does not. Which
> one should client believe - the address or the
> name?
>
> But OK, I probably need to dig out the patch to
> have a look.
>   
In the middle of re-doing our old patch (it should be very small this 
time, with Frank's patch already in mainline and Chuck taking over IPV6 
issues) so the logic can be clear. I also have a feel that you never 
look into "sm-notify" ? Get nfs-util git tree source and take a look at 
the "README" file in the top directory. Either Neil Brown or Steve 
Dickson had added a nice "DAEMON STARTUP ORDER" there. Basically the 
whole issue is a two-steps thing:

1. statd is the process responsible for recording client address into 
the nsm directory
2. sm-notify is the process responsible for notifying clients about 
server reboot activities.

The "sm-notify" design allows the flexibility of notifying clients with 
different cluster configurations, including yours. I think you already 
know this stuff very well... just do a "man sm-notify" ... it should 
clear up your confusions.

-- Wendy

Re: [patch] fix statd -n

[Janne Karhunen]

On Sat, May 3, 2008 at 11:29 AM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:

> > >  I think you mis-understand our approach. With our patch, you do *not*
> need
> > > to do anything if you have only one single (floating) interface to do
> nfs
> > > export (and you can have the machine configured to use other hostname).
> The
> > > statd's "my_name" is correctly filled (from kernel by our patch) - it is
> no
> > > longer bound to 127.0.0.1.
> > >
> > >
> >
> > What happens when it fails over to node that has
> > different name?  Mon_name in NOTIFY changes
> > while address it's coming from does not. Which
> > one should client believe - the address or the
> > name?
> >
> > But OK, I probably need to dig out the patch to
> > have a look.
> >
> >
>  In the middle of re-doing our old patch (it should be very small this time,
> with Frank's patch already in mainline and Chuck taking over IPV6 issues) so
> the logic can be clear. I also have a feel that you never look into
> "sm-notify" ?

I didn't, as I was under a (apparently false) impression
that this indeed is a separate, manual way to trigger
the notification for whatever purpose. My apologies,


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Fri, May 02, 2008 at 08:24:25PM -0400, Janne Karhunen wrote:
> On Fri, May 2, 2008 at 6:33 PM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:
> 
> >  Maybe I mislead you in previous post. The "-H" is only an "example" - to
> > show people how to selectively move an ip address around without affecting
> > other co-existing nfs ip interface on the same server. I did plan to submit
> > a complete user mode patch so the program after "-H" will have a default
> > executable (but user still can use their own if they don't like our nlm
> > directory structure).
> 
> Even if you ship the default executable user would
> still need to go and edit it (add the address). Well,
> I guess it would not be that much harder than
> specifying the -n, but somehow this does seem
> more arcane (for simple setups).

In any case, if there's still a legimate use case for -n, even if it's
not great, we should err on the side of fixing it just to save problems
for anyone with an existing working setup.

If -n has never worked at all for anyone, then OK, let's get rid of it.

--b.

Re: [patch] fix statd -n

[Wendy Cheng]

J. Bruce Fields wrote:
> On Fri, May 02, 2008 at 08:24:25PM -0400, Janne Karhunen wrote:
>   
>> On Fri, May 2, 2008 at 6:33 PM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:
>>
>>     
>>>  Maybe I mislead you in previous post. The "-H" is only an "example" - to
>>> show people how to selectively move an ip address around without affecting
>>> other co-existing nfs ip interface on the same server. I did plan to submit
>>> a complete user mode patch so the program after "-H" will have a default
>>> executable (but user still can use their own if they don't like our nlm
>>> directory structure).
>>>       
>> Even if you ship the default executable user would
>> still need to go and edit it (add the address). Well,
>> I guess it would not be that much harder than
>> specifying the -n, but somehow this does seem
>> more arcane (for simple setups).
>>     
>
> In any case, if there's still a legimate use case for -n, even if it's
> not great, we should err on the side of fixing it just to save problems
> for anyone with an existing working setup.
>
> If -n has never worked at all for anyone, then OK, let's get rid of it.
>
> --b.
>   
Simpler code and consolidated logic flow are always better - would like 
to vote for its deletion.

I had a very bad experience with a filesystem (*cough*) that accepted 
few mis-understood boundary condition fixes that ended up disturbing the 
core logic. It still couldn't recover from it and the code churning 
keeps going on until today.

-- Wendy

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, May 5, 2008 at 10:59 AM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:

> > In any case, if there's still a legimate use case for -n, even if it's
> > not great, we should err on the side of fixing it just to save problems
> > for anyone with an existing working setup.
> >
>  Simpler code and consolidated logic flow are always better - would like to
> vote for its deletion.

So the solution would be to use -H with a script that
does sm-notify with user specified name (-v)? Kernel
name can't be used directly as it might migrate to
node that has different name.


-- 
// Janne

Re: [patch] fix statd -n

[Wendy Cheng]

Janne Karhunen wrote:
> On Mon, May 5, 2008 at 10:59 AM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:
>
>   
>>> In any case, if there's still a legimate use case for -n, even if it's
>>> not great, we should err on the side of fixing it just to save problems
>>> for anyone with an existing working setup.
>>>
>>>       
>>  Simpler code and consolidated logic flow are always better - would like to
>> vote for its deletion.
>>     
>
> So the solution would be to use -H with a script that
> does sm-notify with user specified name (-v)? Kernel
> name can't be used directly as it might migrate to
> node that has different name.
>
>
>   
No. "-H" is only for different export (server IP) interfaces. Say server 
has 10.1.1.1 and 10.1.1.2 as NFS export interfaces. If 10.1.1.1 is 
migrated but 10.1.1.2 stays, you would like to notify the nfs clients 
coming in from 10.1.1.1 but not the clients from 10.1.1.2, you need to 
structure your sm directory seperately. That is the usage of "-H" (to 
allow admin to organize client IPs).

For a single export IP interface like you have (even you have a seperate 
hostname), ignore "-H". I believe "sm-notify -v" should be enough - 
otherwise we have bugs - i.e., sm-notify should be part of your nfsd 
bring-up script.

-- Wendy

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, May 5, 2008 at 11:21 AM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:

>  No. "-H" is only for different export (server IP) interfaces. Say server
> has 10.1.1.1 and 10.1.1.2 as NFS export interfaces. If 10.1.1.1 is migrated
> but 10.1.1.2 stays, you would like to notify the nfs clients coming in from
> 10.1.1.1 but not the clients from 10.1.1.2, you need to structure your sm
> directory seperately. That is the usage of "-H" (to allow admin to organize
> client IPs).

Right.


>  For a single export IP interface like you have (even you have a seperate
> hostname), ignore "-H". I believe "sm-notify -v" should be enough -
> otherwise we have bugs - i.e., sm-notify should be part of your nfsd
> bring-up script.

As statd runs this directly and reads name for -v from
MY_NAME, how exactly would you pass it MY_NAME
given that -n is removed?


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Mon, May 05, 2008 at 11:23:37AM -0400, Janne Karhunen wrote:
> On Mon, May 5, 2008 at 11:21 AM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:
> 
> >  No. "-H" is only for different export (server IP) interfaces. Say server
> > has 10.1.1.1 and 10.1.1.2 as NFS export interfaces. If 10.1.1.1 is migrated
> > but 10.1.1.2 stays, you would like to notify the nfs clients coming in from
> > 10.1.1.1 but not the clients from 10.1.1.2, you need to structure your sm
> > directory seperately. That is the usage of "-H" (to allow admin to organize
> > client IPs).
> 
> Right.
> 
> 
> >  For a single export IP interface like you have (even you have a seperate
> > hostname), ignore "-H". I believe "sm-notify -v" should be enough -
> > otherwise we have bugs - i.e., sm-notify should be part of your nfsd
> > bring-up script.
> 
> As statd runs this directly and reads name for -v from
> MY_NAME,

Nope; see the -L option to statd.

--b.

> how exactly would you pass it MY_NAME
> given that -n is removed?

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, May 5, 2008 at 11:23 AM, Janne Karhunen
<janne.karhunen@gmail.com> wrote:

>  >  For a single export IP interface like you have (even you have a seperate
>  > hostname), ignore "-H". I believe "sm-notify -v" should be enough -
>  > otherwise we have bugs - i.e., sm-notify should be part of your nfsd
>  > bring-up script.
>
>  As statd runs this directly and reads name for -v from
>  MY_NAME, how exactly would you pass it MY_NAME
>  given that -n is removed?

So users should add this themselves into their scripts?


-- 
// Janne

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, May 5, 2008 at 11:25 AM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > As statd runs this directly and reads name for -v from
>  > MY_NAME,
>
>  Nope; see the -L option to statd.

So the let the user worry approach. Statd does not
notify anyone and user is supposed to do it with
sm-notify manually. This will work, but -n looks
cleaner me..


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Mon, May 05, 2008 at 11:28:03AM -0400, Janne Karhunen wrote:
> On Mon, May 5, 2008 at 11:25 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> >  > As statd runs this directly and reads name for -v from
> >  > MY_NAME,
> >
> >  Nope; see the -L option to statd.
> 
> So the let the user worry approach. Statd does not
> notify anyone and user is supposed to do it with
> sm-notify manually.

We should modify distro startup scripts to run sm-notify and add -L to
statd, following the recommendations in nfs-utils/README.

That will leave the -v option to sm-notify, which will need to be added
somehow to handle the highly-available-nfs case.

> This will work, but -n looks cleaner me..

I've lost the thread here a little.  Let me try to summarize, and tell
me what I've forgotten:

So we've got multiple nfs servers that may serve the same export.  Only
one serves a given export at a time.

To keep the clients from having to understand this, we tell them to
mount a single "floating" ip address, and when we want to use a
different server, we move that floating ip address, and the clients
automatically follow it.  The only problem is that the new server
doesn't know about locks held on the old server.  We solve that problem
by telling clients that the server has rebooted, and that they must
reclaim locks.

To make that work, we need to ensure that statd notifies the clients
that "floating-ip" has rebooted.  (They will get confused if they are
notified that "some-other-ip" has rebooted, where "some-other-ip" is
just some random ip that the new server happens to use on a different
interface.)

In the case that you care about, a given server only has one floating ip
address pointing at it at a time.  For your case, you could start statd
either with:

	sm-notify -v <floating ip>
	statd -L

or (assuming it was fixed)

	statd -n <floating ip>

and either would do the job of notifying all monitored clients that
<floating ip> has "rebooted".

Wendy also worries about the case of a server with multiple floating
ip's that may move independently of each other, or with both floating
ip's and ip's that don't move.  In that case we want to notify only
some subset of the clients.

So she uses 'statd -H' to modify the way statd stores its records, so
that it can easily distinguish between clients that are associated with
a floating ip that's moving (and need to be notified), and clients that
aren't (and don't need to be notified right now).

The statd -n option has actually *never* worked, in any previous version
of nfs-utils--with it specified, statd can no longer communicate with
the nfs server on the same host.  That means the server won't find out
when a client reboots.  (But I guess even the broken -n may still have
been sufficient to allow clients to monitor the server and find out when
the server reboots.)

--b.

Re: [patch] fix statd -n

[Wendy Cheng]

Janne Karhunen wrote:
> On Mon, May 5, 2008 at 11:25 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
>   
>>  > As statd runs this directly and reads name for -v from
>>  > MY_NAME,
>>
>>  Nope; see the -L option to statd.
>>     
>
> So the let the user worry approach. Statd does not
> notify anyone and user is supposed to do it with
> sm-notify manually. This will work, but -n looks
> cleaner me..
>
>   

Or we can do this ...
If user specified "statd -n", we run "sm-notify -v" command for them ? 
So we don't need to fork the code but also keep someone happy ?

Give me one day to see whether it is doable ?

-- Wendy

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, May 5, 2008 at 12:00 PM, Wendy Cheng <s.wendy.cheng@gmail.com> wrote:

> > >  > As statd runs this directly and reads name for -v from
> > >  > MY_NAME,
> > >
> > >  Nope; see the -L option to statd.
> > >
> >
> > So the let the user worry approach. Statd does not
> > notify anyone and user is supposed to do it with
> > sm-notify manually. This will work, but -n looks
> > cleaner me..
>
>  Or we can do this ...
>  If user specified "statd -n", we run "sm-notify -v" command for them ? So
> we don't need to fork the code but also keep someone happy ?

Ok, so -n remains..


>  Give me one day to see whether it is doable ?

Given above 1.1.* probably works already, but
it does not address the ANY bind - but do you
even wish that to be addressed?


-- 
// Janne

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, May 5, 2008 at 11:58 AM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > So the let the user worry approach. Statd does not
>  > notify anyone and user is supposed to do it with
>  > sm-notify manually.
>
>  We should modify distro startup scripts to run sm-notify and add -L to
>  statd, following the recommendations in nfs-utils/README.
>
>  That will leave the -v option to sm-notify, which will need to be added
>  somehow to handle the highly-available-nfs case.

Humm ... imho this does not add anything to current
1.1.* tree. Exactly same things happen when -n is
being used.


>  > This will work, but -n looks cleaner me..
>
>  I've lost the thread here a little.  Let me try to summarize, and tell
>  me what I've forgotten:
>
>  So we've got multiple nfs servers that may serve the same export.  Only
>  one serves a given export at a time.
>
>  To keep the clients from having to understand this, we tell them to
>  mount a single "floating" ip address, and when we want to use a
>  different server, we move that floating ip address, and the clients
>  automatically follow it.  The only problem is that the new server
>  doesn't know about locks held on the old server.  We solve that problem
>  by telling clients that the server has rebooted, and that they must
>  reclaim locks.
>
>  To make that work, we need to ensure that statd notifies the clients
>  that "floating-ip" has rebooted.  (They will get confused if they are
>  notified that "some-other-ip" has rebooted, where "some-other-ip" is
>  just some random ip that the new server happens to use on a different
>  interface.)

Exactly right.


>  The statd -n option has actually *never* worked, in any previous version
>  of nfs-utils--with it specified, statd can no longer communicate with
>  the nfs server on the same host.  That means the server won't find out
>  when a client reboots.  (But I guess even the broken -n may still have
>  been sufficient to allow clients to monitor the server and find out when
>  the server reboots.)

-n may work as is in 1.1.* tree:

a) It will bind to ANY, so kernel lockd will see packets coming
in from LO. 1.0.x bound the -n interface which will break.

b) sm-notify is run with -v and correct name (that will bind to
correct IP and use the right name)

So the only thing missing would be to limit the port visibility
of long-standing sockets; but this should probably be
discussed in another thread if you think it's worth it?


-- 
// Janne

Re: [patch] fix statd -n

[J. Bruce Fields]

On Mon, May 05, 2008 at 12:42:49PM -0400, Janne Karhunen wrote:
> On Mon, May 5, 2008 at 11:58 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >  The statd -n option has actually *never* worked, in any previous version
> >  of nfs-utils--with it specified, statd can no longer communicate with
> >  the nfs server on the same host.  That means the server won't find out
> >  when a client reboots.  (But I guess even the broken -n may still have
> >  been sufficient to allow clients to monitor the server and find out when
> >  the server reboots.)
> 
> -n may work as is in 1.1.* tree:

Oh, right, I'd forgotten about that change.

> a) It will bind to ANY, so kernel lockd will see packets coming
> in from LO. 1.0.x bound the -n interface which will break.
> 
> b) sm-notify is run with -v and correct name (that will bind to
> correct IP and use the right name)
> 
> So the only thing missing would be to limit the port visibility
> of long-standing sockets; but this should probably be
> discussed in another thread if you think it's worth it?

Is the only justification just to limit the consequences if a remote
exploit is found in statd?

If so, iptables seems the better solution, since it provides a uniform
way of limiting the exposure of the ports for all the various
nfs-related (and other) services, as opposed to just being a one-off
thing for statd.

--b.

Re: [patch] fix statd -n

[Janne Karhunen]

On Mon, May 5, 2008 at 1:02 PM, J. Bruce Fields <bfields@fieldses.org> wrote:

>  > So the only thing missing would be to limit the port visibility
>  > of long-standing sockets; but this should probably be
>  > discussed in another thread if you think it's worth it?
>
>  Is the only justification just to limit the consequences if a remote
>  exploit is found in statd?

It will also make it a LOT easier to debug and understand.
Discussions like this would have never existed given that
binds would have been specific.


-- 
// Janne