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:     Trond Myklebust <trondmy@hammerspace.com>
CC:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "jlayton@kernel.org" <jlayton@kernel.org>
Subject: Re: [PATCH RFC] NFSD: Instantiate a filecache entry when creating a
 file
Thread-Topic: [PATCH RFC] NFSD: Instantiate a filecache entry when creating a
 file
Thread-Index: AQHYP8uq2f68inC7SkKq9UfIdDsXqKzPI6sAgAAdL4A=
Date:   Fri, 25 Mar 2022 00:35:28 +0000
Message-ID: <E31250AB-636B-4AE6-8A31-69A3C26223EF@oracle.com>
References: <164815968129.1809.12154191330176123202.stgit@manet.1015granger.net>
 <83b0b0292bda06ffa56487ea1a019ea142107fa3.camel@hammerspace.com>
In-Reply-To: <83b0b0292bda06ffa56487ea1a019ea142107fa3.camel@hammerspace.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SXtlOF6rj+H1a3aeInE6XPE9dYN38y8X7Tu/biK3Hx0KLzDDFGGFDK4z/f1x?=
 =?us-ascii?Q?zstxNlPoM6X6HP3wO+hRfR18edQpQj9fS44WAocmr6KpZv8BAlpnuyExnALq?=
 =?us-ascii?Q?HOz+9arGCH7kjecUBgN7Z2lYzOWQF+UpHaIkj5LcAuXtI936MfGl53eFGcZi?=
 =?us-ascii?Q?bqq52JrbzTcACkxgonBudxNlSoHUlLF2mD52B/rqAF5ZNaxCh7Ssjr6h+eBW?=
 =?us-ascii?Q?gaIuQ2C7vTrLPZ6JQwN8V2Cx3Z0Lkd4LcnjQDuI6Vs+z1mvNMJ/dbArGkLRT?=
 =?us-ascii?Q?G7zerXVi94DqAwjJpPlTMWPtJ4TlbgidVxzuAkZrltZ14mn/BH3oWmb+gso8?=
 =?us-ascii?Q?wZFVhsrwSmdB5B92z3dDcBbUNcXLZwIO9KJzLad8c5ZSr1X1bQwdDvdfNUM3?=
 =?us-ascii?Q?my/cR3Z7h6pP69Tu2Rn4WkypXgNVOlDlDvFfN2t7R37UEE7MGJIIpwZxSYKG?=
 =?us-ascii?Q?vMLvWUQdLuwkfd/sZzJRqpLNdzZlcJljHzEctLkdeUOMeC/uuNBTwUZsQ9KU?=
 =?us-ascii?Q?xkCnd0PpICxs4zUdp3R2ZaMJD8hiZby9yDQUePEedA1yyUZs3u0kIxcwHx6C?=
 =?us-ascii?Q?G0coIok6SvTbKtDmaQcuMfpbbmAn8GC2+NP7QJ8fUwnmy+CUjmDkwin/Gyw4?=
 =?us-ascii?Q?RMQMCub+YxRAxp3Az+koz7xH1GtDVYjZ4vOJr9OXs3Z5q9SvxsY4KTigY3rR?=
 =?us-ascii?Q?fYTxwPKZMtCV/foUH8600hxqbQ557QwbowFE+qdtnXWifh95uoEt7zJDrPIw?=
 =?us-ascii?Q?LBbbMhv/F5e0SHCLLYVCJuflarRWdhb1t6bBnbLhL9InYFRzqtzsF9pk96Rj?=
 =?us-ascii?Q?p3+Dn85YCBDOvGMmdstN0Xh2AZ/U8dHYWU3VyPjn5GywpHAKfgfWkj44xT2S?=
 =?us-ascii?Q?+pKkVOM7fzNfYUaxPgPi2kK4lo51XBI21wB62XwUUm5wphzRgUbL3WpndBnn?=
 =?us-ascii?Q?/ODXwkLVMb0GiO2RLEWY03mA+NsPYD5W5VXI04cqEY05tfVa/WmX1xqbrTf0?=
 =?us-ascii?Q?Bqldry2iUvEWQmErKIOmCcTje8TPDtNNxi5K0jckmSQovGcr8V8ZiebuutHa?=
 =?us-ascii?Q?aLdJLZ9dW6OuqMCLP0U47cr13f3VZ+fT5xchO7Vh9mebRjQrvGkllG8ggX0P?=
 =?us-ascii?Q?PO+aaI9A5A6luUawjg1/GOVLM1jpzYsINppZ2O4WDxBFM3j/QszSuS5ktnPn?=
 =?us-ascii?Q?GajJojWZohsoa5iIi1HsTit36w5laVVpuq03bEUOwrV06xl7y/51yFcK1+GG?=
 =?us-ascii?Q?ivw2E5c0Y9dxAiLihcFGzohvJSB28qZ1fvkNNTZQqD5y/yAl/LWOzZDc+c+F?=
 =?us-ascii?Q?emR+KhA9+hP3cMuwatEPRHxhr18MzQhiPLXNof7+BEqCnLXezNJH6UbYdWrc?=
 =?us-ascii?Q?v/ERPqP3JG3MbzRUcnhTrxbg7y0Z70Gj0VavPjKUoaWoYArK4J3ovHufaIB/?=
 =?us-ascii?Q?sUzuv965xmC0ILMU9+Cti1H7AtGgt9bWEN45rhTDBBpDy+c5Wg2bqVJC1HPX?=
 =?us-ascii?Q?rMvE6KbZeLjJTYbVY882/65H33M5lq9R3c7jhCpCVEr12dk2Q0g5d++3iygS?=
 =?us-ascii?Q?CNCCFwyz0UaQcnYl4weBkt8fn3waz9EhswfYianuSvQds/Ge7PTGQI20MK74?=
 =?us-ascii?Q?++DtTgQweohVCZAs6pQ8qINZMeSP6McBbVXMlrKhR4WuNb7tksHO8DOR7+pt?=
 =?us-ascii?Q?Rzu8Y02VdW3QHmTko7FvvCKQCtBQ97fGLOK3y+MwxLYanEzOdz6eGllUsFz1?=
 =?us-ascii?Q?0sAwftClWs3eUu60jpIKTN36YkDH+Nk=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <9580544501A9D941A43FD4C4C1634CC5@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: 1200958c-b82c-4d33-de40-08da0df75c36
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Mar 2022 00:35:28.0592
 (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: pte53HVOuj2taoW53bJzRsmO2wAAQ4hunZRsho3vi4+X0TbNKcdIoTYzsJtG0bJME5VPN2ioFts9409fqFZTZQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1437
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10296 signatures=694973
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0 spamscore=0
 mlxlogscore=999 phishscore=0 bulkscore=0 malwarescore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2203250000
X-Proofpoint-GUID: STcTKYe8j2nhVP5ukei-yjE1MX4KktYf
X-Proofpoint-ORIG-GUID: STcTKYe8j2nhVP5ukei-yjE1MX4KktYf
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_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE 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 Mar 24, 2022, at 6:51 PM, Trond Myklebust <trondmy@hammerspace.com> wr=
ote:
>=20
> On Thu, 2022-03-24 at 18:08 -0400, Chuck Lever wrote:
>> There have been reports of races that cause NFSv4 OPEN(CREATE) to
>> return an error even though the requested file was created. NFSv4
>> does not seem to provide a status code for this case.
>>=20
>> There are plenty of things that can go wrong between the
>> vfs_create() call in do_nfsd_create() and nfs4_vfs_get_file(), but
>> one of them is another client looking up and modifying the file's
>> mode bits in that window. If that happens, the creator might lose
>> access to the file before it can be opened.
>>=20
>> Instead of opening the newly created file in nfsd4_process_open2(),
>> open it as soon as the file is created, and leave it sitting in the
>> file cache.
>>=20
>> This patch is not optimal, of course - we should replace the
>> vfs_create() call in do_nfsd_create() with a call that creates the
>> file and returns a "struct file *" that can be planted immediately
>> in nf->nf_file.
>>=20
>> But first, I would like to hear opinions about the approach
>> suggested above.
>>=20
>> BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=3D382
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  fs/nfsd/vfs.c |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>=20
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 02a544645b52..80b568fa12f1 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -1410,6 +1410,7 @@ do_nfsd_create(struct svc_rqst *rqstp, struct
>> svc_fh *fhp,
>>         __be32          err;
>>         int             host_err;
>>         __u32           v_mtime=3D0, v_atime=3D0;
>> +       struct nfsd_file *nf;
>> =20
>>         /* XXX: Shouldn't this be nfserr_inval? */
>>         err =3D nfserr_perm;
>> @@ -1535,6 +1536,14 @@ do_nfsd_create(struct svc_rqst *rqstp, struct
>> svc_fh *fhp,
>>                 iap->ia_atime.tv_nsec =3D 0;
>>         }
>> =20
>> +       /* Attempt to instantiate a filecache entry while we still
>> hold
>> +        * the parent's inode mutex. */
>> +       err =3D nfsd_file_acquire(rqstp, resfhp, NFSD_MAY_WRITE, &nf);
>> +       if (err)
>> +               /* This would be bad */
>> +               goto out;
>> +       nfsd_file_put(nf);
>=20
> Why rely on the file cache to carry the nfsd_file? Isn't it much easier
> just to pass it back directly to the caller?

My thought was that the "struct file *" has to end up in the
filecache anyway, even in the NFSv3 case. If I do this right,
I can avoid the subsequent call to nfsd_open_verified(), which
seems to be the source of the racy chmod behavior I mentioned
above. That might help NFSv3 too, which would need to recreate
the "struct file *" in nfsd_read() or nfsd_write() anyway.


> There are only 2 callers of do_nfsd_create(), so you'd have
> nfsd3_proc_create() that will just call nfsd_file_put() as per above,
> whereas the NFSv4 specific do_open_lookup() could just stash it in the
> struct nfsd4_open so that it can get passed into do_open_permission()
> and eventually nfsd4_process_open2().

Neither nfsd4_process_open2() or do_open_permission() seem
directly interested in the nfsd_file --- it's all under the
covers, in nfs4_get_vfs_file(). But yes, a "struct nfsd4_open"
is passed to and visible in nfs4_get_vfs_file(), as is the
open->op_created boolean.


>> +
>>   set_attr:
>>         err =3D nfsd_create_setattr(rqstp, resfhp, iap);
>> =20
>>=20
>>=20
>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

--
Chuck Lever