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(-)
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 */
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;
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,
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;
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; };
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
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
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
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
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
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
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
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
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
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?
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
-
Arnd Bergmann -
Ksenija Stanojevic -
Ksenija Stanojević