Received-SPF: pass (google.com: domain of linux-kernel+bounces-140504-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 11 Apr 2024 09:27:54 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Vasily Gorbik <gor@linux.ibm.com>
Cc: Jeff Layton <jlayton@kernel.org>, Neil Brown <neilb@suse.de>,
        Olga Kornievskaia <kolga@netapp.com>, Dai Ngo <Dai.Ngo@oracle.com>,
        Tom Talpey <tom@talpey.com>, linux-nfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] NFSD: fix endianness issue in nfsd4_encode_fattr4
Message-ID: <ZhflWqUg88h1Cuse@tissot.1015granger.net>
References: <patch.git-0ff58649313e.your-ad-here.call-01712828617-ext-4049@work.hours>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <patch.git-0ff58649313e.your-ad-here.call-01712828617-ext-4049@work.hours>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?YE3QiDE9J1J2Kq18JCqJzm1wv6Hy99p/eeN/aSuMfwRdM8u8bJxz9I3GdrQX?=
 =?us-ascii?Q?JG/VuJVDl0R2FQW++PD4szbKqb/8YGy8mSrKDc1ytyp8xzT27LINSUlIR5SM?=
 =?us-ascii?Q?4/ISoSqSt9hVzrsV4ZXa+g0XUI5Py1cMTaf8B25x0sxkLcYC5fBCkLItFTsV?=
 =?us-ascii?Q?Si/MT1AliT2p+DvfEAG0v9YA0H2NhPi/0H+nv1X4LyMbdwdb72ifIXngp52c?=
 =?us-ascii?Q?K5i0ma2Pcozma6/aAJmIvgL9onLc6mgYWFSZKpRUxmpswV5d+aEUjYgU5SHX?=
 =?us-ascii?Q?1Fl1G+L9HHjm/lYM9ZLtkATw7CNI2Q0DM0r1ARTMLk7A8UQYcaLjcuKINlNv?=
 =?us-ascii?Q?Qo1l8keMWcedSRC3O1T+D7GRqNC1WSGbg3hm9/Jsi+Sn6Ks1FLAxoLVIrZ9G?=
 =?us-ascii?Q?UnNWxfj34qcc+DoiqJnCUUDnIBuW41lWgPyGf5zADSPNroC6VPhukbsiObhf?=
 =?us-ascii?Q?M/RaWXzGtO+WgmE3pZx0hyqwFXPB5W5xm10+xSssP2BMwOygSsXov/ySoRKk?=
 =?us-ascii?Q?3mAMaVJKT4DptSAGjtSloyYEwRgnVyjEuD4D5lsFkHvCYZIiygkRAVXULzXW?=
 =?us-ascii?Q?xnc33E0DufgMuAE5l3mu01ix9aR1ZQ/TUwzHpdZGCgd1DeTewwc7aPfgStPQ?=
 =?us-ascii?Q?+AP2tWwB3LKUOAB2ihX0l7WwOZX+OOK0tM+lq3pj1/tkQGH8pM3DLAU4LpQ7?=
 =?us-ascii?Q?gxWEMO65ohGLlQ89OPjskYPcL1HKWtkzkhpsoXOxrqVNSOmo/9S6qLNH6ier?=
 =?us-ascii?Q?4NnHpI/mupzoosPQUNjRU/At3UHbXag6l6kZLQdrXiiCnJ0QLpBgoy08W/Hi?=
 =?us-ascii?Q?8pn7bdiFZZ7IQuvW96wAqvHquxplwXGCHVUvfwuZu38MIzcMJNjTQfJrDHFS?=
 =?us-ascii?Q?8r9YqN6bMeDCG4+g3okJ+ZcqLtbvgVTbFapwsOmCHSvoqwHT8hrF1aqGY35n?=
 =?us-ascii?Q?mmUZZzQi7dARWc/j2UTDoVNa6s8XWpt/2m67yzfvK6dgpa2HyiGcW2A5QfsS?=
 =?us-ascii?Q?4dWa6DiwXAxJGisq/3UbmWILb/n1qIEXs3MnvTxFPej/oANmvYFrqWSzT5x0?=
 =?us-ascii?Q?+vo7zSXmiz8Z5gNQeh1hwEmyx93LLKT0O3yr9t8wLIzsTjnFvhtDIfXAWmbd?=
 =?us-ascii?Q?AUWNSgWPWzDU5RwG7WKcTXp75TPsg+LsaSCisj+iTCPyI3qeUNRhYpHbIHZN?=
 =?us-ascii?Q?oZREYpd+yyoKYZXxJE410x45QS+nddtqbFTNAZ+BDyLzsujuoS2xZN0mTu1r?=
 =?us-ascii?Q?mL56qN6Vzv3SUUElMWmQpXhD4/fmUeugJULQ9JHs5+AczG/V2/3yM14Oxn9O?=
 =?us-ascii?Q?GFPvhUcydUyJkLT6nTbgoNcEJ3T+DshMOlcdBLBON9tae2yIvRSK6N0LJE9l?=
 =?us-ascii?Q?P8R6eWt3z/XQEZaj/yjcLt/pTs2AwTAe9MTG7fNH7V9kCVQ8O2Yv5CNggjPT?=
 =?us-ascii?Q?5DfhrPlU6/e5D0jG/QDHb4S1OYs+rVkFEs+3XAyw3DFkIYPD81YqISxsjZac?=
 =?us-ascii?Q?ibm9NET1e/VaMTgtoL6NoNpH40TEQQat14a5wgWjej8CWmZXWt9jCrcLcAXv?=
 =?us-ascii?Q?mddO7m0ZwMSwaDOjbWocSRirT4k/7/MbOoyjqH3LX1TbJW38jVzR1MvNfc39?=
 =?us-ascii?Q?bA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	UN0k8iBBQRICAokclO73AOc2ENeuI9/3HiFQn5VLcLMSkmGHWG3pp9QcZiCXw7bH7WQP3xCSNSwPPmUc6S9qsg8uvdXSBvrJ+8Y99udJeSTxxvP1a8E9C0t417ldWh4gjF5F1gRPCLE49fFDHGAX+Mn9JCh7xJOmUtZfUNgygJb6i1+r7avwi2ShzBkzRgv85JVevo2tpTOkZ0QG2vZBG4mrgBZ4S62q9jv8oiEHztk+l7FjjdhKjfGG7C322uQj3ICC4DJ3ciopyS8A1i7LT6OsmH4oiJCfKOjOFH1w+P9n6toPBxDlKfs3J7fldoLIBmN/XkVjTiq7VAAwKnzVEIgY7sNtzvu8GDK7Pdr3iDCfc52gp5cRfWAWHc/Yypabg/QR6OxaW4rl+Yt8LALQ2woKJ+c7ohselIBkwd7RjqBYbyjILSpIG8JsOAzzedFsCL0GOadAXjbIMbc0Y7KHXNzgJKrkXMNa421eUmlxXHOu2/vifIBsSzontmcUaz5rfnyDytNHaDQrnsaxOKFaOf9/jgvmQdSFWgyVAqJuOUHjI8vGaCR9enmoEoXUoizeX8Q1izV7S1/jvxMq/pTi/NngslhCIfm5Meun8ndT5yU=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 61cf8de9-3c67-4adb-1304-08dc5a2b330a
