[PATCH 0/5] fs: nfsd: Remove time_t

[PATCH 0/5] fs: nfsd: Remove time_t

[Ksenija Stanojevic]

Replace time_t with u32 in variables boot_time, nfsd4_lease,
nfsd4_grace, dl_time, oo_time.

Changes have not been made to variables that use inode timestamp
or function seconds_since_boot, which aren't y2038 safe.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>

Ksenija Stanojevic (5):
  fs: nfsd: Replace time_t with time64_t in boot_time
  fs: nfsd: Replace time_t with u32 in nfsd4_lease
  fs: nfsd: Replace time_t with u32 in nfsd4_grace
  fs: nfsd: Replace time_t with u32 dl_time
  fs: nfsd: Replace time_t with u32 oo_time

 fs/nfsd/netns.h        |  6 +++---
 fs/nfsd/nfs4callback.c |  2 +-
 fs/nfsd/nfs4recover.c  |  6 +++---
 fs/nfsd/nfs4state.c    | 20 ++++++++++----------
 fs/nfsd/nfsctl.c       |  6 +++---
 fs/nfsd/state.h        |  6 +++---
 6 files changed, 23 insertions(+), 23 deletions(-)

-- 
1.9.1

[PATCH 1/5] fs: nfsd: Replace time_t with u32 in boot_time

[Ksenija Stanojevic]

Replace time_t type and get_seconds function which are not y2038 safe on
32-bit systems. Function ktime_get_seconds use monotonic instead of real
time and therefore will not cause overflow.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 fs/nfsd/netns.h       | 2 +-
 fs/nfsd/nfs4recover.c | 6 +++---
 fs/nfsd/nfs4state.c   | 8 ++++----
 fs/nfsd/state.h       | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index d8b16c2..d5cce15 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -53,7 +53,7 @@ struct nfsd_net {
 
 	struct lock_manager nfsd4_manager;
 	bool grace_ended;
-	time_t boot_time;
+	u32 boot_time;
 
 	/*
 	 * reclaim_str_hashtbl[] holds known client info from previous reset/reboot
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index e3d4709..eec431a 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1164,7 +1164,7 @@ nfsd4_cltrack_client_has_session(struct nfs4_client *clp)
 }
 
 static char *
-nfsd4_cltrack_grace_start(time_t grace_start)
+nfsd4_cltrack_grace_start(unsigned long grace_start)
 {
 	int copied;
 	size_t len;
@@ -1177,7 +1177,7 @@ nfsd4_cltrack_grace_start(time_t grace_start)
 	if (!result)
 		return result;
 
-	copied = snprintf(result, len, GRACE_START_ENV_PREFIX "%ld",
+	copied = snprintf(result, len, GRACE_START_ENV_PREFIX "%lu",
 				grace_start);
 	if (copied >= len) {
 		/* just return nothing if output was truncated */
@@ -1388,7 +1388,7 @@ nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
 	char *legacy;
 	char timestr[22]; /* FIXME: better way to determine max size? */
 
-	sprintf(timestr, "%ld", nn->boot_time);
+	sprintf(timestr, "%x", nn->boot_time);
 	legacy = nfsd4_cltrack_legacy_topdir();
 	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy, NULL);
 	kfree(legacy);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0f1d569..8245236 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1612,9 +1612,9 @@ STALE_CLIENTID(clientid_t *clid, struct nfsd_net *nn)
 	 * precisely 2^32 (about 136 years) before this one.  That seems
 	 * a safe assumption:
 	 */
-	if (clid->cl_boot == (u32)nn->boot_time)
+	if (clid->cl_boot == nn->boot_time)
 		return 0;
-	dprintk("NFSD stale clientid (%08x/%08x) boot_time %08lx\n",
+	dprintk("NFSD stale clientid (%08x/%08x) boot_time %08x\n",
 		clid->cl_boot, clid->cl_id, nn->boot_time);
 	return 1;
 }
@@ -4276,7 +4276,7 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	__be32 status;
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
 
