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

[Arnd Bergmann <arnd@arndb.de>]

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