[PATCH] nfds: avoid gettimeofday for nfssvc_boot time

[PATCH] nfds: avoid gettimeofday for nfssvc_boot time

[Arnd Bergmann]

do_gettimeofday() is deprecated and we should generally use time64_t
based functions instead.

In case of nfsd, all three users of nfssvc_boot only use the initial
time as a unique token, and are not affected by it overflowing, so they
are not affected by the y2038 overflow.

This converts the structure to timespec64 anyway and adds comments
to all uses, to document that we have thought about it and avoid
having to look at it again.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/nfsd/netns.h    |  2 +-
 fs/nfsd/nfs3xdr.c  | 10 ++++++----
 fs/nfsd/nfs4proc.c |  5 +++--
 fs/nfsd/nfssvc.c   |  2 +-
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 3714231a9d0f..1c91391f4805 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -107,7 +107,7 @@ struct nfsd_net {
 	bool lockd_up;
 
 	/* Time of server startup */
-	struct timeval nfssvc_boot;
+	struct timespec64 nfssvc_boot;
 
 	/*
 	 * Max number of connections this nfsd container will allow. Defaults
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index bf444b664011..3579e0ae1131 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -747,8 +747,9 @@ nfs3svc_encode_writeres(struct svc_rqst *rqstp, __be32 *p)
 	if (resp->status == 0) {
 		*p++ = htonl(resp->count);
 		*p++ = htonl(resp->committed);
-		*p++ = htonl(nn->nfssvc_boot.tv_sec);
-		*p++ = htonl(nn->nfssvc_boot.tv_usec);
+		/* unique identifier, y2038 overflow can be ignored */
+		*p++ = htonl((u32)nn->nfssvc_boot.tv_sec);
+		*p++ = htonl(nn->nfssvc_boot.tv_nsec);
 	}
 	return xdr_ressize_check(rqstp, p);
 }
@@ -1118,8 +1119,9 @@ nfs3svc_encode_commitres(struct svc_rqst *rqstp, __be32 *p)
 	p = encode_wcc_data(rqstp, p, &resp->fh);
 	/* Write verifier */
 	if (resp->status == 0) {
-		*p++ = htonl(nn->nfssvc_boot.tv_sec);
-		*p++ = htonl(nn->nfssvc_boot.tv_usec);
+		/* unique identifier, y2038 overflow can be ignored */
+		*p++ = htonl((u32)nn->nfssvc_boot.tv_sec);
+		*p++ = htonl(nn->nfssvc_boot.tv_nsec);
 	}
 	return xdr_ressize_check(rqstp, p);
 }
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 7896f841482e..008ea0b627d0 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -564,10 +564,11 @@ static void gen_boot_verifier(nfs4_verifier *verifier, struct net *net)
 
 	/*
 	 * This is opaque to client, so no need to byte-swap. Use
-	 * __force to keep sparse happy
+	 * __force to keep sparse happy. y2038 time_t overflow is
+	 * irrelevant in this usage.
 	 */
 	verf[0] = (__force __be32)nn->nfssvc_boot.tv_sec;
-	verf[1] = (__force __be32)nn->nfssvc_boot.tv_usec;
+	verf[1] = (__force __be32)nn->nfssvc_boot.tv_nsec;
 	memcpy(verifier->data, verf, sizeof(verifier->data));
 }
 
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 6bbc717f40f2..28ff3e078af6 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -516,7 +516,7 @@ int nfsd_create_serv(struct net *net)
 		register_inet6addr_notifier(&nfsd_inet6addr_notifier);
 #endif
 	}
-	do_gettimeofday(&nn->nfssvc_boot);		/* record boot time */
+	ktime_get_real_ts64(&nn->nfssvc_boot); /* record boot time */
 	return 0;
 }
 
-- 
2.9.0

Re: [PATCH] nfds: avoid gettimeofday for nfssvc_boot time

[Jeff Layton]