-	dprintk("process_renew(%08x/%08x): starting\n", 
+	dprintk("process_renew(%08x/%08x): starting\n",
 			clid->cl_boot, clid->cl_id);
 	status = lookup_clientid(clid, cstate, nn);
 	if (status)
@@ -6603,7 +6603,7 @@ nfs4_state_start_net(struct net *net)
 	ret = nfs4_state_create_net(net);
 	if (ret)
 		return ret;
-	nn->boot_time = get_seconds();
+	nn->boot_time = ktime_get_seconds();
 	nn->grace_ended = false;
 	nn->nfsd4_manager.block_opens = true;
 	locks_start_grace(net, &nn->nfsd4_manager);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 583ffc1..5c49ed65 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -305,7 +305,7 @@ struct nfs4_client {
 #endif
 	struct xdr_netobj	cl_name; 	/* id generated by client */
 	nfs4_verifier		cl_verifier; 	/* generated by client */
-	time_t                  cl_time;        /* time of last lease renewal */
+	u32			cl_time;        /* time of last lease renewal */
 	struct sockaddr_storage	cl_addr; 	/* client ipaddress */
 	bool			cl_mach_cred;	/* SP4_MACH_CRED in force */
 	struct svc_cred		cl_cred; 	/* setclientid principal */
-- 
1.9.1

Re: [Y2038] [PATCH 1/5] fs: nfsd: Replace time_t with u32 in boot_time

[Arnd Bergmann]

On Tuesday 27 October 2015 09:08:35 Ksenija Stanojevic wrote:
> Replace time_t type and get_seconds function which are not y2038 safe on
> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
> time and therefore will not cause overflow.
> 
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>

I don't think using monotonic time is safe here: 

> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0f1d569..8245236 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1612,9 +1612,9 @@ STALE_CLIENTID(clientid_t *clid, struct nfsd_net *nn)
>  	 * precisely 2^32 (about 136 years) before this one.  That seems
>  	 * a safe assumption:
>  	 */
> -	if (clid->cl_boot == (u32)nn->boot_time)
> +	if (clid->cl_boot == nn->boot_time)
>  		return 0;

It looks like clid->cl_boot comes from a machine on the other side
of the network, and we need to ensure that the values are unique
above all things.

> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -305,7 +305,7 @@ struct nfs4_client {
>  #endif
>  	struct xdr_netobj	cl_name; 	/* id generated by client */
>  	nfs4_verifier		cl_verifier; 	/* generated by client */
> -	time_t                  cl_time;        /* time of last lease renewal */
> +	u32			cl_time;        /* time of last lease renewal */
>  	struct sockaddr_storage	cl_addr; 	/* client ipaddress */
>  	bool			cl_mach_cred;	/* SP4_MACH_CRED in force */
>  	struct svc_cred		cl_cred; 	/* setclientid principal */
> 

This change seems unrelated to the rest of this patch.

	Arnd

Re: [Y2038] [PATCH 1/5] fs: nfsd: Replace time_t with u32 in boot_time

[Ksenija Stanojević]

On Tue, Nov 3, 2015 at 4:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 27 October 2015 09:08:35 Ksenija Stanojevic wrote:
>> Replace time_t type and get_seconds function which are not y2038 safe on
>> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
>> time and therefore will not cause overflow.
>>
>> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
>
> I don't think using monotonic time is safe here:

I was under the impression that comment:

* We're assuming the clid was not given out from a boot
* precisely 2^32 (about 136 years) before this one.  That seems
* a safe assumption:

is implying that monotonic time is used (should be used).

>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 0f1d569..8245236 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1612,9 +1612,9 @@ STALE_CLIENTID(clientid_t *clid, struct nfsd_net *nn)
>>        * precisely 2^32 (about 136 years) before this one.  That seems
>>        * a safe assumption:
>>        */
>> -     if (clid->cl_boot == (u32)nn->boot_time)
>> +     if (clid->cl_boot == nn->boot_time)
>>               return 0;
>
> It looks like clid->cl_boot comes from a machine on the other side
> of the network, and we need to ensure that the values are unique
> above all things.
>
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -305,7 +305,7 @@ struct nfs4_client {
>>  #endif
>>       struct xdr_netobj       cl_name;        /* id generated by client */
>>       nfs4_verifier           cl_verifier;    /* generated by client */
>> -     time_t                  cl_time;        /* time of last lease renewal */
>> +     u32                     cl_time;        /* time of last lease renewal */
>>       struct sockaddr_storage cl_addr;        /* client ipaddress */
>>       bool                    cl_mach_cred;   /* SP4_MACH_CRED in force */
>>       struct svc_cred         cl_cred;        /* setclientid principal */
>>
>
> This change seems unrelated to the rest of this patch.
>
>         Arnd

Re: [Outreachy kernel] Re: [Y2038] [PATCH 1/5] fs: nfsd: Replace time_t with u32 in boot_time

[Arnd Bergmann]

On Tuesday 03 November 2015 18:17:48 Ksenija Stanojević wrote:
> On Tue, Nov 3, 2015 at 4:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 27 October 2015 09:08:35 Ksenija Stanojevic wrote:
> >> Replace time_t type and get_seconds function which are not y2038 safe on
> >> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
> >> time and therefore will not cause overflow.
> >>
> >> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> >
> > I don't think using monotonic time is safe here:
> 
> I was under the impression that comment:
> 
> * We're assuming the clid was not given out from a boot
> * precisely 2^32 (about 136 years) before this one.  That seems
> * a safe assumption:
> 
> is implying that monotonic time is used (should be used).

You almost convinced me, and I just spent an hour trying to
understand what is actually going on. I think I've got it now,
and this is what happens:

The NFS client sends a SETCLIENTID request to the server, which
generates the clientid using boot_time and a unique (for this instance)
number, together these two make up a 'clientid_t'. This is sent
back to the client in the response, and it gets encoded as part
of nfsd4_encode_setclientid() in the line

        p = xdr_encode_opaque_fixed(p, &scd->se_clientid, 8);

The reason this does not show up when you grep for cl_boot is that
xdr_encode_opaque_fixed just takes the number as set of eight bytes
that are not meaningful to the client. The client then sends back
the number to the server on other requests, e.g. in nfsd4_sessionid,
and the server checks cl_boot to ensure that is the same number
as boot_time, and this way it can tell whether the server has been
rebooted while keeping the client session running.

If you use monotonic times for generating boot_time, that means
it could be the same after a reboot because the time to get
from poweron to starting the NFS server is relatively deterministic.
This however defeats the purpose of comparing the boot times.

So you have to use real time here, but it is safe to truncate the
time to the low 32 bits because the number is never used as the
actual time, just as a token that is required to be unique
for each time the server gets booted.

	Arnd

[PATCH 2/5] fs: nfsd: Replace time_t with u32 in nfsd4_lease

[Ksenija Stanojevic]

Replace time_t type and get_seconds function which are not y2038 safe on
32-bit systems. Function ktime_get_seconds use monotonic instead of real
time and therefore will not cause overflow.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 fs/nfsd/netns.h        | 2 +-
 fs/nfsd/nfs4callback.c | 2 +-
 fs/nfsd/nfs4state.c    | 6 +++---
 fs/nfsd/nfsctl.c       | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index d5cce15..85b114f 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -94,7 +94,7 @@ struct nfsd_net {
 	bool in_grace;
 	struct nfsd4_client_tracking_ops *client_tracking_ops;
 
-	time_t nfsd4_lease;
+	u32 nfsd4_lease;
 	time_t nfsd4_grace;
 
 	bool nfsd_net_up;
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index e7f50c4..8a177c6 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -681,7 +681,7 @@ static const struct rpc_program cb_program = {
 static int max_cb_time(struct net *net)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
-	return max(nn->nfsd4_lease/10, (time_t)1) * HZ;
+	return max(nn->nfsd4_lease/10, (u32)1) * HZ;
 }
 
 static struct rpc_cred *callback_cred;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8245236..4bceb878 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4332,8 +4332,8 @@ nfs4_laundromat(struct nfsd_net *nn)
 	struct nfs4_delegation *dp;
 	struct nfs4_ol_stateid *stp;
 	struct list_head *pos, *next, reaplist;
-	time_t cutoff = get_seconds() - nn->nfsd4_lease;
-	time_t t, new_timeo = nn->nfsd4_lease;
+	u32 cutoff = ktime_get_seconds() - nn->nfsd4_lease;
+	u32 t, new_timeo = nn->nfsd4_lease;
 
 	dprintk("NFSD: laundromat service - starting\n");
 	nfsd4_end_grace(nn);
@@ -4399,7 +4399,7 @@ nfs4_laundromat(struct nfsd_net *nn)
 	}
 	spin_unlock(&nn->client_lock);
 
-	new_timeo = max_t(time_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
+	new_timeo = max_t(u32, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
 	return new_timeo;
 }
 
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 9690cb4..6e0cfd6 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -932,7 +932,7 @@ static ssize_t write_maxconn(struct file *file, char *buf, size_t size)
 
 #ifdef CONFIG_NFSD_V4
 static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
-				  time_t *time, struct nfsd_net *nn)
+				  u32 *time, struct nfsd_net *nn)
 {
 	char *mesg = buf;
 	int rv, i;
@@ -964,7 +964,7 @@ static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
 }
 
 static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
-				time_t *time, struct nfsd_net *nn)
+				u32 *time, struct nfsd_net *nn)
 {
 	ssize_t rv;
 
-- 
1.9.1

Re: [Y2038] [PATCH 2/5] fs: nfsd: Replace time_t with u32 in nfsd4_lease

[Arnd Bergmann]

On Tuesday 27 October 2015 09:09:35 Ksenija Stanojevic wrote:
> Replace time_t type and get_seconds function which are not y2038 safe on
> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
> time and therefore will not cause overflow.
> 
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>

You are missing an explanation about why the u32 type is used. The
change to that type looks absolutely appropriate here, but it seems
unrelated to what you write above.

> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index d5cce15..85b114f 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -94,7 +94,7 @@ struct nfsd_net {
>  	bool in_grace;
>  	struct nfsd4_client_tracking_ops *client_tracking_ops;
>  
> -	time_t nfsd4_lease;
> +	u32 nfsd4_lease;
>  	time_t nfsd4_grace;
>  
>  	bool nfsd_net_up;
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index e7f50c4..8a177c6 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -681,7 +681,7 @@ static const struct rpc_program cb_program = {
>  static int max_cb_time(struct net *net)
>  {
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> -	return max(nn->nfsd4_lease/10, (time_t)1) * HZ;
> +	return max(nn->nfsd4_lease/10, (u32)1) * HZ;
>  }

coding style: it would be helpful to convert this to use max_t()
instead of max() to avoid the cast.

>  static struct rpc_cred *callback_cred;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 8245236..4bceb878 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4332,8 +4332,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>  	struct nfs4_delegation *dp;
>  	struct nfs4_ol_stateid *stp;
>  	struct list_head *pos, *next, reaplist;
> -	time_t cutoff = get_seconds() - nn->nfsd4_lease;
> -	time_t t, new_timeo = nn->nfsd4_lease;
> +	u32 cutoff = ktime_get_seconds() - nn->nfsd4_lease;
> +	u32 t, new_timeo = nn->nfsd4_lease;
>  
>  	dprintk("NFSD: laundromat service - starting\n");
>  	nfsd4_end_grace(nn);

The change to "cutoff" needs to be done together with the 'cl_time'
change from patch 1, and all the users of that variable.

> @@ -4399,7 +4399,7 @@ nfs4_laundromat(struct nfsd_net *nn)
>  	}
>  	spin_unlock(&nn->client_lock);
>  
> -	new_timeo = max_t(time_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> +	new_timeo = max_t(u32, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>  	return new_timeo;
>  }
>  
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 9690cb4..6e0cfd6 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -932,7 +932,7 @@ static ssize_t write_maxconn(struct file *file, char *buf, size_t size)
>  
>  #ifdef CONFIG_NFSD_V4
>  static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
> -				  time_t *time, struct nfsd_net *nn)
> +				  u32 *time, struct nfsd_net *nn)
>  {
>  	char *mesg = buf;
>  	int rv, i;
> @@ -964,7 +964,7 @@ static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
>  }
>  
>  static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
> -				time_t *time, struct nfsd_net *nn)
> +				u32 *time, struct nfsd_net *nn)
>  {
>  	ssize_t rv;

This function is called for both nfsd4_lease and nfsd4_grace, so by changing
only one of the two types, you generate a compiler warning.

	Arnd

Re: [Y2038] [PATCH 2/5] fs: nfsd: Replace time_t with u32 in nfsd4_lease

[Ksenija Stanojević]

On Tue, Nov 3, 2015 at 4:14 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 27 October 2015 09:09:35 Ksenija Stanojevic wrote:
>> Replace time_t type and get_seconds function which are not y2038 safe on
>> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
>> time and therefore will not cause overflow.
>>
>> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
>
> You are missing an explanation about why the u32 type is used. The
> change to that type looks absolutely appropriate here, but it seems
> unrelated to what you write above.

It' safe to assume that system will be rebooted in 136 years and 32-bit
number is enough to store a monotonic time value.

>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>> index d5cce15..85b114f 100644
>> --- a/fs/nfsd/netns.h
>> +++ b/fs/nfsd/netns.h
>> @@ -94,7 +94,7 @@ struct nfsd_net {
>>       bool in_grace;
>>       struct nfsd4_client_tracking_ops *client_tracking_ops;
>>
>> -     time_t nfsd4_lease;
>> +     u32 nfsd4_lease;
>>       time_t nfsd4_grace;
>>
>>       bool nfsd_net_up;
>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>> index e7f50c4..8a177c6 100644
>> --- a/fs/nfsd/nfs4callback.c
>> +++ b/fs/nfsd/nfs4callback.c
>> @@ -681,7 +681,7 @@ static const struct rpc_program cb_program = {
>>  static int max_cb_time(struct net *net)
>>  {
>>       struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>> -     return max(nn->nfsd4_lease/10, (time_t)1) * HZ;
>> +     return max(nn->nfsd4_lease/10, (u32)1) * HZ;
>>  }
>
> coding style: it would be helpful to convert this to use max_t()
> instead of max() to avoid the cast.
>
>>  static struct rpc_cred *callback_cred;
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 8245236..4bceb878 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4332,8 +4332,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>>       struct nfs4_delegation *dp;
>>       struct nfs4_ol_stateid *stp;
>>       struct list_head *pos, *next, reaplist;
>> -     time_t cutoff = get_seconds() - nn->nfsd4_lease;
>> -     time_t t, new_timeo = nn->nfsd4_lease;
>> +     u32 cutoff = ktime_get_seconds() - nn->nfsd4_lease;
>> +     u32 t, new_timeo = nn->nfsd4_lease;
>>
>>       dprintk("NFSD: laundromat service - starting\n");
>>       nfsd4_end_grace(nn);
>
> The change to "cutoff" needs to be done together with the 'cl_time'
> change from patch 1, and all the users of that variable.

You commented in patch 1 that change to cl_time is unrelated to other
changes in that patch. Should I make a new patch which changes cl_time
and cutoff or do all those changes in patch 1?

>> @@ -4399,7 +4399,7 @@ nfs4_laundromat(struct nfsd_net *nn)
>>       }
>>       spin_unlock(&nn->client_lock);
>>
>> -     new_timeo = max_t(time_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>> +     new_timeo = max_t(u32, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>>       return new_timeo;
>>  }
>>
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index 9690cb4..6e0cfd6 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -932,7 +932,7 @@ static ssize_t write_maxconn(struct file *file, char *buf, size_t size)
>>
>>  #ifdef CONFIG_NFSD_V4
>>  static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
>> -                               time_t *time, struct nfsd_net *nn)
>> +                               u32 *time, struct nfsd_net *nn)
>>  {
>>       char *mesg = buf;
>>       int rv, i;
>> @@ -964,7 +964,7 @@ static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
>>  }
>>
>>  static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
>> -                             time_t *time, struct nfsd_net *nn)
>> +                             u32 *time, struct nfsd_net *nn)
>>  {
>>       ssize_t rv;
>
> This function is called for both nfsd4_lease and nfsd4_grace, so by changing
> only one of the two types, you generate a compiler warning.

Ok, so changes to nfsd4_lease and nfsd4_grace should be done in one patch?

Re: [Y2038] [PATCH 2/5] fs: nfsd: Replace time_t with u32 in nfsd4_lease

[Arnd Bergmann]

On Monday 09 November 2015 20:37:22 Ksenija Stanojević wrote:
> On Tue, Nov 3, 2015 at 4:14 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 27 October 2015 09:09:35 Ksenija Stanojevic wrote:
> >> Replace time_t type and get_seconds function which are not y2038 safe on
> >> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
> >> time and therefore will not cause overflow.
> >>
> >> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> >
> > You are missing an explanation about why the u32 type is used. The
> > change to that type looks absolutely appropriate here, but it seems
> > unrelated to what you write above.
> 
> It' safe to assume that system will be rebooted in 136 years and 32-bit
> number is enough to store a monotonic time value.

Not exactly in this case: nfsd4_lease is not actually used to store
a monotonic time (which would be local to the system, starting at boot
and not shared with other systems), but instead stores a time interval
that *is* shared with other systems as a 32-bit value in
nfsd4_encode_fattr().

So in order to make it obvious to everyone what happens here, explain
that you picked this type because it matches the size on the wire protocol,
and also why the wire protocol is safe.

> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 8245236..4bceb878 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -4332,8 +4332,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> >>       struct nfs4_delegation *dp;
> >>       struct nfs4_ol_stateid *stp;
> >>       struct list_head *pos, *next, reaplist;
> >> -     time_t cutoff = get_seconds() - nn->nfsd4_lease;
> >> -     time_t t, new_timeo = nn->nfsd4_lease;
> >> +     u32 cutoff = ktime_get_seconds() - nn->nfsd4_lease;
> >> +     u32 t, new_timeo = nn->nfsd4_lease;
> >>
> >>       dprintk("NFSD: laundromat service - starting\n");
> >>       nfsd4_end_grace(nn);
> >
> > The change to "cutoff" needs to be done together with the 'cl_time'
> > change from patch 1, and all the users of that variable.
> 
> You commented in patch 1 that change to cl_time is unrelated to other
> changes in that patch. Should I make a new patch which changes cl_time
> and cutoff or do all those changes in patch 1?

Try to make it a separate patch. As you are dealing with rather complex
stuff here, the patches should be as small as possible while being
large enough to make sense on their own. I see now that there are
also 'oo_time' and 'dl_time' in the same function that are compared to
'cutoff'. I would try changing all four of these in one patch at first,
and then see if that patch is simple enough to still be explained
easily.

> >> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> >> index 9690cb4..6e0cfd6 100644
> >> --- a/fs/nfsd/nfsctl.c
> >> +++ b/fs/nfsd/nfsctl.c
> >> @@ -932,7 +932,7 @@ static ssize_t write_maxconn(struct file *file, char *buf, size_t size)
> >>
> >>  #ifdef CONFIG_NFSD_V4
> >>  static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
> >> -                               time_t *time, struct nfsd_net *nn)
> >> +                               u32 *time, struct nfsd_net *nn)
> >>  {
> >>       char *mesg = buf;
> >>       int rv, i;
> >> @@ -964,7 +964,7 @@ static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
> >>  }
> >>
> >>  static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
> >> -                             time_t *time, struct nfsd_net *nn)
> >> +                             u32 *time, struct nfsd_net *nn)
> >>  {
> >>       ssize_t rv;
> >
> > This function is called for both nfsd4_lease and nfsd4_grace, so by changing
> > only one of the two types, you generate a compiler warning.
> 
> Ok, so changes to nfsd4_lease and nfsd4_grace should be done in one patch?

Yes, I think so. Again, try it first as a combined patch for the two and then see
if the patch gets simpler or more complicated when split up.

	Arnd

[PATCH 3/5] fs: nfsd: Replace time_t with u32 in nfsd4_grace

[Ksenija Stanojevic]

Replace time_t type and get_seconds function which are not y2038 safe on
32-bit systems. Function ktime_get_seconds use monotonic instead of real
time and therefore will not cause overflow.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 fs/nfsd/netns.h     | 2 +-
 fs/nfsd/nfs4state.c | 2 +-
 fs/nfsd/nfsctl.c    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 85b114f..aeaf896 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -95,7 +95,7 @@ struct nfsd_net {
 	struct nfsd4_client_tracking_ops *client_tracking_ops;
 
 	u32 nfsd4_lease;
-	time_t nfsd4_grace;
+	u32 nfsd4_grace;
 
 	bool nfsd_net_up;
 	bool lockd_up;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4bceb878..4b3c87d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6608,7 +6608,7 @@ nfs4_state_start_net(struct net *net)
 	nn->nfsd4_manager.block_opens = true;
 	locks_start_grace(net, &nn->nfsd4_manager);
 	nfsd4_client_tracking_init(net);
-	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
+	printk(KERN_INFO "NFSD: starting %x-second grace period (net %p)\n",
 	       nn->nfsd4_grace, net);
 	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
 	return 0;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 6e0cfd6..e1709ce 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -960,7 +960,7 @@ static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size,
 		*time = i;
 	}
 
-	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);
+	return scnprintf(buf, (u32)SIMPLE_TRANSACTION_LIMIT, "%x\n", *time);
 }
 
 static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
