linux-kernel - Re: [PATCH] nfds: avoid gettimeofday for nfssvc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1508410486.4912.11.camel@redhat.com>
Date:   Thu, 19 Oct 2017 06:54:46 -0400
From:   Jeff Layton <jlayton@...hat.com>
To:     Arnd Bergmann <arnd@...db.de>, y2038@...ts.linaro.org,
        "J. Bruce Fields" <bfields@...ldses.org>
Cc:     Deepa Dinamani <deepa.kernel@...il.com>,
        linux-fsdevel@...r.kernel.org,
        Trond Myklebust <trond.myklebust@...marydata.com>,
        NeilBrown <neilb@...e.com>, Kinglong Mee <kinglongmee@...il.com>,
        linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nfds: avoid gettimeofday for nfssvc_boot time

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@...db.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@...hat.com>