On Thu, 2017-10-19 at 12:04 +0200, Arnd Bergmann wrote:
> do_gettimeofday() is deprecated and we should generally use time64_t
> based functions instead.
> 
> In case of nfsd, all three users of nfssvc_boot only use the initial
> time as a unique token, and are not affected by it overflowing, so they
> are not affected by the y2038 overflow.
> 
> This converts the structure to timespec64 anyway and adds comments
> to all uses, to document that we have thought about it and avoid
> having to look at it again.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  fs/nfsd/netns.h    |  2 +-
>  fs/nfsd/nfs3xdr.c  | 10 ++++++----
>  fs/nfsd/nfs4proc.c |  5 +++--
>  fs/nfsd/nfssvc.c   |  2 +-
>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 3714231a9d0f..1c91391f4805 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -107,7 +107,7 @@ struct nfsd_net {
>  	bool lockd_up;
>  
>  	/* Time of server startup */
> -	struct timeval nfssvc_boot;
> +	struct timespec64 nfssvc_boot;
>  
>  	/*
>  	 * Max number of connections this nfsd container will allow. Defaults
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index bf444b664011..3579e0ae1131 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -747,8 +747,9 @@ nfs3svc_encode_writeres(struct svc_rqst *rqstp, __be32 *p)
>  	if (resp->status == 0) {
>  		*p++ = htonl(resp->count);
>  		*p++ = htonl(resp->committed);
> -		*p++ = htonl(nn->nfssvc_boot.tv_sec);
> -		*p++ = htonl(nn->nfssvc_boot.tv_usec);
> +		/* unique identifier, y2038 overflow can be ignored */
> +		*p++ = htonl((u32)nn->nfssvc_boot.tv_sec);
> +		*p++ = htonl(nn->nfssvc_boot.tv_nsec);
>  	}
>  	return xdr_ressize_check(rqstp, p);
>  }
> @@ -1118,8 +1119,9 @@ nfs3svc_encode_commitres(struct svc_rqst *rqstp, __be32 *p)
>  	p = encode_wcc_data(rqstp, p, &resp->fh);
>  	/* Write verifier */
>  	if (resp->status == 0) {
> -		*p++ = htonl(nn->nfssvc_boot.tv_sec);
> -		*p++ = htonl(nn->nfssvc_boot.tv_usec);
> +		/* unique identifier, y2038 overflow can be ignored */
> +		*p++ = htonl((u32)nn->nfssvc_boot.tv_sec);
> +		*p++ = htonl(nn->nfssvc_boot.tv_nsec);
>  	}
>  	return xdr_ressize_check(rqstp, p);
>  }
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 7896f841482e..008ea0b627d0 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -564,10 +564,11 @@ static void gen_boot_verifier(nfs4_verifier *verifier, struct net *net)
>  
>  	/*
>  	 * This is opaque to client, so no need to byte-swap. Use
> -	 * __force to keep sparse happy
> +	 * __force to keep sparse happy. y2038 time_t overflow is
> +	 * irrelevant in this usage.
>  	 */
>  	verf[0] = (__force __be32)nn->nfssvc_boot.tv_sec;
> -	verf[1] = (__force __be32)nn->nfssvc_boot.tv_usec;
> +	verf[1] = (__force __be32)nn->nfssvc_boot.tv_nsec;
>  	memcpy(verifier->data, verf, sizeof(verifier->data));
>  }
>  
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 6bbc717f40f2..28ff3e078af6 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -516,7 +516,7 @@ int nfsd_create_serv(struct net *net)
>  		register_inet6addr_notifier(&nfsd_inet6addr_notifier);
>  #endif
>  	}
> -	do_gettimeofday(&nn->nfssvc_boot);		/* record boot time */
> +	ktime_get_real_ts64(&nn->nfssvc_boot); /* record boot time */
>  	return 0;
>  }
>  

I wonder if we'd be better off just using nfssvc_boot.tv_sec as the
verifier? I don't see us ever calling that ktime_get_real_ts64 more than
once per second for this purpose, and that would eliminate wraparound.
That said, wraparound is not a huge concern here anyway, so this is
certainly fine for now:

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH] nfds: avoid gettimeofday for nfssvc_boot time

[Arnd Bergmann]

On Thu, Oct 19, 2017 at 12:54 PM, Jeff Layton <jlayton@redhat.com> wrote:
>
> I wonder if we'd be better off just using nfssvc_boot.tv_sec as the
> verifier? I don't see us ever calling that ktime_get_real_ts64 more than
> once per second for this purpose, and that would eliminate wraparound.
> That said, wraparound is not a huge concern here anyway, so this is
> certainly fine for now:

I now have the feeling that we had previously had the same discussion
when someone else submitted a similar patch that ended up never getting
merged. I might also be confusing this with a different subsystem that
had the same requirement.

If we want this to be as unique as possible and also never (within
a few hundred years) wrap, we could call ktime_get_real_ns(), which
returns a 64-bit nanoseconds number.

> Reviewed-by: Jeff Layton <jlayton@redhat.com>

Thanks.

     Arnd

Re: [PATCH] nfds: avoid gettimeofday for nfssvc_boot time

[J. Bruce Fields]

On Thu, Oct 19, 2017 at 01:04:35PM +0200, Arnd Bergmann wrote:
> On Thu, Oct 19, 2017 at 12:54 PM, Jeff Layton <jlayton@redhat.com> wrote:
> >
> > I wonder if we'd be better off just using nfssvc_boot.tv_sec as the
> > verifier? I don't see us ever calling that ktime_get_real_ts64 more than
> > once per second for this purpose, and that would eliminate wraparound.
> > That said, wraparound is not a huge concern here anyway, so this is
> > certainly fine for now:

It might reduce the chances of a collision if someone is doing extreme
boot-time optimization, or if time goes backwards for some reason?

> I now have the feeling that we had previously had the same discussion
> when someone else submitted a similar patch that ended up never getting
> merged. I might also be confusing this with a different subsystem that
> had the same requirement.
> 
> If we want this to be as unique as possible and also never (within
> a few hundred years) wrap, we could call ktime_get_real_ns(), which
> returns a 64-bit nanoseconds number.

Anyway, no objection to doing this differently if someone wants, but
I'll just take this patch for now.

Thanks.--b.

> 
> > Reviewed-by: Jeff Layton <jlayton@redhat.com>
> 
> Thanks.
> 
>      Arnd