-- 
1.9.1

Re: [Y2038] [PATCH 3/5] fs: nfsd: Replace time_t with u32 in nfsd4_grace

[Arnd Bergmann]

On Tuesday 27 October 2015 09:10:45 Ksenija Stanojevic wrote:
> Replace time_t type and get_seconds function which are not y2038 safe on
> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
> time and therefore will not cause overflow.
> 
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>

I think this patch needs to be folded into the patch that changes the
type for nfsd4_lease, as mentioned there.

>  	locks_start_grace(net, &nn->nfsd4_manager);
>  	nfsd4_client_tracking_init(net);
> -	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
> +	printk(KERN_INFO "NFSD: starting %x-second grace period (net %p)\n",
>  	       nn->nfsd4_grace, net);
>  	queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);

>  
> -	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);
> +	return scnprintf(buf, (u32)SIMPLE_TRANSACTION_LIMIT, "%x\n", *time);
>  }
>  
>  static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
> 

You change the format string to print the time as hexadecimal. Is that
intentional? If there is a reason for doing it, explain it in the changelog,
otherwise leave it as "%d".

The cast of SIMPLE_TRANSACTION_LIMIT to u32 looks misplaced as well.

	Arnd

Re: [Y2038] [PATCH 3/5] fs: nfsd: Replace time_t with u32 in nfsd4_grace