X-MS-Exchange-CrossTenant-AuthSource: DS7PR10MB5134.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Apr 2024 13:27:56.7776
 (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: iobgurmVywGds2Ug2UKZkEoOFyWrQ7u3VP8bs4K8+mfKHmmCjxcfplgFxSKwn6i3q+XGF61ZJolKgJs+B9+wlA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR10MB7147
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-11_06,2024-04-09_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 malwarescore=0
 suspectscore=0 spamscore=0 mlxscore=0 bulkscore=0 adultscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404110098
X-Proofpoint-ORIG-GUID: lImtKuxz4WNXEKZr3RJQS8mKDqcXazOJ
X-Proofpoint-GUID: lImtKuxz4WNXEKZr3RJQS8mKDqcXazOJ

On Thu, Apr 11, 2024 at 11:45:57AM +0200, Vasily Gorbik wrote:
> The nfs4 mount fails with EIO on 64-bit big endian architectures since
> v6.7. The issue arises from employing a union in the nfsd4_encode_fattr4()
> function to overlay a 32-bit array with a 64-bit values based bitmap,
> which does not function as intended. Address the endianness issue by
> utilizing bitmap_from_arr32() to copy 32-bit attribute masks into a
> bitmap in an endianness-agnostic manner.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: fce7913b13d0 ("NFSD: Use a bitmask loop to encode FATTR4 results")
> Link: https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/2060217
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>

Thank you for the bug fix! Applied to nfsd-fixes (for v6.9-rc).


> ---
>  fs/nfsd/nfs4xdr.c | 47 +++++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 10439d569d9c..85d43b3249f9 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3519,11 +3519,13 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  		    struct dentry *dentry, const u32 *bmval,
>  		    int ignore_crossmnt)
>  {
> +	DECLARE_BITMAP(attr_bitmap, ARRAY_SIZE(nfsd4_enc_fattr4_encode_ops));
>  	struct nfsd4_fattr_args args;
>  	struct svc_fh *tempfh = NULL;
>  	int starting_len = xdr->buf->len;
>  	__be32 *attrlen_p, status;
>  	int attrlen_offset;
> +	u32 attrmask[3];
>  	int err;
>  	struct nfsd4_compoundres *resp = rqstp->rq_resp;
>  	u32 minorversion = resp->cstate.minorversion;
> @@ -3531,10 +3533,6 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  		.mnt	= exp->ex_path.mnt,
>  		.dentry	= dentry,
>  	};
> -	union {
> -		u32		attrmask[3];
> -		unsigned long	mask[2];
> -	} u;
>  	unsigned long bit;
>  	bool file_modified = false;
>  	u64 size = 0;
> @@ -3550,20 +3548,19 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  	/*
>  	 * Make a local copy of the attribute bitmap that can be modified.
>  	 */
> -	memset(&u, 0, sizeof(u));
> -	u.attrmask[0] = bmval[0];
> -	u.attrmask[1] = bmval[1];
> -	u.attrmask[2] = bmval[2];
> +	attrmask[0] = bmval[0];
> +	attrmask[1] = bmval[1];
> +	attrmask[2] = bmval[2];
>  
>  	args.rdattr_err = 0;
>  	if (exp->ex_fslocs.migrated) {
> -		status = fattr_handle_absent_fs(&u.attrmask[0], &u.attrmask[1],
> -						&u.attrmask[2], &args.rdattr_err);
> +		status = fattr_handle_absent_fs(&attrmask[0], &attrmask[1],
> +						&attrmask[2], &args.rdattr_err);
>  		if (status)
>  			goto out;
>  	}
>  	args.size = 0;
> -	if (u.attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
> +	if (attrmask[0] & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
>  		status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry),
>  					&file_modified, &size);
>  		if (status)
> @@ -3582,16 +3579,16 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  
>  	if (!(args.stat.result_mask & STATX_BTIME))
>  		/* underlying FS does not offer btime so we can't share it */
> -		u.attrmask[1] &= ~FATTR4_WORD1_TIME_CREATE;
> -	if ((u.attrmask[0] & (FATTR4_WORD0_FILES_AVAIL | FATTR4_WORD0_FILES_FREE |
> +		attrmask[1] &= ~FATTR4_WORD1_TIME_CREATE;
> +	if ((attrmask[0] & (FATTR4_WORD0_FILES_AVAIL | FATTR4_WORD0_FILES_FREE |
>  			FATTR4_WORD0_FILES_TOTAL | FATTR4_WORD0_MAXNAME)) ||
> -	    (u.attrmask[1] & (FATTR4_WORD1_SPACE_AVAIL | FATTR4_WORD1_SPACE_FREE |
> +	    (attrmask[1] & (FATTR4_WORD1_SPACE_AVAIL | FATTR4_WORD1_SPACE_FREE |
>  		       FATTR4_WORD1_SPACE_TOTAL))) {
>  		err = vfs_statfs(&path, &args.statfs);
>  		if (err)
>  			goto out_nfserr;
>  	}
> -	if ((u.attrmask[0] & (FATTR4_WORD0_FILEHANDLE | FATTR4_WORD0_FSID)) &&
> +	if ((attrmask[0] & (FATTR4_WORD0_FILEHANDLE | FATTR4_WORD0_FSID)) &&
>  	    !fhp) {
>  		tempfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
>  		status = nfserr_jukebox;
> @@ -3606,10 +3603,10 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  		args.fhp = fhp;
>  
>  	args.acl = NULL;
> -	if (u.attrmask[0] & FATTR4_WORD0_ACL) {
> +	if (attrmask[0] & FATTR4_WORD0_ACL) {
>  		err = nfsd4_get_nfs4_acl(rqstp, dentry, &args.acl);
>  		if (err == -EOPNOTSUPP)
> -			u.attrmask[0] &= ~FATTR4_WORD0_ACL;
> +			attrmask[0] &= ~FATTR4_WORD0_ACL;
>  		else if (err == -EINVAL) {
>  			status = nfserr_attrnotsupp;
>  			goto out;
> @@ -3621,17 +3618,17 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  
>  #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>  	args.context = NULL;
> -	if ((u.attrmask[2] & FATTR4_WORD2_SECURITY_LABEL) ||
> -	     u.attrmask[0] & FATTR4_WORD0_SUPPORTED_ATTRS) {
> +	if ((attrmask[2] & FATTR4_WORD2_SECURITY_LABEL) ||
> +	     attrmask[0] & FATTR4_WORD0_SUPPORTED_ATTRS) {
>  		if (exp->ex_flags & NFSEXP_SECURITY_LABEL)
>  			err = security_inode_getsecctx(d_inode(dentry),
>  						&args.context, &args.contextlen);
>  		else
>  			err = -EOPNOTSUPP;
>  		args.contextsupport = (err == 0);
> -		if (u.attrmask[2] & FATTR4_WORD2_SECURITY_LABEL) {
> +		if (attrmask[2] & FATTR4_WORD2_SECURITY_LABEL) {
>  			if (err == -EOPNOTSUPP)
> -				u.attrmask[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> +				attrmask[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
>  			else if (err)
>  				goto out_nfserr;
>  		}
> @@ -3639,8 +3636,8 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
>  
>  	/* attrmask */
> -	status = nfsd4_encode_bitmap4(xdr, u.attrmask[0],
> -				      u.attrmask[1], u.attrmask[2]);
> +	status = nfsd4_encode_bitmap4(xdr, attrmask[0], attrmask[1],
> +				      attrmask[2]);
>  	if (status)
>  		goto out;
>  
> @@ -3649,7 +3646,9 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, struct xdr_stream *xdr,
>  	attrlen_p = xdr_reserve_space(xdr, XDR_UNIT);
>  	if (!attrlen_p)
>  		goto out_resource;
> -	for_each_set_bit(bit, (const unsigned long *)&u.mask,
> +	bitmap_from_arr32(attr_bitmap, attrmask,
> +			  ARRAY_SIZE(nfsd4_enc_fattr4_encode_ops));
> +	for_each_set_bit(bit, attr_bitmap,
>  			 ARRAY_SIZE(nfsd4_enc_fattr4_encode_ops)) {
>  		status = nfsd4_enc_fattr4_encode_ops[bit](xdr, &args);
>  		if (status != nfs_ok)
> -- 
> 2.41.0

-- 
Chuck Lever