Re: [PATCH] exportfs: modify can_test() to use LONG_MAX when appropriate

[Harshula Jayasuriya <harshula@redhat.com>]

On Fri, 2013-11-15 at 17:38 +1100, NeilBrown wrote:
> On Fri, 15 Nov 2013 17:15:33 +1100 Harshula Jayasuriya <harshula@redhat.com>
> wrote:
> 
> > On Mon, 2013-11-11 at 22:20 +1100, Harshula Jayasuriya wrote:
> > > On Mon, 2013-11-11 at 18:53 +1100, NeilBrown wrote:
> > > > On Mon, 11 Nov 2013 18:20:34 +1100 Harshula Jayasuriya <harshula@redhat.com>
> > > > wrote:
> > > > 
> > > > > This patch is the nfs-utils patch corresponding to the kernel patch 
> > > > > commit 2f74f972d4cc7d83408ea0c32d424edcb44887bf "sunrpc: prepare NFS 
> > > > > for 2038": "The kernel sunrpc code needs to handle seconds since epoch 
> > > > > greater than 2147483647. This means functions that parse time as an 
> > > > > int need to handle it as time_t."
> > > > > 
> > > > > When appropriate exportfs should use LONG_MAX in can_test() instead of
> > > > > INT_MAX.
> > > > > 
> > > > > kernel INT_MAX + exportfs INT_MAX: "Tue Jan 19 03:14:08 UTC 2038"
> > > > > =================================================================
> > > > > 
> > > > > exportfs: /mnt/export does not support NFS export:
> > > > > ------------------------------------------------------------
> > > > >  # cat /proc/net/rpc/auth.unix.ip/content
> > > > >  #class IP domain
> > > > > ------------------------------------------------------------
> > > > > 
> > > > > + mount fail:
> > > > > ------------------------------------------------------------
> > > > >  # cat /proc/net/rpc/auth.unix.ip/content
> > > > >  #class IP domain
> > > > >  # expiry=2147483768 refcnt=2 flags=4
> > > > >  # nfsd 192.168.1.6 -no-domain-
> > > > > ------------------------------------------------------------
> > > > > 
> > > > > kernel LONG_MAX + exportfs INT_MAX: "Tue Jan 19 03:14:08 UTC 2038"
> > > > > ==================================================================
> > > > > 
> > > > > "exportfs: /mnt/export does not support NFS export":
> > > > > ------------------------------------------------------------
> > > > >  # cat /proc/net/rpc/auth.unix.ip/content
> > > > >  #class IP domain
> > > > > ------------------------------------------------------------
> > > > > 
> > > > > + mount success:
> > > > > ------------------------------------------------------------
> > > > >  # cat /proc/net/rpc/auth.unix.ip/content
> > > > >  #class IP domain
> > > > >  # expiry=2147485448 refcnt=2 flags=1
> > > > >  nfsd 192.168.1.6 *
> > > > > ------------------------------------------------------------
> > > > > 
> > > > > kernel LONG_MAX + exportfs LONG_MAX: "Tue Jan 19 03:14:08 UTC 2038"
> > > > > ===================================================================
> > > > > 
> > > > > exportfs:
> > > > > ------------------------------------------------------------
> > > > >  # cat /proc/net/rpc/auth.unix.ip/content
> > > > >  #class IP domain
> > > > >  # expiry=9223372036854775807 refcnt=1 flags=1
> > > > >  nfsd 0.0.0.0 -test-client-
> > > > > ------------------------------------------------------------
> > > > > 
> > > > > + mount success:
> > > > > ------------------------------------------------------------
> > > > >  # cat /proc/net/rpc/auth.unix.ip/content
> > > > >  #class IP domain
> > > > >  # expiry=2147485448 refcnt=2 flags=1
> > > > >  nfsd 192.168.1.6 *
> > > > >  # expiry=9223372036854775807 refcnt=1 flags=1
> > > > >  nfsd 0.0.0.0 -test-client-
> > > > > ------------------------------------------------------------
> > > > > 
> > > > > Signed-off-by: Harshula Jayasuriya <harshula@redhat.com>
> > > > > ---
> > > > >  utils/exportfs/exportfs.c | 20 ++++++++++++++++----
> > > > >  1 file changed, 16 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> > > > > index da5fe21..bccbed5 100644
> > > > > --- a/utils/exportfs/exportfs.c
> > > > > +++ b/utils/exportfs/exportfs.c
> > > > > @@ -27,6 +27,8 @@
> > > > >  #include <netdb.h>
> > > > >  #include <errno.h>
> > > > >  #include <dirent.h>
> > > > > +#include <limits.h>
> > > > > +#include <time.h>
> > > > >  
> > > > >  #include "sockaddr.h"
> > > > >  #include "misc.h"
> > > > > @@ -406,17 +408,27 @@ unexportfs(char *arg, int verbose)
> > > > >  
> > > > >  static int can_test(void)
> > > > >  {
> > > > > +	char buf[1024];
> > > > >  	int fd;
> > > > >  	int n;
> > > > > -	char *setup = "nfsd 0.0.0.0 2147483647 -test-client-\n";
> > > > > +
> > > > >  	fd = open("/proc/net/rpc/auth.unix.ip/channel", O_WRONLY);
> > > > > -	if ( fd < 0) return 0;
> > > > > -	n = write(fd, setup, strlen(setup));
> > > > > +	if (fd < 0)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (time(NULL) > INT_MAX)
> > > > > +		sprintf(buf, "nfsd 0.0.0.0 %ld -test-client-\n", LONG_MAX);
> > > > > +	else
> > > > > +		sprintf(buf, "nfsd 0.0.0.0 %d -test-client-\n", INT_MAX);
> > > > 
> > > > This generally makes sense, but I think the cut-off is a bit too fine.
> > > > If time(NULL) is exactly INT_MAX, then this might not do what is required.
> > > > How about
> > > >    if (time(NULL) > INT_MAX - (24*3600))
> > > > ??
> > > 
> > > Good point. Mount attempts seem to fail not because of when exportfs is
> > > run, but when the first mount request is made.
> > 
> > Clarification, what I said above is only true when the kernel does *not*
> > have commit 2f74f972d4cc7d83408ea0c32d424edcb44887bf.
> > 
> > I did a few more tests with kernel commit
> > 2f74f972d4cc7d83408ea0c32d424edcb44887bf and found:
> > 
> > 1) When the mount attempt arrives before the expiry time, set in
> > can_test(), is reached, /proc/net/rpc/auth.unix.ip/content will contain
> > the entry for the real mount attempt and test_export()'s "-test-client-"
> > entry. If the mount attempt arrives after the expiry time, only the
> > entry for the real mount attempt will appear. In both cases the mount
> > attempt succeeds.
> > 
> > 2) If the expiry time, set in can_test(), is older than the time when
> > exportfs is run, then test_export() will fail (write
> > to /proc/net/rpc/nfsd.export/channel fails) and validate_export() will
> > print the following error "exportfs: /mnt/export does not support NFS
> > export". However, the mount attempt still succeeds.
> > 
> > Neil, have I missed something here? It looks like we don't need to add
> > the tolerance to the time check.
> 
> The problem scenario that I imagine is:
> 
> > > > > +	if (time(NULL) > INT_MAX)
> 
> happens towards the end ofthe second when time(NULL) == INT_MAX.  The test
> fails and INT_MAX is stored in 'buf'.
> 
> By the time buf gets written to the kernel, time has passed and so the kernel
> sees that the entry written has passed.
> So an already-expired entry gets stored in the cache, which is clearly
> pointless and could conceivably result in the in the subsequent
> 'test_export()' call failing incorrectly.

True.

> It is admittedly a tiny window and it may not cause a problem.
> It just seems to me that waiting until the very last moment to switch over to
> used the new max-time value is pointless brinkmanship.
> It is obviously correct to use LONG_MAX a day (or even a month) before the
> change over.  So just do it and don't worry about trying to prove whether it
> is actually necessary or not.

Sure :-)

cya,
#

> NeilBrown
> 
> 
> 
> > cya,
> > #
> > 
> > > Is 1 day the right amount of tolerance? Or should we just go for a much
> > > larger tolerance, on the assumption that the running kernel will have
> > > commit 2f74f972d4cc7d83408ea0c32d424edcb44887bf by then? :-)
> > > 
> > > cya,
> > > #
> > > 
> > > 
> > > > > +
> > > > > +	n = write(fd, buf, strlen(buf));
> > > > >  	close(fd);
> > > > >  	if (n < 0)
> > > > >  		return 0;
> > > > > +
> > > > >  	fd = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY);
> > > > > -	if ( fd < 0) return 0;
> > > > > +	if (fd < 0)
> > > > > +		return 0;
> > > > >  	close(fd);
> > > > >  	return 1;
> > > > >  }
> > > 
> > > --
> > > 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
> > 
>