From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Neil Brown <neilb@suse.de>
CC:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Jerry Zhang <jerry@skydio.com>
Subject: Re: [PATCH] SUNRPC: return proper error from get_expiry()
Thread-Topic: [PATCH] SUNRPC: return proper error from get_expiry()
Thread-Index: AQHZUYpeEnYYHsI3SUS/OXrZaxDUNq7w5z8A
Date:   Wed, 8 Mar 2023 13:52:04 +0000
Message-ID: <FE3AE30C-924D-4828-9F94-3A223AAEEA92@oracle.com>
References: <167825826081.8008.16276753342643583003@noble.neil.brown.name>
In-Reply-To: <167825826081.8008.16276753342643583003@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?y54mWXinSrxH8dfSwp8hTa68iOIgCyVzuc2IYGHDfhl5e8+6n8fDbz7KJWR9?=
 =?us-ascii?Q?eUQ+DFMjYxULQdM6+UWSSW7MceGZ/HBbE8+JYsJd++/43wXnzNhSOrNcuh3j?=
 =?us-ascii?Q?oU2rpCvqcZil0l7tSGTsg/+4geQX6hIWnJlWdMczL8tKC92JwmgUyt/kqXEq?=
 =?us-ascii?Q?hcTPAprh7xadKY50+ahry6nEe4RFJKuYTr9jEsJVweBTSKBmi1sstI7zl/Ew?=
 =?us-ascii?Q?DgLW53JeELWkzHNJaKgc0drLyIRtRGghT4KmK8T7Im6seGJwwXOfAqWRsdpf?=
 =?us-ascii?Q?p2f9UdjcyJyxf20slbfOdaLf2fRU/otrJdSVQkm5l1I64ClhqF0DTF0fshN9?=
 =?us-ascii?Q?WilaOWG9Kl4ki2u5VDsEMLdOjC1nbUw7IgFOKwAoOgpLYnZm0bs2cpJXCIkr?=
 =?us-ascii?Q?LtApY433cHRmqdSruyMBlsZszbT+rZJEFIM531XIUzA505IxFdib6ymAW2L5?=
 =?us-ascii?Q?yAgIN+2S2CJwjnbmJZ5cKNDX8/SmeTVn3Z1JoYKKTyOy9XX79bk5F2+b8BXy?=
 =?us-ascii?Q?GwDLDHoIOLPOOhFOmb7gDaa75ck39p/rTZgHOy4L78At6cLdCE6HM6ErTR5f?=
 =?us-ascii?Q?9Bn0D2gvzDvwNKFiuN69XMYzxoJfd/jEOok8ZHy//cBwQUurCukxPESc5nYp?=
 =?us-ascii?Q?zAD3EtMQobC/nmsYPOD6Uj5Bse+5hmzxgL+lzutmgQUIUiFgWZw30fRb65dt?=
 =?us-ascii?Q?myHyjPoDF0noAyGkYLmH0tPO4gsp1OOBu8jeel3WZjDOp2r3qwPqF55CUrhD?=
 =?us-ascii?Q?QcUpEvhkHh8T/pIaZXOVVTcH3krBIqZFz9B9XDV1eznEKqqVwE4+deam0eK9?=
 =?us-ascii?Q?O0y2EIlfqvY4lBQIFoKC0I30vntoD4fo0W8eBBLov3lRiicDu3AQS2zwpo+b?=
 =?us-ascii?Q?ddzdbEPJzO9/bN6JAg641xStjSDBSvJD7AkY1x+D4dsY8VoSq4R7IkXxOVlX?=
 =?us-ascii?Q?RNKT3oin3NgqMI31HZIj++4Hi3/dIEaXz0XFqpiPYA8TFZX7ow4NAPtzvq73?=
 =?us-ascii?Q?6grNYOhI/G05nCplaE2u4cSlRUq4ZPOgLAkq6lzpumBg1rPXhiVca3M7vK2S?=
 =?us-ascii?Q?yqoHXbBEwMCfgwSFbH+fsRKgBYk2WQTMyJaPMrYIlqn8VOs7MmieR8sKbHNf?=
 =?us-ascii?Q?ecydeKji3NJGq6NNPX8lc2fHc6zCOU9q8eLllBFFC9DzFpc3AmrQ3Jhwwboa?=
 =?us-ascii?Q?MfCHZiwe/I4CZiOuXmQxm+3uhQ6h56vMd9EnbADOzdVw6KLUnqvQLQZ6+SC9?=
 =?us-ascii?Q?Mw1Ia1jPVu65Gzs/kq2Axq+vMtwwKzVsCPNS9B/+RNh9rcLxqCdv789nE1vO?=
 =?us-ascii?Q?HtItitCHdVIp02EbNqAnSGMcJNN/hG7jtGsnEMJRIxFJfrRJhm5VKt8FboT9?=
 =?us-ascii?Q?TgRqVPSDIWvkQ5IbjcX0YQSBmxXTtl9RA9HgxL0HXV6GLHx2nhhU9OhyA7Lg?=
 =?us-ascii?Q?k1RVTJx08ESli/lPLWbrOkOF2SRaUCOvTacZmM0Vqy9zpPlkrobUU01kxXPw?=
 =?us-ascii?Q?+irKdzvsAbk2sPt4GE2v+QIsSo8SR4Aalqn/kHKFzJkshhgqJlJ4INqBfqxw?=
 =?us-ascii?Q?xzLWjIf2poQmyCLMlj2K3E2ieIgLXCxRF1xKYAATkydf4x29FOl/J8d7KxsH?=
 =?us-ascii?Q?Tw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <386888B6CB211D4586C8E8A397D96F28@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: qq8/b89zOKbEBc8Z5JZJCad3/oMoaYvDVul2CDg5CNc57pf2a63xtutrvzrS/EAD8LSszHKHtmqU6RyIgkaSJJ7Y4S04jpDZOg0/6TUgLZ4KcUoBHF9MIw6HIkKlsnVdAKweXmkgREMQYHMZpuKg2wF1It9MRWQQPj45IRix/Yo+1Tm2gS1totEUZuPDcqcrKW7p4yVoGvpkHCkTsjpC6GchfBDAo6CllesoafLC1T6ydyazTwwU2o/RT9DYYiGs3cJlotEh2NZaPD6qCBLW2jmfCCgjoEtfAXC/kj4YHub/VLnKgFXHUcvsYmrK+KG/T/wTgPQeQBmmlGMvsSr+B6j+nj0ZBXZ7X/bdl9IdJ2oml/L93uwMQw4a83kIn2XjIAjoCXiEVR+9GbRpLBOUQ9UzpizyF8sHBQWismxkEDovUAaZf3cnKIPNjBkbGWWN80Hs8uVyLCsNAof2PMUQuKeKin+gEauwPZkLLh9BhBDO6xZSTRO98HaEIzL4sK56uxByF6wqdCQwV8pzyyakN7elTcmiMykO76fGGREcqL3cd8vH+AHZTCSejVc9WwcjRhKBzvf9yDspn9D+SNgd576edSDGI4HrZcHp/OjDLSwdX4JSiEUSp7HbmbLVvdY/pSw89QTFu6mo07Ghbl9y/gkGOHkbl0OZFb5tWGP+ClIHaLqBXPMoavVrTJI6q6SxBVLgPZZgJIlb3Zy9G8u0G8d1SfrtSytlMzF61lI2ylVQall0VcXjxhgX9YJE3oKL9QiPe/lq+RVIx/F7oIbs2VonWE9TD6GOXDI5hgkBQTrkYVlFO5aRVefVwVuGEfKD25qxSO0LLE/NA/D9WByQvA==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 10cf5ae4-d6d7-4dcf-131d-08db1fdc4cf3
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Mar 2023 13:52:04.6696
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: lmByUuVk+VJ22imBQTwoUoSgEU55JaREuwqjpj1sVSbyDleXv0cYaRvdzF6RRtyVj4KHvyNCDzi5Bci7BuPzXQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4689
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-08_08,2023-03-08_03,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 spamscore=0 bulkscore=0
 mlxlogscore=999 suspectscore=0 malwarescore=0 mlxscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2303080118
