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:     Jeff Layton <jlayton@kernel.org>
CC:     Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <dai.ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Boyang Xue <bxue@redhat.com>
Subject: Re: [PATCH] nfsd: remove unsafe BUG_ON from set_change_info
Thread-Topic: [PATCH] nfsd: remove unsafe BUG_ON from set_change_info
Thread-Index: AQHZuxrUUcSB+OrawEaFlgaCCuixU6/Cw9WAgAAE6gCAAAE3gA==
Date:   Thu, 20 Jul 2023 15:37:38 +0000
Message-ID: <C4A9048C-C3C8-4C62-B68F-7170C6CDC5BE@oracle.com>
References: <20230720-bz2223560-v1-1-edb4900043b8@kernel.org>
 <4B067A0F-93E3-435A-A32B-B17BC07D4606@oracle.com>
 <061f2b988de3da1dac32ecb3d8ac76319065b51d.camel@kernel.org>
In-Reply-To: <061f2b988de3da1dac32ecb3d8ac76319065b51d.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?7uYbJqZ35BdntHbz3AN4NTUA8kcfrZPnFm8ByMek2A9LboxBPGgTKSpWiEAC?=
 =?us-ascii?Q?evigs4nrYYutMbbBa2D/Aha3NoHYwZGX0IT90GGMWOebHVVshjp9yFlewypt?=
 =?us-ascii?Q?JWDqVoMZRjD/VMDl5JwgCfdWZsOKV147PPp0TnCmMMqJtn4YyyqMirQxFCw3?=
 =?us-ascii?Q?byDW3b0o3lMLWceRBDfCbMFqhRA6VwvJ0+aREUkRKvadFCKXa/7xy6fkiSTr?=
 =?us-ascii?Q?AHZsZAM0FaBXy/XaHUZZRq31n77gIudvNNsOB4mXbfBbN/v+EWtRK7yHTfa5?=
 =?us-ascii?Q?bhi8ywQZJYK5S5tXmUeVpQRKgcsIZZMZWXAEfn2NKcGImdPkeHG+j46Drqli?=
 =?us-ascii?Q?tipVUzG1f0Yl11S1n2M+6EJ640I4H4y415JHLJCoWMoDRSnX5Evv10MKhsrf?=
 =?us-ascii?Q?gM2VJqxlZk84l7mJ4DEXDzcoEh0qVb9YkpGL58PCro5pVxw79pc4Q4gZgSQF?=
 =?us-ascii?Q?xu3t5F4QjukrQjTmCpoZK9hIJZ+I/3cEz5ASaYt1Kdx1WYCNLK2HZUn7QJpY?=
 =?us-ascii?Q?+IXE4V/R7VzJBaThsNB8+K/cmMPnigO4QabVNHYcgLbmmI2I/3Ai1PrWvz33?=
 =?us-ascii?Q?11zbuVpPfnpfxHEMj4MdKMS3c77YuvYZUH/9gfS35wytk4v6ihTOe+Po6wJv?=
 =?us-ascii?Q?64OeeKy6FbneHhKIfv9pcF04M4rpvyQ4oZDU44Xtx8zoq2vUukLmAuP45LJC?=
 =?us-ascii?Q?6BnX1bNr2mVRLqHCMKKUs1jTGRzSl9Ce9+CT/iPyUBd85iEEcBayKhwr53Np?=
 =?us-ascii?Q?+dPzMoFHbdAFEfWDgqhgJP/JhaA3WQdVvJ9iu1OcNxYCNQTHtsMlWDG3JLCf?=
 =?us-ascii?Q?Yrb4BMZ71b/wfeSnkf7ItzBX5g2aKifu3hRQyCdQTk9DvShvOGauiPlrz3Eh?=
 =?us-ascii?Q?MGZIla/a6RTxXtzBOjVjnd/EWsRxWIYE80lLaIn26JeP5Iwv4H73LTQoYjei?=
 =?us-ascii?Q?R6XdcuZA2lrCqp60ypJpi8W/xvQNRsiQARzZCUJBdfma7EyAaSXUmnDLPlHr?=
 =?us-ascii?Q?oihQwS1HFPDL+KwQmwgbkAXS8uWe3xdpOkBewDprPXYr4YzH4+N5X6v+RAb7?=
 =?us-ascii?Q?1dsi7VTzxMKhBtbf+MKZ6z37NIm9zpxgsAsAr0LflC1CcodvzqftItNAv9Gx?=
 =?us-ascii?Q?G7upq/0YjUwyXCPV4ZGpxLiWMnOQa8krM/nDyGmDv45RvqYzDAPXvHWnWkCl?=
 =?us-ascii?Q?l8YwDcQZezn1OA/iJT00GvUPevmxrHw1zrmFSPEDZJiqiCikz+x6aWixfT24?=
 =?us-ascii?Q?G3Jv1tHMZ+ML1Dxo4A9CwHiM0jGhJsEsvNnQPxq6Lrn+BXJlIrvwYz2i/H2q?=
 =?us-ascii?Q?o4X18GtN2WeERV4eaDEoFSGhhao6H8D+HEa5GSuYJK18mU8wtPiWlkPtUYYp?=
 =?us-ascii?Q?YJvJ3+g5KcLh5bo++KJmLYgryLedzlIREl2PyGlngpHKnlKwkYdRXRlLswib?=
 =?us-ascii?Q?IkbG5EkC+i8HZO5GwFgJmHNxaPMS60jQwOaqQDgdsInKxCLUUSl6b4YRKaAn?=
 =?us-ascii?Q?OtqPA2B3CyLZk8ebO1IyjU+T4QFRA5VNc6ZVj7ptKYuIFG7OmS1uL2MYIbdU?=
 =?us-ascii?Q?GdpRouphBaFw5XsFDCS0MB/DH18noc/BA7k59iA4QA/EMqK72xVs7ZbER0n1?=
 =?us-ascii?Q?Vw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <E50F0BF6CECDEB41BC78423F0CC289C6@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: K0DblmKsv4yzc6Nv0U9qI+LGS0/EDAsdVXxZWmsRGgBPIXl8RQ5Bqbsd7C0BLwVZtfCuzbTbzbmg3L10KX5U40J0uwPrINkqFfYshvRAAzd6LnyQ+PVPdst8lGKLChVf176vQUODwT+pMJm4yx6AG+V/4JWBi+Fl6/PnmNLjmOO7nFP/DtpPnk6KmbBvXQ11wwK+ynMF/xNxyHjN/k/erdFUY5O5fag9lgQvn+l82B7KaLfley4rLGgzmtCeulXV6vT1k52b1PhRQ77yHy92THJqJ8Yi7uF9tvYifPj45A4YAf/D9hpYURFW1tUXMTS45/tQZGECT861TKG9StKGHnkdc8ckZktfNj4+YEGnd3twUQvWJ8QE70giGZtGtKhs1kVaUYFqRk8X0R+aP0GpSIoYJKc7nB3AhlT7TWDOnvhHZqVtWLXcuUX1KE/QWjXABHQ69m3oupv4IXwJ7QZ2Nj5Qag4rkqgp4rAa5qmUNWLFCdXKKmQBHoad5bKCJbosMUTzVCDRpHcZYvUfHI0IR9Rqw80OfZM49J46/EZs4yRMBCrWRkG0tAAX/+ACNwjn7ksCDpD9NgxXvhmDfCLlozReeJV6AL8NJ2gf4ArLblMOLdR26nYFwSorVeG2vsG+wqGwD3IlmBD0pOaqsZM1mxWllzrHJLmSQEdineUHi3pEg6+QIrcoZ8JVrKdWBX5+m5zU/FT7cENwY+GmI/ajzV2g5sSy/IULxNB07PsLwXzcycU6NFrBGt7zHWIEnXqvguR1De2MFlLXewkbuZbcMq//uAsBY2LX0sZZseTzxDDft+NJEm90IvoqrwpUa2Q5mN7sojarPaK73+ZxQ78jbA8njua7W78CQuBeZIlfJlM3IzJQ54Yn17zM+v2BmZe8D+DPX+kBgH+Fc9BdsB63KQ==
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: 880198eb-a7b7-4f3d-316f-08db89373f6a
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jul 2023 15:37:38.3089
 (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: sdVNJ98EQqhbMv7PNZ7VQLhZofk1Rqurg9NXDnu6DCO4Dlu5C0/GQzG3XjbNCARJ+xz7/UbvKL7hGNab1bPVoA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN6PR10MB7442
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-20_08,2023-07-20_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 mlxlogscore=999
 suspectscore=0 phishscore=0 mlxscore=0 adultscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2306200000
 definitions=main-2307200131
X-Proofpoint-GUID: kKG0G8TaEJXqKrov5RwYFTuBdVbGPTtA
X-Proofpoint-ORIG-GUID: kKG0G8TaEJXqKrov5RwYFTuBdVbGPTtA
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 Jul 20, 2023, at 11:33 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Thu, 2023-07-20 at 15:15 +0000, Chuck Lever III wrote:
>>=20
>>> On Jul 20, 2023, at 10:59 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>>=20
>>> At one time, nfsd would scrape inode information directly out of struct
>>> inode in order to populate the change_info4. At that time, the BUG_ON i=
n
>>> set_change_info made some sense, since having it unset meant a coding
>>> error.
>>>=20
>>> More recently, it calls vfs_getattr to get this information, which can
>>> fail. If that fails, fh_pre_saved can end up not being set. While this
>>> situation is unfortunate, we don't need to crash the box.
>>=20
>> I'm always happy to get rid of a BUG_ON(). But I'm not sure even
>> a warning is necessary in this case. It's not likely that it's
>> a software bug or something that the server administrator can
>> do something about.
>>=20
>> Can you elaborate on why the vfs_getattr() might fail? Eg, how
>> was it failing in 2223560 ?
>>=20
>=20
> I'm fine with dropping the WARN_ON. You are correct that there is
> probably little the admin can do about it.
>=20
> vfs_getattr can fail for all sorts of reasons. It really depends on the
> underlying filesystem. In 2223560, I don't know for sure, but just prior
> to the oops, there were these messages in the log:
>=20
> [51935.482019] XFS (vda3): Filesystem has been shut down due to log error=
 (0x2).=20
> [51935.482020] XFS (vda3): Please unmount the filesystem and rectify the =
problem(s).=20
> [51935.482550] vda3: writeback error on inode 25320400, offset 2097152, s=
ector 58684120=20
>=20
> My assumption was that the fs being shut down caused some VFS operations
> to start returning errors (including getattr) and that is why
> fh_pre_saved ultimately didn't get set.

I'm wondering if the operation should just fail in this case
rather than return a cobbled-up changeinfo4. Maybe for another
day.


>>> Move set_change_info to nfs4proc.c since all of the callers are there.
>>> Revise the condition for setting "atomic" to also check for
>>> fh_pre_saved. Drop the BUG_ON and make it a WARN_ON, and just have it
>>> zero out both change_attr4s when this occurs.
>>>=20
>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=3D2223560
>>> Reported-by: Boyang Xue <bxue@redhat.com>
>>=20
>> checkpatch now wants
>>=20
>> Reported-by:
>> Closes:
>>=20
>> in that order.
>>=20
>=20
>=20
> Mmmmkay. So I assume the URL should go in the Closes: field then?

Yes, a bug link goes in the Closes: field.


> I'll take out the WARN_ON_ONCE and resend, once others have had a chance
> to comment.

Don't miss the other comments below.


> Thanks!
>=20
>>=20
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>> fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++++++++++
>>> fs/nfsd/xdr4.h     | 11 -----------
>>> 2 files changed, 30 insertions(+), 11 deletions(-)
>>>=20
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index d8e7a533f9d2..e6f406f27821 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -380,6 +380,36 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct s=
vc_fh *fhp,
>>> return status;
>>> }
>>>=20
>>> +/**
>>> + * set_change_info - set up the change_info4 for a reply
>>> + * @cinfo: pointer to nfsd4_change_info to be populated
>>> + * @fhp: pointer to svc_fh to use as source
>>> + *
>>> + * Many operations in NFSv4 require change_info4 in the reply. This fu=
nction
>>> + * populates that from the info that we (should!) have already collect=
ed. In
>>> + * the event that we didn't get any pre-attrs, throw a warning and jus=
t
>>> + * zero out both change_attr4 fields.
>>> + */
>>> +static void
>>> +set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp)
>>> +{
>>> + cinfo->atomic =3D (u32)(fhp->fh_pre_saved && fhp->fh_post_saved && !f=
hp->fh_no_atomic_attr);
>>> +
>>> + /*
>>> + * In the event that we couldn't fetch attributes from the
>>> + * server for some reason, just zero out the before and after
>>=20
>> "From the server"? Is it only likely to fail if the exported
>> filesystem is an NFS mount? Or did you mean "from the filesystem" ?
>>=20
>>=20
>>> + * change values, after throwing a warning.
>>> + */
>>> + if (WARN_ON_ONCE(!fhp->fh_pre_saved)) {
>>=20
>> Maybe you should clear ->atomic as well in this case.
>>=20
>>=20
>>> + cinfo->before_change =3D 0;
>>> + cinfo->after_change =3D 0;
>>> + return;
>>> + }
>>> +
>>> + cinfo->before_change =3D fhp->fh_pre_change;
>>> + cinfo->after_change =3D fhp->fh_post_change;
>>> +}
>>> +
>>> static __be32
>>> do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cst=
ate, struct nfsd4_open *open, struct svc_fh **resfh)
>>> {
>>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>>> index b2931fdf53be..9e67f63c5f4d 100644
>>> --- a/fs/nfsd/xdr4.h
>>> +++ b/fs/nfsd/xdr4.h
>>> @@ -775,17 +775,6 @@ void warn_on_nonidempotent_op(struct nfsd4_op *op)=
;
>>>=20
>>> #define NFS4_SVC_XDRSIZE sizeof(struct nfsd4_compoundargs)
>>>=20
>>> -static inline void
>>> -set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp)
>>> -{
>>> - BUG_ON(!fhp->fh_pre_saved);
>>> - cinfo->atomic =3D (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr=
);
>>> -
>>> - cinfo->before_change =3D fhp->fh_pre_change;
>>> - cinfo->after_change =3D fhp->fh_post_change;
>>> -}
>>> -
>>> -
>>> bool nfsd4_mach_creds_match(struct nfs4_client *cl, struct svc_rqst *rq=
stp);
>>> bool nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, struct xdr_str=
eam *xdr);
>>> bool nfs4svc_encode_compoundres(struct svc_rqst *rqstp, struct xdr_stre=
am *xdr);
>>>=20
>>> ---
>>> base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e
>>> change-id: 20230720-bz2223560-9c4690a8217b
>>>=20
>>> Best regards,
>>> --=20
>>> Jeff Layton <jlayton@kernel.org>
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>


--
Chuck Lever