fs: nfsd: Remove time_t

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 <[email protected]>

Quoted 5 lines · 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 <[email protected]>

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

fs/nfsd/nfs4state.c STALE_CLIENTID(clientid_t *clid, struct nfsd_net *nn) · line 1616

Quoted 11 lines

fs/nfsd/nfs4state.c +1 -1

index 0f1d569..8245236 100644
@@ -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.

fs/nfsd/state.h struct nfs4_client { · line 311

Quoted 12 lines

fs/nfsd/state.h +1 -1

@@ -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

Quoted 8 lines · Arnd Bergmann

Quoted 5 lines · 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 <[email protected]>

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).

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 <[email protected]>

fs/nfsd/netns.h +1 -1

index d5cce15..85b114f 100644
@@ -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;

fs/nfsd/nfs4callback.c +0 -0

index e7f50c4..8a177c6 100644

fs/nfsd/nfs4callback.c +0 -0

 static struct rpc_cred *callback_cred;

fs/nfsd/nfs4state.c +0 -0

index 8245236..4bceb878 100644

fs/nfsd/nfs4state.c +1 -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;
 }
 

fs/nfsd/nfsctl.c +1 -1

index 9690cb4..6e0cfd6 100644
@@ -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;

Quoted 5 lines · 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 <[email protected]>

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.

Quoted 18 lines

fs/nfsd/netns.h +1 -1

index d5cce15..85b114f 100644
@@ -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;

fs/nfsd/nfs4callback.c +0 -0

index e7f50c4..8a177c6 100644

fs/nfsd/nfs4callback.c static const struct rpc_program cb_program = { · line 685

Quoted 6 lines

fs/nfsd/nfs4callback.c +1 -1

@@ -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.

Quoted 14 lines

fs/nfsd/nfs4callback.c +0 -0

 static struct rpc_cred *callback_cred;

fs/nfsd/nfs4state.c +2 -2

index 8245236..4bceb878 100644
@@ -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;
 

fs/nfsd/nfs4state.c nfs4_laundromat(struct nfsd_net *nn) · line 4339

Quoted 2 lines

fs/nfsd/nfs4state.c +0 -0

      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.

Quoted 25 lines

fs/nfsd/nfs4state.c +1 -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;
 }
 

fs/nfsd/nfsctl.c +1 -1

index 9690cb4..6e0cfd6 100644
@@ -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,
 }
 

fs/nfsd/nfsctl.c static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size, · line 969

Quoted 5 lines

fs/nfsd/nfsctl.c +1 -1

 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.

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · 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.

Quoted 5 lines

Quoted 1 line

Signed-off-by Ksenija Stanojevic <[email protected]>

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.

Quoted 13 lines

Quoted 13 lines

fs/nfsd/nfs4state.c +2 -2

index 8245236..4bceb878 100644
@@ -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;
 

Quoted 5 lines

fs/nfsd/nfs4state.c nfs4_laundromat(struct nfsd_net *nn) · line 4339

Quoted 2 lines

fs/nfsd/nfs4state.c +0 -0

      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?

Quoted 16 lines

Quoted 16 lines

fs/nfsd/nfsctl.c +1 -1

index 9690cb4..6e0cfd6 100644
@@ -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,
 }
 

Quoted 8 lines

fs/nfsd/nfsctl.c static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_t size, · line 969

Quoted 5 lines

fs/nfsd/nfsctl.c +1 -1

 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?

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 <[email protected]>

(quoted diff) +0 -1

 	locks_start_grace(net, &nn->nfsd4_manager);
 	nfsd4_client_tracking_init(net);
-	printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",

Quoted 5 lines · 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 <[email protected]>

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

Quoted 12 lines

      locks_start_grace(net, &nn->nfsd4_manager);
      nfsd4_client_tracking_init(net);

(quoted diff) +2 -2

-     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);
 }

Quoted 2 lines

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.

Quoted 6 lines

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.

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 <[email protected]>

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 <[email protected]>