Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
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>
Subject: Re: [PATCH 06/13] NFSD: add posix ACLs to struct nfsd_attrs
Thread-Topic: [PATCH 06/13] NFSD: add posix ACLs to struct nfsd_attrs
Thread-Index: AQHYoLurDuvdQIXikEO4o1VXFN1HkK2VbHcA
Date:   Fri, 29 Jul 2022 14:24:53 +0000
Message-ID: <2CE51ADF-AC32-48EF-9D9B-107BC8EC0370@oracle.com>
References: <165881740958.21666.5904057696047278505.stgit@noble.brown>
 <165881793057.21666.7472233362797480106.stgit@noble.brown>
In-Reply-To: <165881793057.21666.7472233362797480106.stgit@noble.brown>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?70n8Z6m/9zin2B3yYhjPMSgaENvvak5mLk/au7/CgY/LmObuAZqiQOT3pVGl?=
 =?us-ascii?Q?xziV+bASAaDFX0EtZEDDJYh+VfgoeGdYkWZIbMPxv5+KbmoXzc94UBjKZ1Gy?=
 =?us-ascii?Q?jys0va88yPH2duRIp/kFLgDF96UvjBUpYgXel25FzrKKELbO0qjqjpYGeSJb?=
 =?us-ascii?Q?OcGVRfJmIKkW3PIaByPfZwnprU5DDuatOBYvce+jt7aqxRC1v43SwJGAQqhp?=
 =?us-ascii?Q?eTUzJmo30l2Q9nIQKlBcpSGSRgJherqTGB60cVM3hsvRTg1YqgcjlGCiS4Co?=
 =?us-ascii?Q?BQYREoyB4DDs1hvfW/VTCwvxqAzqSLDPUr04SOos0TUGeON6YS7mAI8Cw9Wb?=
 =?us-ascii?Q?bmXku5y7OdkxMaYjRMkR2QqWW6RCgYigVIjn0O2Os3F38840FnoG7PEcdkb7?=
 =?us-ascii?Q?Ddpy9JO3r/br4EPXdUtMEvfH5T+8+gnb8Kf7NphPlP0+Pfb2SF8Mfck0/PLW?=
 =?us-ascii?Q?XF9mSJh94mbRgplMFagOwhhVv4GmUl7nCMTL9SZ1vul3V+4n857rb4MKKHmn?=
 =?us-ascii?Q?vW0q0o7BHw893KZjkHXXFfkM+K3v+iZWduplcKt2sVdEP/HgQcVbjWTbfYm7?=
 =?us-ascii?Q?kPOuuqgPLSYSv379ikzVMyFoTbOb0L4tLE7EcYUDGNc33Bx0vJqweFqAkmWH?=
 =?us-ascii?Q?jnhzDgaFiEz4bx/Xd7gVoprVBAEHJ8lY97HJivZ/6AMWbRzwlHfyJQ1VHeWV?=
 =?us-ascii?Q?RpV0MA8yvkUOT+Ey+2I4GGQh3w3g60xEBgXITW/48n3erKH+uIJ/dEfb/mNp?=
 =?us-ascii?Q?X7eoONz5PkZD4FvO41Te0uzlRY7Hg8B3T/pGP4KdHjz+LDWQstmj6HF2+X7H?=
 =?us-ascii?Q?h5L9lWiHPelUGhsYAcpZb3UD0ycdP0ZrfeSRk2YWFYzST4UzYdlY9GqC06DK?=
 =?us-ascii?Q?ky3s4qZCoLLwwaevSiCJIkopb7K9LNcgmrDXU7yXBOE1KenniK2gbDTkus0G?=
 =?us-ascii?Q?V0YfzJ3UeEDEG6xYwDW/Dl5vFeJUtCfPcE4GcQCte5WNrwJDeR+G7qssgzqQ?=
 =?us-ascii?Q?rS59VZ5H2GdPBLtQy1qIXz9cvpilngRGOp/FrSHnhzwovAEnxJGC42gnJTGQ?=
 =?us-ascii?Q?tL93R3B5HD1nYmnWl6yrTStmD4f6gv/SZnDd9X0CZWanjCWJtb4H21bOUggT?=
 =?us-ascii?Q?YaDhALsSpCliihxbxGbFb/0v+i2AGljmQ+3o/xyrfbF2Qt2+f29U3rFQ+E4y?=
 =?us-ascii?Q?RlLAJ9LNCzAnYYtlwSacEDNGG0GFElLatOPWlygu5Dkh+C9rVejLZLrfOahA?=
 =?us-ascii?Q?AhcIepPsuQ9ZkKXShaakHUeK/5sZxP9uD55zFnvhe9kgr+YXDr9IEzdXUO0x?=
 =?us-ascii?Q?7OmaHM5nW5DJ3MKvYpwXMSFZjJdOuJrsiVLSYH4y9YjD0aFanmEi8fDxKngM?=
 =?us-ascii?Q?eTkUjNqDgrjzgrjvqF3Ss6NHCt/Ki1Ciebd/dXPvlRoVQ29j1rotLtRzfjk2?=
 =?us-ascii?Q?stCUEGDOReK+0LOEbIozetOG6nqMyH9b0rdE8c4FGizscRd+Yhi3kLDkV1DN?=
 =?us-ascii?Q?nIIzmQ0rS+BIJPul0m5uw43lKFnOGSiTvZCpNz5ivz+pAPRoAd/Q84p4rJ3q?=
 =?us-ascii?Q?DInuYK+W17ph6zCdERdANk8bwUUs7m6w3PC8ojoOQaBhbdPmZgwui2ZsucT5?=
 =?us-ascii?Q?4A=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3F5A57451F550D4ABA30A327867E73A4@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
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: a02adf1f-5b53-453d-d7a6-08da716e1a7f
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jul 2022 14:24:53.0648
 (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: AYUsb6MyixahjfpudScpoQ+BQRiI17nUcwNOwjZxHo8vtVgGwe/eVFNYcS2yKdugWtuVbrlShvnP9nT0lCw8yg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6202
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-29_16,2022-07-28_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 phishscore=0 bulkscore=0
 adultscore=0 mlxscore=0 mlxlogscore=999 spamscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207290064
X-Proofpoint-GUID: qvUojvKosW086AafLpEXzhPsTQ_3Y7tg
X-Proofpoint-ORIG-GUID: qvUojvKosW086AafLpEXzhPsTQ_3Y7tg
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
>=20
> pacl and dpacl pointers are added to struct nfsd_attrs, which requires
> that we have an nfsd_attrs_free() function to free them.
> Those nfsv4 functions that can set ACLs now set up these pointers
> based on the passed in NFSv4 ACL.
>=20
> nfsd_setattr() sets the acls as appropriate.
>=20
> Errors are handled as with security labels.
>=20
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/acl.h      |    6 ++++--
> fs/nfsd/nfs4acl.c  |   46 +++++++---------------------------------------
> fs/nfsd/nfs4proc.c |   46 ++++++++++++++++------------------------------
> fs/nfsd/vfs.c      |    8 ++++++++
> fs/nfsd/vfs.h      |   10 ++++++++++
> 5 files changed, 45 insertions(+), 71 deletions(-)
>=20
> diff --git a/fs/nfsd/acl.h b/fs/nfsd/acl.h
> index ba14d2f4b64f..4b7324458a94 100644
> --- a/fs/nfsd/acl.h
> +++ b/fs/nfsd/acl.h
> @@ -38,6 +38,8 @@
> struct nfs4_acl;
> struct svc_fh;
> struct svc_rqst;
> +struct nfsd_attrs;
> +enum nfs_ftype4;
>=20
> int nfs4_acl_bytes(int entries);
> int nfs4_acl_get_whotype(char *, u32);
> @@ -45,7 +47,7 @@ __be32 nfs4_acl_write_who(struct xdr_stream *xdr, int w=
ho);
>=20
> int nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry,
> 		struct nfs4_acl **acl);
> -__be32 nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct nfs4_acl *acl);
> +__be32 nfsd4_acl_to_attr(enum nfs_ftype4 type, struct nfs4_acl *acl,
> +			 struct nfsd_attrs *attr);
>=20
> #endif /* LINUX_NFS4_ACL_H */
> diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> index eaa3a0cf38f1..8a5b847b6c73 100644
> --- a/fs/nfsd/nfs4acl.c
> +++ b/fs/nfsd/nfs4acl.c
> @@ -751,58 +751,26 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl =
*acl,
> 	return ret;
> }
>=20
> -__be32
> -nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct nfs4_acl *acl)
> +__be32 nfsd4_acl_to_attr(enum nfs_ftype4 type, struct nfs4_acl *acl,
> +			 struct nfsd_attrs *attr)
> {
> -	__be32 error;
> 	int host_error;
> -	struct dentry *dentry;
> -	struct inode *inode;
> -	struct posix_acl *pacl =3D NULL, *dpacl =3D NULL;
> 	unsigned int flags =3D 0;
>=20
> -	/* Get inode */
> -	error =3D fh_verify(rqstp, fhp, 0, NFSD_MAY_SATTR);
> -	if (error)
> -		return error;
> -
> -	dentry =3D fhp->fh_dentry;
> -	inode =3D d_inode(dentry);
> +	if (!acl)
> +		return nfs_ok;
>=20
> -	if (S_ISDIR(inode->i_mode))
> +	if (type =3D=3D NF4DIR)
> 		flags =3D NFS4_ACL_DIR;
>=20
> -	host_error =3D nfs4_acl_nfsv4_to_posix(acl, &pacl, &dpacl, flags);
> +	host_error =3D nfs4_acl_nfsv4_to_posix(acl, &attr->pacl, &attr->dpacl,
> +					     flags);
> 	if (host_error =3D=3D -EINVAL)
> 		return nfserr_attrnotsupp;
> -	if (host_error < 0)
> -		goto out_nfserr;
> -
> -	fh_lock(fhp);
> -
> -	host_error =3D set_posix_acl(&init_user_ns, inode, ACL_TYPE_ACCESS, pac=
l);
> -	if (host_error < 0)
> -		goto out_drop_lock;
> -
> -	if (S_ISDIR(inode->i_mode)) {
> -		host_error =3D set_posix_acl(&init_user_ns, inode,
> -					   ACL_TYPE_DEFAULT, dpacl);
> -	}
> -
> -out_drop_lock:
> -	fh_unlock(fhp);
> -
> -	posix_acl_release(pacl);
> -	posix_acl_release(dpacl);
> -out_nfserr:
> -	if (host_error =3D=3D -EOPNOTSUPP)
> -		return nfserr_attrnotsupp;
> 	else
> 		return nfserrno(host_error);
> }
>=20
> -
> static short
> ace2type(struct nfs4_ace *ace)
> {
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 83d2b645b0d6..c49cd04cb567 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -128,26 +128,6 @@ is_create_with_attrs(struct nfsd4_open *open)
> 		    || open->op_createmode =3D=3D NFS4_CREATE_EXCLUSIVE4_1);
> }
>=20
> -/*
> - * if error occurs when setting the acl, just clear the acl bit
> - * in the returned attr bitmap.
> - */
> -static void
> -do_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -		struct nfs4_acl *acl, u32 *bmval)
> -{
> -	__be32 status;
> -
> -	status =3D nfsd4_set_nfs4_acl(rqstp, fhp, acl);
> -	if (status)
> -		/*
> -		 * We should probably fail the whole open at this point,
> -		 * but we've already created the file, so it's too late;
> -		 * So this seems the least of evils:
> -		 */
> -		bmval[0] &=3D ~FATTR4_WORD0_ACL;
> -}
> -
> static inline void
> fh_dup2(struct svc_fh *dst, struct svc_fh *src)
> {
> @@ -281,6 +261,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_=
fh *fhp,
> 	if (host_err)
> 		return nfserrno(host_err);
>=20
> +	if (is_create_with_attrs(open))
> +		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);
> +
> 	fh_lock_nested(fhp, I_MUTEX_PARENT);
>=20
> 	child =3D lookup_one_len(open->op_fname, parent, open->op_fnamelen);
> @@ -382,8 +365,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc=
_fh *fhp,
>=20
> 	if (attrs.label_failed)
> 		open->op_bmval[2] &=3D ~FATTR4_WORD2_SECURITY_LABEL;
> +	if (attrs.acl_failed)
> +		open->op_bmval[0] &=3D ~FATTR4_WORD0_ACL;
> out:
> 	fh_unlock(fhp);
> +	nfsd_attrs_free(&attrs);
> 	if (child && !IS_ERR(child))
> 		dput(child);
> 	fh_drop_write(fhp);
> @@ -446,9 +432,6 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_c=
ompound_state *cstate, stru
> 	if (status)
> 		goto out;
>=20
> -	if (is_create_with_attrs(open) && open->op_acl !=3D NULL)
> -		do_set_nfs4_acl(rqstp, *resfh, open->op_acl, open->op_bmval);
> -
> 	nfsd4_set_open_owner_reply_cache(cstate, open, *resfh);
> 	accmode =3D NFSD_MAY_NOP;
> 	if (open->op_created ||
> @@ -779,6 +762,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_com=
pound_state *cstate,
> 	if (status)
> 		return status;
>=20
> +	status =3D nfsd4_acl_to_attr(create->cr_type, create->cr_acl, &attrs);
> 	current->fs->umask =3D create->cr_umask;
> 	switch (create->cr_type) {
> 	case NF4LNK:
> @@ -837,10 +821,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_co=
mpound_state *cstate,
>=20
> 	if (attrs.label_failed)
> 		create->cr_bmval[2] &=3D ~FATTR4_WORD2_SECURITY_LABEL;
> -
> -	if (create->cr_acl !=3D NULL)
> -		do_set_nfs4_acl(rqstp, &resfh, create->cr_acl,
> -				create->cr_bmval);
> +	if (attrs.label_failed)
> +		create->cr_bmval[0] &=3D ~FATTR4_WORD0_ACL;

I think this needs to be "if (attrs.acl_failed)". I've fixed this
in my tree.


> 	fh_unlock(&cstate->current_fh);
> 	set_change_info(&create->cr_cinfo, &cstate->current_fh);
> @@ -849,6 +831,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_com=
pound_state *cstate,
> 	fh_put(&resfh);
> out_umask:
> 	current->fs->umask =3D 0;
> +	nfsd_attrs_free(&attrs);
> 	return status;
> }
>=20
> @@ -1123,6 +1106,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_=
compound_state *cstate,
> 		.iattr =3D &setattr->sa_iattr,
> 		.label =3D &setattr->sa_label,
> 	};
> +	struct inode *inode;
> 	__be32 status =3D nfs_ok;
> 	int err;
>=20
> @@ -1145,9 +1129,10 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4=
_compound_state *cstate,
> 	if (status)
> 		goto out;
>=20
> -	if (setattr->sa_acl !=3D NULL)
> -		status =3D nfsd4_set_nfs4_acl(rqstp, &cstate->current_fh,
> -					    setattr->sa_acl);
> +	inode =3D cstate->current_fh.fh_dentry->d_inode;
> +	status =3D nfsd4_acl_to_attr(S_ISDIR(inode->i_mode) ? NF4DIR : NF4REG,
> +				   setattr->sa_acl, &attrs);
> +
> 	if (status)
> 		goto out;
> 	status =3D nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> @@ -1155,6 +1140,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_=
compound_state *cstate,
> 	if (!status)
> 		status =3D attrs.label_failed;
> out:
> +	nfsd_attrs_free(&attrs);
> 	fh_drop_write(&cstate->current_fh);
> 	return status;
> }
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index e7a18bedf499..4bb05586a258 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -461,6 +461,14 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *=
fhp,
> 	if (attr->label && attr->label->len)
> 		attr->label_failed =3D security_inode_setsecctx(
> 			dentry, attr->label->data, attr->label->len);
> +	if (attr->pacl)
> +		attr->acl_failed =3D set_posix_acl(&init_user_ns,
> +						 inode, ACL_TYPE_ACCESS,
> +						 attr->pacl);
> +	if (!attr->acl_failed && attr->dpacl && S_ISDIR(inode->i_mode))
> +		attr->acl_failed =3D set_posix_acl(&init_user_ns,
> +						 inode, ACL_TYPE_DEFAULT,
> +						 attr->dpacl);
> 	fh_unlock(fhp);
> 	if (size_change)
> 		put_write_access(inode);
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 8464e04af1ea..9343aac0bd15 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -6,6 +6,8 @@
> #ifndef LINUX_NFSD_VFS_H
> #define LINUX_NFSD_VFS_H
>=20
> +#include <linux/fs.h>
> +#include <linux/posix_acl.h>
> #include "nfsfh.h"
> #include "nfsd.h"
>=20
> @@ -45,9 +47,17 @@ typedef int (*nfsd_filldir_t)(void *, const char *, in=
t, loff_t, u64, unsigned);
> struct nfsd_attrs {
> 	struct iattr *iattr;
> 	struct xdr_netobj *label;
> +	struct posix_acl *pacl, *dpacl;
> 	int label_failed;
> +	int acl_failed;
> };
>=20
> +static inline void nfsd_attrs_free(struct nfsd_attrs *attrs)
> +{
> +	posix_acl_release(attrs->pacl);
> +	posix_acl_release(attrs->dpacl);
> +}
> +
> int		nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
> 		                struct svc_export **expp);
> __be32		nfsd_lookup(struct svc_rqst *, struct svc_fh *,
>=20
>=20

--
Chuck Lever