On Tue, Mar 7, 2023 at 4:27 PM NeilBrown <neilb@xxxxxxx> wrote:
>
>
> (I've removed some bouncing email addresses, and also removed
> Trond and Anna who are responsible for the NFS client and so
> not directly interested in the server. They will likely find
> this on the list if they are interested).
>
> On Wed, 08 Mar 2023, Jerry Zhang wrote:
> > On Tue, Mar 7, 2023 at 3:20 PM NeilBrown <neilb@xxxxxxx> wrote:
> > >
> > > On Wed, 08 Mar 2023, Jerry Zhang wrote:
> > > > On Tue, Mar 7, 2023 at 2:31 PM NeilBrown <neilb@xxxxxxx> wrote:
> > > > >
> > > > > On Wed, 08 Mar 2023, Jerry Zhang wrote:
> > > > > > The expiry time field is mean to be expressed in seconds since boot.
> > > > >
> > > > > Correct.
> > > > >
> > > > > > The get_expiry() function parses a relative time value in seconds.
> > > > >
> > > > > Incorrect. It parses and absoulte wall-clock time.
> > > > I'm not familiar with the source of truth for this info. Is there a
> > > > specification of some sort?
> > > >
> > > > For reference, we were seeing writes to
> > > > /proc/net/rpc/nfsd.export/channel randomly fail with EINVAL despite
> > > > usually succeeding with the same invocation. Upon investigation this
> > > > was the string that exportfs was writing "-test-client- /path/to/mount
> > > > 3 0 65534 65534 0". "3" was the value for expiry in this message,
> > > > which led me to conclude that this is a relative field. If it isn't,
> > > > perhaps this is a bug in userspace nfs tools?
> > >
> > > The above information is very useful. This sort of detail should always
> > > be included with a bug report, or a patch proposing to fix a bug.
> > >
> > > The intent of that "3" is to be a time in the past. We don't want the
> > > -test-client- entry to be added to the cache, but we want a failure
> > > message if the path cannot be exported. So we set a time in the past as
> > > the expiry time.
> > > Using 0 is awkward as it often has special meaning, so I chose '3'.
> > >
> > > >
> > > > The failure in this was if nfs-server starts exactly 3s after bootup,
> > > > boot.tv_sec would be 3 and thus get_expiry() returns 0, causing a
> > > > failure to be returned.
> > >
> > > I don't understand this. getboottime64() doesn't report time since boot.
> > > It reports the time when the system booted. It only changes when the
> > > system time is deliberately changed.
> > Ok I misinterpreted what this function does.
> > > At boot, it presumably reports 0. As soon as some tool (e.g. systemd or
> > > ntpdate) determines what the current time it and calls settimeofday() or
> > > a similar function, the system time is changed, and the boot-time is
> > > changed by the same amount. Typically this will make it well over 1
> > > billion (for anything booted this century).
> > > So for the boot time to report as '3', something would need to set the
> > > current time to a moment early in January 1970. I'd be surprised if
> > > anything is doing that.
> > I see the discrepency now -- our system is actually an embedded
> > platform without an RTC. So it thinks that it is "1970" every time it
> > boots up, at least until it connects to the internet or similar, which
> > it may or may not ever do. We use NFS to share mountpoints between 2
> > linux systems on our board connected via usb-ethernet. The fact that
> > it allows simultaneous access gives it an advantage over other
> > protocols like mass storage.
> >
> > Its likely that the code is working as intended then, it just didn't
> > take our particular usecase into account.
> >
> > >
> > > How much tracing have you done? Have you printed out the value of
> > > boot.tv_sec and confirmed that it is '3' or have you only deduced it
> > > from other evidence.
> > > Exactly what firm evidence do you have?
> > Sure I've added this simple debug print with the necessary info
> >
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index 15422c951fd1..5af49198b162 100644
> > --- a/fs/nfsd/export.c
> > +++ b/fs/nfsd/export.c
> > @@ -528,10 +528,12 @@ static int svc_export_parse(struct cache_detail
> > *cd, char *mesg, int mlen)
> > int len;
> > int err;
> > struct auth_domain *dom = NULL;
> > struct svc_export exp = {}, *expp;
> > int an_int;
> > + struct timespec64 boot;
> > + char* orig_mesg = mesg;
> >
> > if (mesg[mlen-1] != '\n')
> > return -EINVAL;
> > mesg[mlen-1] = 0;
> >
> > @@ -564,10 +566,12 @@ static int svc_export_parse(struct cache_detail
> > *cd, char *mesg, int mlen)
> > exp.ex_devid_map = NULL;
> >
> > /* expiry */
> > err = -EINVAL;
> > exp.h.expiry_time = get_expiry(&mesg);
> > + getboottime64(&boot);
> > + printk("mesg is '%s' expiry is %lld and boot_s is %lld\n",
> > orig_mesg, exp.h.expiry_time, boot.tv_sec);
> > if (exp.h.expiry_time == 0)
> > goto out3;
> >
> > /* flags */
> > err = get_int(&mesg, &an_int);
> >
> > and the output is
> >
> > [ 14.093506] mesg is '-test-client- /path/to/mount 3 8192 65534
> > 65534 0' expiry is 0 and boot_s is 3
> >
> > which largely confirms the info above.
> >
> > Do you think we'd be able to handle this case cleanly?
>
> Thanks for all the details. Yes I think we can and should handle this
> case cleanly. I think the core problem is that get_expiry() mixes the
> error code into the expiry time. If we separate those out the problem
> should disappear.
>
> Please try this patch.
That worked, thanks for the quick fix!
>
> From: NeilBrown <neilb@xxxxxxx>
> Date: Wed, 8 Mar 2023 11:19:01 +1100
> Subject: [PATCH] SUNRPC: return proper error from get_expiry()
>
> The get_expiry() function currently returns a timestamp, and uses the
> special return value of 0 to indicate an error.
>
> Unfortunately this causes a problem when 0 is the correct return value.
>
> On a system with no RTC it is possible that the boot time will be seen
> to be "3". When exportfs probes to see if a particular filesystem
> supports NFS export it tries to cache information with an expiry time of
> "3". The intention is for this to be "long in the past". Even with not
> RTC it will not be far in the future (at most a second or two) so this
> is harmless.
> But if the boot time happens to have been calculated to be "3", then
> get_expiry will fail incorrectly as it converts the number to "seconds
> since bootime".
>
> To avoid this problem we change get_expiry() to report the error quite
> separately from the expiry time. The error is now the return value.
> The expiry time is reported through a by-reference parameter.
>
> Reported-by: Jerry Zhang <jerry@xxxxxxxxxx>
Tested-by: Jerry Zhang <jerry@xxxxxxxxxx>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/nfsd/export.c | 13 ++++++-------
> fs/nfsd/nfs4idmap.c | 8 ++++----
> include/linux/sunrpc/cache.h | 15 ++++++++-------
> net/sunrpc/auth_gss/svcauth_gss.c | 12 ++++++------
> net/sunrpc/svcauth_unix.c | 12 ++++++------
> 5 files changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 668c7527b17e..6da74aebe1fb 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -123,11 +123,11 @@ static int expkey_parse(struct cache_detail *cd, char *mesg, int mlen)
>
> /* OK, we seem to have a valid key */
> key.h.flags = 0;
> - key.h.expiry_time = get_expiry(&mesg);
> - if (key.h.expiry_time == 0)
> + err = get_expiry(&mesg, &key.h.expiry_time);
> + if (err)
> goto out;
>
> - key.ek_client = dom;
> + key.ek_client = dom;
> key.ek_fsidtype = fsidtype;
> memcpy(key.ek_fsid, buf, len);
>
> @@ -610,9 +610,8 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
> exp.ex_devid_map = NULL;
>
> /* expiry */
> - err = -EINVAL;
> - exp.h.expiry_time = get_expiry(&mesg);
> - if (exp.h.expiry_time == 0)
> + err = get_expiry(&mesg, &exp.h.expiry_time);
> + if (err)
> goto out3;
>
> /* flags */
> @@ -624,7 +623,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
> if (err || an_int < 0)
> goto out3;
> exp.ex_flags= an_int;
> -
> +
> /* anon uid */
> err = get_int(&mesg, &an_int);
> if (err)
> diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
> index 5e9809aff37e..7a806ac13e31 100644
> --- a/fs/nfsd/nfs4idmap.c
> +++ b/fs/nfsd/nfs4idmap.c
> @@ -240,8 +240,8 @@ idtoname_parse(struct cache_detail *cd, char *buf, int buflen)
> goto out;
>
> /* expiry */
> - ent.h.expiry_time = get_expiry(&buf);
> - if (ent.h.expiry_time == 0)
> + error = get_expiry(&buf, &ent.h.expiry_time);
> + if (error)
> goto out;
>
> error = -ENOMEM;
> @@ -408,8 +408,8 @@ nametoid_parse(struct cache_detail *cd, char *buf, int buflen)
> memcpy(ent.name, buf1, sizeof(ent.name));
>
> /* expiry */
> - ent.h.expiry_time = get_expiry(&buf);
> - if (ent.h.expiry_time == 0)
> + error = get_expiry(&buf, &ent.h.expiry_time);
> + if (error)
> goto out;
>
> /* ID */
> diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
> index ec5a555df96f..518bd28f5ab8 100644
> --- a/include/linux/sunrpc/cache.h
> +++ b/include/linux/sunrpc/cache.h
> @@ -300,17 +300,18 @@ static inline int get_time(char **bpp, time64_t *time)
> return 0;
> }
>
> -static inline time64_t get_expiry(char **bpp)
> +static inline int get_expiry(char **bpp, time64_t *rvp)
> {
> - time64_t rv;
> + int error;
> struct timespec64 boot;
>
> - if (get_time(bpp, &rv))
> - return 0;
> - if (rv < 0)
> - return 0;
> + error = get_time(bpp, rvp);
> + if (error)
> + return error;
> +
> getboottime64(&boot);
> - return rv - boot.tv_sec;
> + (*rvp) -= boot.tv_sec;
> + return 0;
> }
>
> #endif /* _LINUX_SUNRPC_CACHE_H_ */
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index acb822b23af1..bfaf584d296a 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -258,11 +258,11 @@ static int rsi_parse(struct cache_detail *cd,
>
> rsii.h.flags = 0;
> /* expiry */
> - expiry = get_expiry(&mesg);
> - status = -EINVAL;
> - if (expiry == 0)
> + status = get_expiry(&mesg, &expiry);
> + if (status)
> goto out;
>
> + status = -EINVAL;
> /* major/minor */
> len = qword_get(&mesg, buf, mlen);
> if (len <= 0)
> @@ -484,11 +484,11 @@ static int rsc_parse(struct cache_detail *cd,
>
> rsci.h.flags = 0;
> /* expiry */
> - expiry = get_expiry(&mesg);
> - status = -EINVAL;
> - if (expiry == 0)
> + status = get_expiry(&mesg, &expiry);
> + if (status)
> goto out;
>
> + status = -EINVAL;
> rscp = rsc_lookup(cd, &rsci);
> if (!rscp)
> goto out;
> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index b1efc34db6ed..9e7798a69051 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -225,9 +225,9 @@ static int ip_map_parse(struct cache_detail *cd,
> return -EINVAL;
> }
>
> - expiry = get_expiry(&mesg);
> - if (expiry ==0)
> - return -EINVAL;
> + err = get_expiry(&mesg, &expiry);
> + if (err)
> + return err;
>
> /* domainname, or empty for NEGATIVE */
> len = qword_get(&mesg, buf, mlen);
> @@ -497,9 +497,9 @@ static int unix_gid_parse(struct cache_detail *cd,
> uid = make_kuid(current_user_ns(), id);
> ug.uid = uid;
>
> - expiry = get_expiry(&mesg);
> - if (expiry == 0)
> - return -EINVAL;
> + err = get_expiry(&mesg, &expiry);
> + if (err)
> + return err;
>
> rv = get_int(&mesg, &gids);
> if (rv || gids < 0 || gids > 8192)
> --
> 2.39.2
>
