Re: [PATCH] SUNRPC: return proper error from get_expiry()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <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)

Nice cleanup. The old return value was very confusing.

Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux