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 04/13] NFSD: set attributes when creating symlinks
Thread-Topic: [PATCH 04/13] NFSD: set attributes when creating symlinks
Thread-Index: AQHYoLujfmoC23Q2SEegG+eOkJm9Ja2QmDgA
Date:   Tue, 26 Jul 2022 12:40:10 +0000
Message-ID: <A4671B71-ECAD-42BF-8B67-5C098FF36172@oracle.com>
References: <165881740958.21666.5904057696047278505.stgit@noble.brown>
 <165881793056.21666.12904500892707412393.stgit@noble.brown>
In-Reply-To: <165881793056.21666.12904500892707412393.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?K40aXwRgWjXSoInn2MvBVlPIrxbGr6tnoT3IB143IAf130WyAN7YZISDSyuR?=
 =?us-ascii?Q?0k1jLjYD1vEY/ZtVETmWBBfPID8Ui6oasxziz2sjOI8gc37Dpff4obXq+5L0?=
 =?us-ascii?Q?nYhibhVWFOChyFFDAbAkLgwlLPVM72W8o4j7NOQ7yPUgihSfqe3BA0JudIBj?=
 =?us-ascii?Q?5nsFK0vpAVtkkesMkoNu64IfH8ndD5w8urAMdgI/S++ds044+ssN24gfaw4A?=
 =?us-ascii?Q?gKqI+cE8Pg6gyHiU8imsjYPF1zSKm1arRb/W/3FjfOOMatvt9fCYy5Hx79KJ?=
 =?us-ascii?Q?K8OaAyetJbWSSZaQKfuF6mSf/mvVrhoraW/PVrCsEqIfnWmzL/VAl/RrVLzR?=
 =?us-ascii?Q?Xt9BK6djoh6W2v4J8++BQRVDedAbUpj9thwFoE62aEDz/rhp/vMjuqVC3BKF?=
 =?us-ascii?Q?EuPNse2cS2esEV9oUP/ovg/uSTwY3jtpQxWWwzl9opqh5c+XOb0Is75wQwu2?=
 =?us-ascii?Q?PVuz8eVXt/YTVHV2ihoDXJcGgm/TMbS7m9BKMSwLxOUOXjPFakanJX2l98Lq?=
 =?us-ascii?Q?hyh19VeBzp0qIre3iZGHTW6cTxMkyUUJuaRwh0LfLXmajsjeU5fxVIdsQlmj?=
 =?us-ascii?Q?mU3u0QXmSrSmGcHu4vk66u9T+x91pI0b2yYCZh4D9W6UfE/r369cj7hZcKdg?=
 =?us-ascii?Q?kLOn9GZdD7N5D/5dG6qN3ozVfkwlMZQ7csPFYATlDtxnUt1bUIAHwVjssChM?=
 =?us-ascii?Q?AlgQeAMSn81oYx5n37vAbuEIOa6iG7n4QaU2NDNNT1qFiaa0b83NOvDc8nyh?=
 =?us-ascii?Q?scLmPRWgMLoaObkTwPryAakiJed+JjUC7L382WoE7I0qKmuqehyZhnErv24D?=
 =?us-ascii?Q?6MWJMfzyFLrbEZwVhNcKW9pG7tH3HU05Txhj5n1bRAtVnAde8gTE0Cqxgvf7?=
 =?us-ascii?Q?FuVJXV6A7yeUzcmWdjQ40STRGvyBcqSbGZ1WcrztR9KJ+v0If5DfU/y8yDfa?=
 =?us-ascii?Q?Kj+J/+dHqLYM+fUeH9AjJ3Sy/Sck1Q1QuwBmUZyCsA0O2Dbz5jdr7HU1ubZQ?=
 =?us-ascii?Q?dPvUjcvgfBIl8MGy2P2YxaANvxT+5DvKYYSq3Vqfb4lZSACwhqYoY5lvWCQj?=
 =?us-ascii?Q?vCHIVzHT2YItLmp5f31dBSlVLsy9tR+ZyeCmzSp2m2HQwCDvpu6IYf00/nhO?=
 =?us-ascii?Q?7Y7YSJaujWpor3S4H7ktPHM7hlXvPSTrKw26lOZVf0DtMItmby+o8/0DliDc?=
 =?us-ascii?Q?69/GhojQJ/g4vqCK4kY5+BJ20RnS5DxsVaP2P0NXBqvKNNqG+KJfDteAGpdm?=
 =?us-ascii?Q?yUxGwW4ZdMIGiL8ynXC5kU3a8yufl3oaOGdIQ4l/un5ZvMFdlD346ONcKeoK?=
 =?us-ascii?Q?pLbu02KiSNPekD5LwwrDfxAfOIS2l/7cw+3ExTX313cvjXVDSazJP7kqXBJE?=
 =?us-ascii?Q?ZvKP2KWUtIgUc7fxRi9wDWDhRwjD6kb4AIIiiMdjZv6Vxmfj5RDsMBsd0blD?=
 =?us-ascii?Q?BqibpENf1HEQcEKRfa7bP2tXa+VY4ZUwDjx+G3Zd3FcalPLayyo/gnypSHJU?=
 =?us-ascii?Q?08kE+xrFhgFTxg/aDNLGCABouoQjjkXhMmzVlBYVhIGyJiJg8d1F8GAzSXmT?=
 =?us-ascii?Q?ZhtjLIBUHHHYICMO/I/cbFbZn92qFc5baWwxrmvGWqbE2/olDWJRuGLLIaTY?=
 =?us-ascii?Q?Exq4NQf/L4+h2SiPM4G5YK3TvuBrHi1No/dT+Vsjv2W7vd/WlCwqr5lnOf1v?=
 =?us-ascii?Q?B/PJMA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <EFD275BF2A6D7A49AA86028AD1E50330@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: 8f6dfd66-1426-4a8d-c852-08da6f03facc
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jul 2022 12:40:10.9343
 (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: IWXWZxqVVUaPEhXDpgs/+awsrjfpxbxrMfc4H/AJULw1rosZC8KrUTCvyDWtdqx2T2j7U42K0zzvtwrezbayxA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1325
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-26_04,2022-07-26_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0 phishscore=0
 spamscore=0 mlxlogscore=999 suspectscore=0 bulkscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207260048
X-Proofpoint-GUID: vqdlImbWxE8695Gnx6jB7oDM51IL_1jO
X-Proofpoint-ORIG-GUID: vqdlImbWxE8695Gnx6jB7oDM51IL_1jO
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
> The NFS protocol includes attributes when creating symlinks.
> Linux does store attributes for symlinks and allows them to be set,
> though they are not used for permission checking.
>=20
> NFSD currently doesn't set standard (struct iattr) attributes when
> creating symlinks, but for NFSv4 it does set ACLs and security labels.
> This is inconsistent.
>=20
> To improve consistency, pass the provided attributes into nfsd_symlink()
> and call nfsd_create_setattr() to set them.
>=20
> We ignore any error from nfsd_create_setattr().  It isn't really clear
> what should be done if a file is successfully created, but the
> attributes cannot be set.  NFS doesn't allow partial success to be
> reported.  Reporting failure is probably more misleading than reporting
> success, so the status is ignored.
>=20
> Signed-off-by: NeilBrown <neilb@suse.de>

While I was trying to get more information about how
security labels are supposed to be applied to symlinks,
Jeff, Bruce, and I had a brief private discussion about
it. Jeff has the impression that NFSD should not apply
ACLs/security labels to symlinks, and we believe that
would be somewhat of a change in behavior.

Now is a good time to hoist that private discussion to
the list, so I'm mentioning it here. I'm thinking it
would be appropriate to introduce that change in this
series, and probably right here in this patch, if we
agree that is the right thing to do going forward.

Otherwise, after a brief glance at the series, it looks
better to me than the previous approach. I will try to
dig in later this week so we can get this work merged at
the end of the upcoming merge window.


> ---
> fs/nfsd/nfs3proc.c |    3 ++-
> fs/nfsd/nfs4proc.c |    2 +-
> fs/nfsd/nfsproc.c  |    3 ++-
> fs/nfsd/vfs.c      |   11 ++++++-----
> fs/nfsd/vfs.h      |    5 +++--
> 5 files changed, 14 insertions(+), 10 deletions(-)
>=20
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 289eb844d086..5e369096e42f 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> {
> 	struct nfsd3_symlinkargs *argp =3D rqstp->rq_argp;
> 	struct nfsd3_diropres *resp =3D rqstp->rq_resp;
> +	struct nfsd_attrs attrs =3D { .iattr =3D &argp->attrs };
>=20
> 	if (argp->tlen =3D=3D 0) {
> 		resp->status =3D nfserr_inval;
> @@ -417,7 +418,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> 	fh_copy(&resp->dirfh, &argp->ffh);
> 	fh_init(&resp->fh, NFS3_FHSIZE);
> 	resp->status =3D nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
> -				    argp->flen, argp->tname, &resp->fh);
> +				    argp->flen, argp->tname, &attrs, &resp->fh);
> 	kfree(argp->tname);
> out:
> 	return rpc_success;
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index ba750d76f515..ee72c94732f0 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -809,7 +809,7 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_com=
pound_state *cstate,
> 	case NF4LNK:
> 		status =3D nfsd_symlink(rqstp, &cstate->current_fh,
> 				      create->cr_name, create->cr_namelen,
> -				      create->cr_data, &resfh);
> +				      create->cr_data, &attrs, &resfh);
> 		break;
>=20
> 	case NF4BLK:
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 594d6f85c89f..d09d516188d2 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
> {
> 	struct nfsd_symlinkargs *argp =3D rqstp->rq_argp;
> 	struct nfsd_stat *resp =3D rqstp->rq_resp;
> +	struct nfsd_attrs attrs =3D { .iattr =3D &argp->attrs };
> 	struct svc_fh	newfh;
>=20
> 	if (argp->tlen > NFS_MAXPATHLEN) {
> @@ -495,7 +496,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
>=20
> 	fh_init(&newfh, NFS_FHSIZE);
> 	resp->status =3D nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen=
,
> -				    argp->tname, &newfh);
> +				    argp->tname, &attrs, &newfh);
>=20
> 	kfree(argp->tname);
> 	fh_put(&argp->ffh);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index a85dc4dd4f3a..91c9ea09f921 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1451,9 +1451,9 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh=
 *fhp, char *buf, int *lenp)
>  */
> __be32
> nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> -				char *fname, int flen,
> -				char *path,
> -				struct svc_fh *resfhp)
> +	     char *fname, int flen,
> +	     char *path, struct nfsd_attrs *attrs,
> +	     struct svc_fh *resfhp)
> {
> 	struct dentry	*dentry, *dnew;
> 	__be32		err, cerr;
> @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_f=
h *fhp,
>=20
> 	host_err =3D vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> 	err =3D nfserrno(host_err);
> +	cerr =3D fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> +	if (!err)
> +		nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
> 	fh_unlock(fhp);
> 	if (!err)
> 		err =3D nfserrno(commit_metadata(fhp));
> -
> 	fh_drop_write(fhp);
>=20
> -	cerr =3D fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> 	dput(dnew);
> 	if (err=3D=3D0) err =3D cerr;
> out:
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 9bb0e3957982..f3f43ca3ac6b 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -114,8 +114,9 @@ __be32		nfsd_vfs_write(struct svc_rqst *rqstp, struct=
 svc_fh *fhp,
> __be32		nfsd_readlink(struct svc_rqst *, struct svc_fh *,
> 				char *, int *);
> __be32		nfsd_symlink(struct svc_rqst *, struct svc_fh *,
> -				char *name, int len, char *path,
> -				struct svc_fh *res);
> +			     char *name, int len, char *path,
> +			     struct nfsd_attrs *attrs,
> +			     struct svc_fh *res);
> __be32		nfsd_link(struct svc_rqst *, struct svc_fh *,
> 				char *, int, struct svc_fh *);
> ssize_t		nfsd_copy_file_range(struct file *, u64,
>=20
>=20

--
Chuck Lever