X-Proofpoint-GUID: 64HisbJhZj3mjwSVElyg_nATxsg1PGhM
X-Proofpoint-ORIG-GUID: 64HisbJhZj3mjwSVElyg_nATxsg1PGhM
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 8, 2023, at 1:51 AM, NeilBrown <neilb@suse.de> wrote:
>=20
>=20
> The get_expiry() function currently returns a timestamp, and uses the
> special return value of 0 to indicate an error.
>=20
> Unfortunately this causes a problem when 0 is the correct return value.
>=20
> 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.
>=20
> 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.
>=20
> Reported-by: Jerry Zhang <jerry@skydio.com>
> Tested-by: Jerry Zhang <jerry@skydio.com>
> Signed-off-by: NeilBrown <neilb@suse.de>

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(-)
>=20
> 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, ch=
ar *mesg, int mlen)
>=20
> 	/* OK, we seem to have a valid key */
> 	key.h.flags =3D 0;
> -	key.h.expiry_time =3D get_expiry(&mesg);
> -	if (key.h.expiry_time =3D=3D 0)
> +	err =3D get_expiry(&mesg, &key.h.expiry_time);
> +	if (err)
> 		goto out;
>=20
> -	key.ek_client =3D dom;=09
> +	key.ek_client =3D dom;
> 	key.ek_fsidtype =3D fsidtype;
> 	memcpy(key.ek_fsid, buf, len);
>=20
> @@ -610,9 +610,8 @@ static int svc_export_parse(struct cache_detail *cd, =
char *mesg, int mlen)
> 	exp.ex_devid_map =3D NULL;
>=20
> 	/* expiry */
> -	err =3D -EINVAL;
> -	exp.h.expiry_time =3D get_expiry(&mesg);
> -	if (exp.h.expiry_time =3D=3D 0)
> +	err =3D get_expiry(&mesg, &exp.h.expiry_time);
> +	if (err)
> 		goto out3;
>=20
> 	/* 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=3D an_int;
> -=09
> +
> 		/* anon uid */
> 		err =3D 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, in=
t buflen)
> 		goto out;
>=20
> 	/* expiry */
> -	ent.h.expiry_time =3D get_expiry(&buf);
> -	if (ent.h.expiry_time =3D=3D 0)
> +	error =3D get_expiry(&buf, &ent.h.expiry_time);
> +	if (error)
> 		goto out;
>=20
> 	error =3D -ENOMEM;
> @@ -408,8 +408,8 @@ nametoid_parse(struct cache_detail *cd, char *buf, in=
t buflen)
> 	memcpy(ent.name, buf1, sizeof(ent.name));
>=20
> 	/* expiry */
> -	ent.h.expiry_time =3D get_expiry(&buf);
> -	if (ent.h.expiry_time =3D=3D 0)
> +	error =3D get_expiry(&buf, &ent.h.expiry_time);
> +	if (error)
> 		goto out;
>=20
> 	/* 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 *ti=
me)
> 	return 0;
> }
>=20
> -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;
>=20
> -	if (get_time(bpp, &rv))
> -		return 0;
> -	if (rv < 0)
> -		return 0;
> +	error =3D get_time(bpp, rvp);
> +	if (error)
> +		return error;
> +
> 	getboottime64(&boot);
> -	return rv - boot.tv_sec;
> +	(*rvp) -=3D boot.tv_sec;
> +	return 0;
> }
>=20
> #endif /*  _LINUX_SUNRPC_CACHE_H_ */
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svca=
uth_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,
>=20
> 	rsii.h.flags =3D 0;
> 	/* expiry */
> -	expiry =3D get_expiry(&mesg);
> -	status =3D -EINVAL;
> -	if (expiry =3D=3D 0)
> +	status =3D get_expiry(&mesg, &expiry);
> +	if (status)
> 		goto out;
>=20
> +	status =3D -EINVAL;
> 	/* major/minor */
> 	len =3D qword_get(&mesg, buf, mlen);
> 	if (len <=3D 0)
> @@ -484,11 +484,11 @@ static int rsc_parse(struct cache_detail *cd,
>=20
> 	rsci.h.flags =3D 0;
> 	/* expiry */
> -	expiry =3D get_expiry(&mesg);
> -	status =3D -EINVAL;
> -	if (expiry =3D=3D 0)
> +	status =3D get_expiry(&mesg, &expiry);
> +	if (status)
> 		goto out;
>=20
> +	status =3D -EINVAL;
> 	rscp =3D 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;
> 	}
>=20
> -	expiry =3D get_expiry(&mesg);
> -	if (expiry =3D=3D0)
> -		return -EINVAL;
> +	err =3D get_expiry(&mesg, &expiry);
> +	if (err)
> +		return err;
>=20
> 	/* domainname, or empty for NEGATIVE */
> 	len =3D qword_get(&mesg, buf, mlen);
> @@ -497,9 +497,9 @@ static int unix_gid_parse(struct cache_detail *cd,
> 	uid =3D make_kuid(current_user_ns(), id);
> 	ug.uid =3D uid;
>=20
> -	expiry =3D get_expiry(&mesg);
> -	if (expiry =3D=3D 0)
> -		return -EINVAL;
> +	err =3D get_expiry(&mesg, &expiry);
> +	if (err)
> +		return err;
>=20
> 	rv =3D get_int(&mesg, &gids);
> 	if (rv || gids < 0 || gids > 8192)
> --=20
> 2.39.2
>=20

--
Chuck Lever