Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Fri, 17 Nov 2023 10:08:22 -0500
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
Cc:     Chuck Lever <cel@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH v2 1/3] NFSD: Fix "start of NFS reply" pointer passed to
 nfsd_cache_update()
Message-ID: <ZVeB5kfH6YAzgwd1@tissot.1015granger.net>
References: <169963305585.5404.9796036538735192053.stgit@bazille.1015granger.net>
 <169963371324.5404.3057239228897633466.stgit@bazille.1015granger.net>
 <ec54d81630f78c764c930a50b4ba75b26e4b8ff0.camel@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ec54d81630f78c764c930a50b4ba75b26e4b8ff0.camel@kernel.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?rB7F1Q4sdp1KutGXFCiuN9ISokSMpX1V+bCP38FDou2Dz0+PFQm+JYqSgJpV?=
 =?us-ascii?Q?FNX5iLx22+evxZOQx/UfByqRJX8Qo0ln3/Fq5ctJKLchJbdLt1F4pxS/7OQV?=
 =?us-ascii?Q?kcdqDLy/p2PpEO/16UYsEuM2dZ4JY6wzItfhfYETidD4oZQ50BIGVbqAIoPK?=
 =?us-ascii?Q?W+3MIuj4gyowZffeUxQY0urn9jLkmyTiWjfmHljfo38+6xPhhpbwzYe+lIkJ?=
 =?us-ascii?Q?FX04PMpm57D6rhpT+74MA2pVWO6Ju1ZXO5/D/17JTpLNioyJjAAblq9wGuNX?=
 =?us-ascii?Q?lUhfdAVc0QGh26HI9Npoh79umeL6Mc7x18a+xl6m+U/rYP3PYCmp4xXLva5I?=
 =?us-ascii?Q?q+S0AwYVaBLiGOe0pqg3aCFSg6k/TGNW/HlzPzSU815KhuTPpVyPoA2maRHl?=
 =?us-ascii?Q?1s0UX1LDM3LIKQ4omHvkD9rhGXhkqtggLkgKE3HE6MPUcrEVBPDmDgzdss1I?=
 =?us-ascii?Q?aID0zMHAsyZ4XVSMqdNyGdf87a/qnanDPoV8I+13H1xpXJyTD/IAekkICKiX?=
 =?us-ascii?Q?GYhgsGbfdORexXMjnKgd/OUVWD/TtBh1Tkykpt+eZBgcOEVFy5Iy5F8l/qhh?=
 =?us-ascii?Q?aDTQfNlkXJ+Mk778sKERVTfb02ylMKOj9YnnKmorc8N8CAnoYROxHPBgw1W6?=
 =?us-ascii?Q?nWMiZrie8dBESu/Bo44oq4HcfnIa3HeyXUsc8ShOGZWwle5bsYxnqJdGqQB6?=
 =?us-ascii?Q?rODF1+dYY2Imc7uDZqCzJYhlOJWFFc5jc1SYe854rWWS2w/cIHpMH83sLxHd?=
 =?us-ascii?Q?ANRzp4apcHKC3dxnI5Yx7xG8ljjNJZXDqWELlux+L407m6cAxU+i78ONZXKX?=
 =?us-ascii?Q?TUi5rxqrmOvr/AwRXC0s7vrHFKWknmkTdyYisGcEmdK7wFLm7ygTpfrvDOBC?=
 =?us-ascii?Q?A4Ugwneq0+5qFpmPX6Gp1ycpzJp7A3NcCKAy4lbJBuM7LAtTL0YLnP2HWs/S?=
 =?us-ascii?Q?Yl1zAjL5G7BLAj6du8Vw8/OnYbjmIkaguZW5ZNh+8q1bxU3V8/dRlf5CIfQM?=
 =?us-ascii?Q?IbfOJGOJqapa08nywGwciXgH+HvZeFhllb9W/CkJpcYCUcOSEstFaW2moIQv?=
 =?us-ascii?Q?AxOoyRGruvMAa4PMRtjNAS/IGfpuO5Xj+7YWzEVTXJDhtpE6b8SvAu2JAq4c?=
 =?us-ascii?Q?TExWLH8Tbl/O5v3Mo8emkTz9Ni6KF696/uDvDyXw7WlDLM2JCs3A/FLo6OJm?=
 =?us-ascii?Q?3FDQDHOr114My6oEJCeVra2BuDmWWpmA3nOVERva5Ow2MOtzMMLn0iYT5JfD?=
 =?us-ascii?Q?m7QwVm/6m33N1bVUwWre/HWUDZSRvcQR6n1bJoUM5gV+Fk8+o++AZTMY3HCF?=
 =?us-ascii?Q?NgaWpk83DCzeRN1CDTw6sscpInaiPnbYiK7s/RiSUzqb3m+QbmxTFEw5JgOu?=
 =?us-ascii?Q?3iuvntZHXRNb0t1Rs8VSykLhG81wYNuXValSdGD/E4Cv/QiwrYSIOtFZYmHs?=
 =?us-ascii?Q?+k4Cs5BkobHeozh0655gU29NP7E+HCf1TH098oz9nZyiBwLYpgd80pThLQL+?=
 =?us-ascii?Q?3FUtu6V93Guf4VydY6/R3vzPiVNSI9kKnrJcV8NeOSDM0b7VvhKEOsUIVLAM?=
 =?us-ascii?Q?ACjCjujvfXRSYBife+ilb4gkf377Ck6RzZmDeStY7lktHhvY2NlEFT5nCPdh?=
 =?us-ascii?Q?5A=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: AZXdktiXqCIYBIreBurs6+miSrwYMgh/nO+4yMpP0KMf/xZ0rU+wPfBrULV3R/i9GRCMeHpS0go9w1xJY6qwgrEjeIiiDLrp9U165ydmKmdvwSXgkptJx8vDPkDeuA8G56qU2HEE07T5Ow9LrTWqoj1bC08cVT4tLnwhqJNT1snsuaK4nk4c8ebXP58CHoPwk6EMYJ5lw767jb6bTn4lSeCBHkTRtwfwghWnXN9wP+gPlyDoFr1TFTmp0Klbfw5ybWsVuVVv7nf6smDevQ+CWDV0MlqGEEQKILzzNnhuF5upFT+VPuxPGvHrNPJiE/ujTTOgYY+GC4raCFoCuCwBcICh5KNqkLPmA7vhEbCjwJAuMA7+ldTJJUV+dE1GdwJNaVHq8VyhO6sDfLXYFy7XsSHDiEIl7srTjG+GCgRcj4ovLzdfC4NoESzcozEclKAXqGrbQlnM6pdTzSluufYffD7KjM2vMVbQQA3TpScN9QXHiTy1AE57ofyHhN8aZvRM7Mz/2D532TjwsgAHYgRK+RPhH4Ao85q08I+krVIpRtlT1Z2szK5mD4jYzxdPNik2J3UpGyxtwDVOoPI/Toztnnwyo+Dn1+JcF7lhgMYeTJTO0fMfqiA1U9oXbGywigth5Tdf0O94lqRQXlslzXuKD/JcD7VNINLyGn6tvC21fm7uMGrfXl2ZUrOsLzoNdNiGWdkfh7rxtiMHQGvq0y3yqCEOzrena3lgnlK0lhoVVKMd4BilrsIShs/bmdJtppln27faLFXWanZ7E5gQvdF3l+3hFkXjM26jcyTNUZznMow=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4e882e9e-8fbb-44d4-b027-08dbe77f0be5
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Nov 2023 15:08:25.0724
 (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: h5BCCDarDu0j4GvZ0stkfJlWZT2J49NUVy2FPR/GCWYrzW3UtFGTVAssFs430roy6j9vxbwFIodY7Hv9+aRHiw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR10MB6042
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-11-17_14,2023-11-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxscore=0 bulkscore=0
 phishscore=0 suspectscore=0 mlxlogscore=999 spamscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311060000
 definitions=main-2311170113
X-Proofpoint-GUID: vqjFQ2SZHoG_zAduQUotdckizA-6ofcV
X-Proofpoint-ORIG-GUID: vqjFQ2SZHoG_zAduQUotdckizA-6ofcV
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
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Fri, 17 Nov 2023 07:08:42 -0800 (PST)

On Fri, Nov 17, 2023 at 09:57:49AM -0500, Jeff Layton wrote:
> On Fri, 2023-11-10 at 11:28 -0500, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > The "statp + 1" pointer that is passed to nfsd_cache_update() is
> > supposed to point to the start of the egress NFS Reply header. In
> > fact, it does point there for AUTH_SYS and RPCSEC_GSS_KRB5 requests.
> > 
> > But both krb5i and krb5p add fields between the RPC header's
> > accept_stat field and the start of the NFS Reply header. In those
> > cases, "statp + 1" points at the extra fields instead of the Reply.
> > The result is that nfsd_cache_update() caches what looks to the
> > client like garbage.
> > 
> > A connection break can occur for a number of reasons, but the most
> > common reason when using krb5i/p is a GSS sequence number window
> > underrun. When an underrun is detected, the server is obliged to
> > drop the RPC and the connection to force a retransmit with a fresh
> > GSS sequence number. The client presents the same XID, it hits in
> > the server's DRC, and the server returns the garbage cache entry.
> > 
> > The "statp + 1" argument has been used since the oldest changeset
> > in the kernel history repo, so it has been in nfsd_dispatch()
> > literally since before history began. The problem arose only when
> > the server-side GSS implementation was added twenty years ago.
> > 
> > This particular patch applies cleanly to v6.5 and later, but needs
> > some context adjustment to apply to earlier kernels. Before v5.16,
> > nfsd_dispatch() does not use xdr_stream, so saving the NFS header
> > pointer before calling ->pc_encode is still an appropriate fix
> > but it needs to be implemented differently.
> > 
> > Cc: <stable@vger.kernel.org> # v5.16+
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfssvc.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index d6122bb2d167..60aacca2bca6 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -981,6 +981,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp)
> >  	const struct svc_procedure *proc = rqstp->rq_procinfo;
> >  	__be32 *statp = rqstp->rq_accept_statp;
> >  	struct nfsd_cacherep *rp;
> > +	__be32 *nfs_reply;
> >  
> >  	/*
> >  	 * Give the xdr decoder a chance to change this if it wants
> > @@ -1014,6 +1015,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp)
> >  	if (test_bit(RQ_DROPME, &rqstp->rq_flags))
> >  		goto out_update_drop;
> >  
> > +	nfs_reply = xdr_inline_decode(&rqstp->rq_res_stream, 0);
> >  	if (!proc->pc_encode(rqstp, &rqstp->rq_res_stream))
> >  		goto out_encode_err;
> >  
> > @@ -1023,7 +1025,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp)
> >  	 */
> >  	smp_store_release(&rqstp->rq_status_counter, rqstp->rq_status_counter + 1);
> >  
> > -	nfsd_cache_update(rqstp, rp, rqstp->rq_cachetype, statp + 1);
> > +	nfsd_cache_update(rqstp, rp, rqstp->rq_cachetype, nfs_reply);
> >  out_cached_reply:
> >  	return 1;
> >  
> > 
> > 
> 
> With this patch, I'm seeing a regression in pynfs RPLY14. In the
> attached capture the client sends a replay of an earlier call, and the
> server responds (frame #97) with a reply that is truncated just after
> the RPC accept state.

I've reproduced it. Looking now.


-- 
Chuck Lever