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 no
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" - 0.
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 <[email protected]>
Tested-by: Jerry Zhang <[email protected]>
Signed-off-by: NeilBrown <[email protected]>
---
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
On Wed, 2023-03-08 at 17:51 +1100, NeilBrown wrote:
> 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 no
> 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" - 0.
>
> 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 <[email protected]>
> Tested-by: Jerry Zhang <[email protected]>
> Signed-off-by: NeilBrown <[email protected]>
> ---
> 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)
Nice cleanup. The old return value was very confusing.
Reviewed-by: Jeff Layton <[email protected]>
> On Mar 8, 2023, at 1:51 AM, NeilBrown <[email protected]> wrote:
>
>
> 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 no
> 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" - 0.
>
> 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 <[email protected]>
> Tested-by: Jerry Zhang <[email protected]>
> Signed-off-by: NeilBrown <[email protected]>
Thanks, Neil. Applied to nfsd-next.
> ---
> 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
>
--
Chuck Lever