Received-SPF: neutral (google.com: 212.227.17.10 is neither permitted nor denied by best guess record for domain of arnd@arndb.de) client-ip=212.227.17.10;
From: Arnd Bergmann <arnd@arndb.de>
To: y2038@lists.linaro.org
Cc: Ksenija =?utf-8?B?U3Rhbm9qZXZpxIc=?= <ksenija.stanojevic@gmail.com>, outreachy-kernel <outreachy-kernel@googlegroups.com>
Subject: Re: [Y2038] [PATCH 2/5] fs: nfsd: Replace time_t with u32 in nfsd4_lease
Date: Tue, 10 Nov 2015 11:01:44 +0100
Message-ID: <3868597.PIV5n69lhm@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <CAL7P5jK=pbSvFURGN55TYUfbhPedOuNyi+Rm=wRKbxxuQdnU5Q@mail.gmail.com>
References: <cover.1445960986.git.ksenija.stanojevic@gmail.com> <8412799.SPuinIILNH@wuerfel> <CAL7P5jK=pbSvFURGN55TYUfbhPedOuNyi+Rm=wRKbxxuQdnU5Q@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"

On Monday 09 November 2015 20:37:22 Ksenija Stanojevi=C4=87 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 s=
afe on
> >> 32-bit systems. Function ktime_get_seconds use monotonic instead o=
f 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.
>=20
> It' safe to assume that system will be rebooted in 136 years and 32-b=
it
> 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 proto=
col,
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 =3D get_seconds() - nn->nfsd4_lease;
> >> -     time_t t, new_timeo =3D nn->nfsd4_lease;
> >> +     u32 cutoff =3D ktime_get_seconds() - nn->nfsd4_lease;
> >> +     u32 t, new_timeo =3D 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.
>=20
> 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_tim=
e
> 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, s=
ize_t size,
> >> -                               time_t *time, struct nfsd_net *nn)=

> >> +                               u32 *time, struct nfsd_net *nn)
> >>  {
> >>       char *mesg =3D 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, siz=
e_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.
>=20
> Ok, so changes to nfsd4_lease and nfsd4_grace should be done in one p=
atch?

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

=09Arnd