[Ksenija Stanojević]

On Tue, Nov 3, 2015 at 4:17 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 27 October 2015 09:10:45 Ksenija Stanojevic wrote:
>> Replace time_t type and get_seconds function which are not y2038 safe on
>> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
>> time and therefore will not cause overflow.
>>
>> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
>
> I think this patch needs to be folded into the patch that changes the
> type for nfsd4_lease, as mentioned there.
>
>>       locks_start_grace(net, &nn->nfsd4_manager);
>>       nfsd4_client_tracking_init(net);
>> -     printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
>> +     printk(KERN_INFO "NFSD: starting %x-second grace period (net %p)\n",
>>              nn->nfsd4_grace, net);
>>       queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ);
>
>>
>> -     return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);
>> +     return scnprintf(buf, (u32)SIMPLE_TRANSACTION_LIMIT, "%x\n", *time);
>>  }
>>
>>  static ssize_t nfsd4_write_time(struct file *file, char *buf, size_t size,
>>
>
> You change the format string to print the time as hexadecimal. Is that
> intentional? If there is a reason for doing it, explain it in the changelog,
> otherwise leave it as "%d".
>
> The cast of SIMPLE_TRANSACTION_LIMIT to u32 looks misplaced as well.

I got following warnings with original code and I was trying to fix them:
include/linux/fs.h:2908:61: warning: format ‘%ld’ expects argument of
type ‘long int’, but argument 4 has type ‘u32’ [-Wformat=]
 #define SIMPLE_TRANSACTION_LIMIT (PAGE_SIZE - sizeof(struct
simple_transaction_argresp))
                                                             ^
