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:     Anna Schumaker <anna@kernel.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Bruce Fields <bfields@fieldses.org>
Subject: Re: [PATCH] NFSD: Don't continue encoding if READ_PLUS gets confused
Thread-Topic: [PATCH] NFSD: Don't continue encoding if READ_PLUS gets confused
Thread-Index: AQHYh+HAA8+QSbxkgUGPQSaEnGmW5q1e0x+A
Date:   Fri, 24 Jun 2022 17:40:08 +0000
Message-ID: <B809231A-ACDE-4D97-B866-F969B08AA7EC@oracle.com>
References: <20220624154737.1387850-1-anna@kernel.org>
In-Reply-To: <20220624154737.1387850-1-anna@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?/bL8yQ8BZy5WXgWjLSV7z7fo+m1Ll12YaIYRmaMymOcgKAWsSNR98Vp8CWdn?=
 =?us-ascii?Q?8SSyV80aSPXDziMxQ+LzzYKZIIe4UZWp/l4LfGvLYkm2BQF9Iu+RLMumnBdp?=
 =?us-ascii?Q?fdvnX+1pJB0HUeIN8A5LPsDuDzfM33DYO/89koXL6qTuy0kgQAtJtHG9P+pV?=
 =?us-ascii?Q?MkfF/yLZTvRwL9hd8LbXVE7JLkgXLv0JuE63lBI+v8H6eexMj7ZdHntvbrBo?=
 =?us-ascii?Q?DHyczeMN/mZsYoQOeYz7vymmZmdBbwKNt09BiRu1f7TEA04JubXjhUd5GArQ?=
 =?us-ascii?Q?juUcCXyUlUk7tR+eXkyqCBzx9z+HRzmDb6ncDW8uLnX5Gk3GfIu1xhX76khJ?=
 =?us-ascii?Q?+lh35+uALtBnNlccSBzEWvzoK11xqYy9BseUZW54Ws1GZlijTwMn/lDySQSC?=
 =?us-ascii?Q?UsnDFW1tIvrObNtkp2XbJB64cWJO4iPJCdZIZMP9y9np+DdATfTVTwOtzlOG?=
 =?us-ascii?Q?22P/4S0+Ei8nbTARDIzP4XXrd4fiAhcKye8JMdVoRVOzyIHP97p4F6oiH3oL?=
 =?us-ascii?Q?UNmU6nbHoZYJ8IutZRn2nz380b0B7umku2eg7q3/AwDWnDskeqZJqpgDmorG?=
 =?us-ascii?Q?AmWMyYTE06RG3Paq3gNM7rbDHAitsCF/0fHPMAIN3tL672jelnYan3X8gIxQ?=
 =?us-ascii?Q?pH2e3obumfhAgZPQP2jesIVz6IWJ42D+EiONaCVLNhleopRQno30uOHbaW0n?=
 =?us-ascii?Q?Or348C/bGj0nfvyRKF3smG8ML3nQ21ielI/XiEonfVfrVYyANVwCRsepaG8D?=
 =?us-ascii?Q?9v7WPQSUjo2o7JevVpiS6LMKOgIMQpUyr/qfkqTKEIw0GEk9bUcLcXjuCD8T?=
 =?us-ascii?Q?QwpEqqaAW2XCC3Uiven8woBLn8HFXgxlZaarnVlQwEceGc++CavHCbhoYmFV?=
 =?us-ascii?Q?FBnOuroHemB5Uc78Pk8SxLIYuPJMrPVpFDvZmMD+1bLhSo7mxugU6dbXf2t6?=
 =?us-ascii?Q?1cxaRUcR0e4fgKJIjCSomEPHeBYC7Z8+tr9wWBoBTQcPpliisyv6KYVTZmz9?=
 =?us-ascii?Q?IsGQ0AeYIXgFUxlDJ9qqvKXx3HHFTjDXJ3ekeOGAsGgzkdg0RBB4spFW1WmL?=
 =?us-ascii?Q?R3q2uZC+0OsfapsjzOifYKfxuhBO62ra6s8pDXrqMOfcfqA7y8K4ZK31N78k?=
 =?us-ascii?Q?mEFVEt9BBO2tsuY29JmnJyaNXcmHVNnPVFLQYtzLQ9zuYeXnsARKUa3JzIwi?=
 =?us-ascii?Q?DgTop2Hl5PB+D8FV7JIO0MO1FtA+zioJwPOvJiVedS4Dn5GjtdUIqhzC9l+g?=
 =?us-ascii?Q?2iMXJKWATHW4AQdnrzUo827P1lUTuwcOSt3rgwFXoNxrZAj+iijdFq6eU3k/?=
 =?us-ascii?Q?G3YTbgZU5S1ej61THeDmIG6aTPeoIFlwjr6DlB2Ekxrq95fyU1X3oDxUZmMz?=
 =?us-ascii?Q?XdJNFAaeNa1TcOgVC9Rv1d2CQPF+JcOYrPFjsfCCqPEUSvjdVTCmbBQG/Hhm?=
 =?us-ascii?Q?n8jRaw8wUAgzchU+NNOFdw5ICbPVEFgjon45OV6ZPulYtXtKTiDrqnhDDzvp?=
 =?us-ascii?Q?TTnaNykRn7galWEgE8+u7kqa1ciXc+uJ0/+moxjv8YMJtbv3OMcR8XO+CXFZ?=
 =?us-ascii?Q?+UhH3OuND57jNg1hycQFAV82I75kdj7YH4ZtW5kB/SPTw9SS+ALAF6l620xv?=
 =?us-ascii?Q?MA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3E4EADDCE0F31D4595D0BD8CFD332E31@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: 713e699b-100e-4fdc-d670-08da560894cd
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jun 2022 17:40:08.2122
 (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: qildBAfqhrLYnt3jYV0SomatexW96ESf1WQxkAPW67pBbLf3VWwE8StlDLzSfuNkD3DVCIYMupSn/S9vwvGizQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR1001MB2084
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-06-24_08:2022-06-23,2022-06-24 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 malwarescore=0
 mlxlogscore=999 bulkscore=0 phishscore=0 suspectscore=0 spamscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2204290000 definitions=main-2206240067
X-Proofpoint-ORIG-GUID: fY6ZQiTNfAKkGKnTHihcQSmyyTpTIFai
X-Proofpoint-GUID: fY6ZQiTNfAKkGKnTHihcQSmyyTpTIFai
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,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

Hi Anna!

> On Jun 24, 2022, at 11:47 AM, Anna Schumaker <anna@kernel.org> wrote:
>=20
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>=20
> If we were in a HOLE segement, but vfs_llseek() claimed we were encoding
> DATA then we would switch over to the DATA encoding function. This
> conflicts with Chuck's latest xdr cleanup patches and can result in a
> crash or silent hang. Let's just return nfserr_io if we find we are in
> this situation, which will cause the encoder to return to the client
> with the number of segments already encoded. The client can then try the
> READ_PLUS call again.
>=20
> Fxes: 6c254bf3b637 (SUNRPC: Fix the calculation of xdr->end in xdr_get_ne=
xt_encode_buffer())

checkpatch complains about this tag.

Also, Bruce said he wasn't able to reproduce the issue on
6c254bf3b637, only on the whole series. Were you able to hit
it with just this commit applied?


> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>

I find this somewhat problematic. I can't apply this patch in
good conscience:

* The usual guideline for applying a workaround upstream is
  that there's been a demonstration that it will be impossible
  to find and fix the real problem. I don't see that here.

* We still don't understand the failure. The XDR code itself
  might be broken? Therefore we don't understand if this
  workaround is 100% effective

* Usually with a workaround, there's a hint of a long-term
  fix... is this the long-term fix?

In other words, I might give this patch to a customer who
needed to get back on her feet quickly. I'm hesitant to take
it as an upstream change without further justification.

IMHO a fix in an XDR consumer (like READ_PLUS) needs to
demonstrate that the current XDR code is working as designed.
Otherwise, the XDR code itself is what needs to be fixed.


> ---
> fs/nfsd/nfs4xdr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>=20
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 61b2aae81abb..dc97d7c7e595 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4792,7 +4792,7 @@ nfsd4_encode_read_plus_hole(struct nfsd4_compoundre=
s *resp,
> 	if (data_pos =3D=3D -ENXIO)
> 		data_pos =3D f_size;
> 	else if (data_pos <=3D read->rd_offset || (data_pos < f_size && data_pos=
 % PAGE_SIZE))
> -		return nfsd4_encode_read_plus_data(resp, read, maxcount, eof, &f_size)=
;
> +		return nfserr_io;
> 	count =3D data_pos - read->rd_offset;
>=20
> 	/* Content type, offset, byte count */
> --=20
> 2.36.1
>=20

--
Chuck Lever