fs/nfsd/nfsctl.c:963:24: note: in expansion of macro ‘SIMPLE_TRANSACTION_LIMIT’
  return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);

but I see now that changing %ld to %d works as well.

Thanks,
Ksenija

Re: [Y2038] [PATCH 3/5] fs: nfsd: Replace time_t with u32 in nfsd4_grace

[Arnd Bergmann]

On Tuesday 03 November 2015 18:27:53 Ksenija Stanojević wrote:
> >
> > You change the format string to print the time as hexadecimal. Is that
> > intentional? If there is a reason for doing it, explain it in the changelog,
> > otherwise leave it as "%d".
> >
> > The cast of SIMPLE_TRANSACTION_LIMIT to u32 looks misplaced as well.
> 
> I got following warnings with original code and I was trying to fix them:
> include/linux/fs.h:2908:61: warning: format ‘%ld’ expects argument of
> type ‘long int’, but argument 4 has type ‘u32’ [-Wformat=]
>  #define SIMPLE_TRANSACTION_LIMIT (PAGE_SIZE - sizeof(struct
> simple_transaction_argresp))
>                                                              ^
> fs/nfsd/nfsctl.c:963:24: note: in expansion of macro ‘SIMPLE_TRANSACTION_LIMIT’
>   return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);
> 
> but I see now that changing %ld to %d works as well.
> 

Ok. I have no idea why gcc decided to mention "in expansion of macro
‘SIMPLE_TRANSACTION_LIMIT’" here, which is obviously confusing as it
has nothing to do with the actual change.

	Arnd

[PATCH 4/5] fs: nfsd: Replace time_t with u32 dl_time

[Ksenija Stanojevic]

Replace time_t type and get_seconds function which are not y2038 safe on
32-bit systems. Function ktime_get_seconds use monotonic instead of real
time and therefore will not cause overflow.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 fs/nfsd/nfs4state.c | 2 +-
 fs/nfsd/state.h     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4b3c87d..1652531 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3499,7 +3499,7 @@ static void nfsd4_cb_recall_prepare(struct nfsd4_callback *cb)
 	 */
 	spin_lock(&state_lock);
 	if (dp->dl_time == 0) {
-		dp->dl_time = get_seconds();
+		dp->dl_time = ktime_get_seconds();
 		list_add_tail(&dp->dl_recall_lru, &nn->del_recall_lru);
 	}
 	spin_unlock(&state_lock);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 5c49ed65..c9ecfc6 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -129,7 +129,7 @@ struct nfs4_delegation {
 	struct list_head	dl_recall_lru;  /* delegation recalled */
 	struct nfs4_clnt_odstate *dl_clnt_odstate;
 	u32			dl_type;
-	time_t			dl_time;
+	u32			dl_time;
 /* For recall: */
 	int			dl_retries;
 	struct nfsd4_callback	dl_recall;
-- 
1.9.1

Re: [Y2038] [PATCH 4/5] fs: nfsd: Replace time_t with u32 dl_time

[Arnd Bergmann]

On Tuesday 27 October 2015 09:11:45 Ksenija Stanojevic wrote:
> Replace time_t type and get_seconds function which are not y2038 safe on
> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
> time and therefore will not cause overflow.
> 
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>

dl_time is compared to the local 'cutoff' in nfs4_laundromat(), so you have
to do the change from real to monotonic time in both places as well (same
as cl_time I mentioned earlier).

	Arnd

[PATCH 5/5] fs: nfsd: Replace time_t with u32 oo_time

[Ksenija Stanojevic]

Replace time_t type and get_seconds function which are not y2038 safe on
32-bit systems. Function ktime_get_seconds use monotonic instead of real
time and therefore will not cause overflow.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 fs/nfsd/nfs4state.c | 2 +-
 fs/nfsd/state.h     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1652531..fa50bb8 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3404,7 +3404,7 @@ move_to_close_lru(struct nfs4_ol_stateid *s, struct net *net)
 	last = oo->oo_last_closed_stid;
 	oo->oo_last_closed_stid = s;
 	list_move_tail(&oo->oo_close_lru, &nn->close_lru);
-	oo->oo_time = get_seconds();
+	oo->oo_time = ktime_get_seconds();
 	spin_unlock(&nn->client_lock);
 	if (last)
 		nfs4_put_stid(&last->st_stid);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index c9ecfc6..20d597e6 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -440,7 +440,7 @@ struct nfs4_openowner {
 	 */
 	struct list_head	oo_close_lru;
 	struct nfs4_ol_stateid *oo_last_closed_stid;
-	time_t			oo_time; /* time of placement on so_close_lru */
+	u32			oo_time; /* time of placement on so_close_lru */
 #define NFS4_OO_CONFIRMED   1
 	unsigned char		oo_flags;
 };
-- 
1.9.1

Re: [Y2038] [PATCH 5/5] fs: nfsd: Replace time_t with u32 oo_time

[Arnd Bergmann]

On Tuesday 27 October 2015 09:12:18 Ksenija Stanojevic wrote:
> Replace time_t type and get_seconds function which are not y2038 safe on
> 32-bit systems. Function ktime_get_seconds use monotonic instead of real
> time and therefore will not cause overflow.
> 
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> 

Same comment as for dl_time and cl_time.

	Arnd