Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Fri, 17 Nov 2023 14:43:09 -0500
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
Cc:     NeilBrown <neilb@suse.de>, linux-nfs@vger.kernel.org,
        Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>
Subject: Re: [PATCH 2/9] nfsd: avoid race after unhash_delegation_locked()
Message-ID: <ZVfCTVTmNd0cgqcY@tissot.1015granger.net>
References: <20231117022121.23310-1-neilb@suse.de>
 <20231117022121.23310-3-neilb@suse.de>
 <40e3c09538c58818e5ab0c713a49d62304c4c4a0.camel@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <40e3c09538c58818e5ab0c713a49d62304c4c4a0.camel@kernel.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?+6HVauOQepiNUEhXe1MdSXZnc7NakJjkNL97Eb2fp6ZoLDzTn48JdOn5lu4A?=
 =?us-ascii?Q?fbws/p0IA6UrBxCifpU6GowPRZKhwydXqmZ0w3ce9CXI83pejYsbBvA50H7X?=
 =?us-ascii?Q?NkoTv+/BbsGOUSQSXJ3x/W4C7/1vERl62FK/RtQvB7lxYvuCVTPdgbZfapBm?=
 =?us-ascii?Q?h/AL3rPiAJxvVr1HtLQeUED94J+9Tahp5bKwXa7HDtPkggYmgRSFcbaBMsl4?=
 =?us-ascii?Q?NL9X9jyk/5LvQpvIsmKcMgVUThoBZZHcxlr+hviXxaVBosUpOjrl2OX27CAh?=
 =?us-ascii?Q?ofLUbRGIgOBAva1bW2OSVc6FSeDAwq6vM8UIMof4hr1u8H7G3ZTqmK/2Xy2N?=
 =?us-ascii?Q?Rjf9auIjIV/CJcrZTtO9O/k30D+X8Titw531XWXPlkCdlZs6+VzFSWYPF5Tw?=
 =?us-ascii?Q?8xUxYtgjbXpiOBvqrRvYKZp2G62xnNIsaxTxApM35lCzsZiRvAl/jGLjTsC2?=
 =?us-ascii?Q?lpCXcwlw91SefPG1VUeleeQn6RVw2j8QRGldGuI+V8qT9xigCDr2tp84jx8O?=
 =?us-ascii?Q?VowMJ3Myz4nW6GjrrBUolzOMlEpfe3UdKahWuRkA3NK2M7cn8ikWqajX8IW0?=
 =?us-ascii?Q?q91DcsuQlXQGKmiyb/HGqvKDoQ2QjPU0LVzS9xTv7ue3SXucw0G2svUCCsts?=
 =?us-ascii?Q?3DfiFj/64/TxxakpQED9HNCzofSuktITWbE1xlysu5oaZFZcliz8z66tZpQY?=
 =?us-ascii?Q?1aSTZA9XQKd86wsOOPDMAwbBpzbI2cdUFDK5oiGynfdB/iD0WISWOznbM7jW?=
 =?us-ascii?Q?E5cz4TDeG8e6Tszk6yCFjICgun86PIFdUmHeewupXoF7cjzO6eNo4sZcRiae?=
 =?us-ascii?Q?ptubjnm1NoQvk3qw3rrCAW72zNr5XctOWdhbWakzOt385+VByCYx7Gmd3D+n?=
 =?us-ascii?Q?oXs6gzCyAySbVwwLlGIpePEe3LSc6cbcWJ1oQWq+6qLsdAaN6+webAatrK+r?=
 =?us-ascii?Q?CPHr//9MXqXxF0dv9RlgjkAB/Y++boD2CtOS+9Pws3TxFdS5LYSId2xMYR8H?=
 =?us-ascii?Q?KmPLpY7Rr+DS9N8/L86vCUrEc61DEc82W/quEMclo714QoeX98JDIr0Plajo?=
 =?us-ascii?Q?JBXfGOm5dRS3RbvyQgXOttep/E7yU5a+ZGhG3f95D+ED7opdvO3Ae9SnZrCS?=
 =?us-ascii?Q?mGveBYrV92q38/5TlK8Pqcw9ROIuc9F1Z+0b2R11PwM1aHaNMcdwQ8DpZoQY?=
 =?us-ascii?Q?z5qgWZsFdZ0R/zKJVLWkL28R1D/8fK6XB/gNS4HB64Fcr7pperg/i03eZ2V/?=
 =?us-ascii?Q?NSQvJmi1EXKBCdlKc8plmpujF0izq6astZhpGc5V28cnbiSpVRfSryaXpvNj?=
 =?us-ascii?Q?ZhYXEtGwXcsc0Nx/ApuOoqNxUtxO7/wClaXSKRicH1ueabHHY9gb4C4gV2Mj?=
 =?us-ascii?Q?00W4YI8hi5IBZERihD/ptJPbEhJQpVKodTYbnIBbZrS0sdwRxaPvUrGdoxq7?=
 =?us-ascii?Q?eLwGtxGfHYbRpRa5KrP1trns0GCw/tGjdhevJFw7C28jIm5Dn1TLqC/APX9U?=
 =?us-ascii?Q?19PkRh853OGZ1vvwzqeqa88QmUXdq44aOoD1JR0PS7rNzyQtzX21KWXwMeLt?=
 =?us-ascii?Q?QbNlx5jMNXZJlC7xaUyUIDcr4pur7RzGl1QKSAIGE2B0mQdD0EiQxB3lACYx?=
 =?us-ascii?Q?bA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: zIgaeFqbhZMVFKhOV72jtM5S6oKLb8bUY9XiqALw3LDotM9kZjq0Bohb4pjjCQY5v30CUStn9Wjux4W8Na3DvsGbDpiJCeism7AAX8tzgCZ2+UK8v9c8SZSSGpv0ZjIKtAH47h5NeAZb4p3ZiR83g912S3/UsV+B+pR8L0C+BBLjXUiUBjQ62tPFlq7swgjC9xgfE7ICTnch9Qw8txrmX43i+nXSGI9wYGRvJMg51hMsM439By5+vZe73bUh5wYvi+f/j1Uax4+RNVJ6zMvI2USmghjwnTWvToF6CPc2gIQtEr11zShyngFpzhN6ASLx/DFhpWbtF5qsLpQh/srfCPRb9t5CNfkI6HBrYGP9cR3BI8EwnPC9megSixc223U9FpwW83YFSZxxHmk/EgkVsu512iHMurypCut+QUalSnnWfxpMWJ8dHz1uaMreiFAiu7O1yJPXh/YFzwvGzo//EUaCLc6JB9zGq8TUzcR4qb+irfdBmqOMvSw/ME3V/iYP/fTbe8DMhoLkq4+lADe9nEzULMvnQhEGlxm84fImwVbwvsY2j2S74tQHutJ2yxU4m62Fe/VlA1g6tm7vtZ6IGw40NcN/a2M5xoHxxiF/sOCIh2fo0r/7dN0saxzhUOuM4aGzXnUi2rJeZsYsm9ZCk4Qm7+GFvlQzk83U7rNUqOGcVeL16Pg4VHf5pz86Y00QVoghLTfEBdNhmLIfMFaANrhkiwuwUpifO0ma0rbIwU+k4reC6g0UmrRjdWXdc6FpSNBelq4rzpIABD/3cGKJ8iMFxe62MAjtOSH9ke6tRq4o/V2f8zrygpWq8SvDAehY1ZJ7jjgIO36I/9L52Nn3hAc4Kl/6uwyuhKR9ZZBB3H1Om8GEdUAQrRekabwbapFY
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6bd198cc-a7f2-4305-5e4a-08dbe7a56f2e
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Nov 2023 19:43:12.5517
 (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: uOIyIk2dya+qfn8EzJpsxtBLacFI6xfVytUuW879pTW3dnjeuFtR4m3yC7ggtFAfW4dxrXuF7qCJV6emj33ppw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR10MB7194
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_19,2023-11-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=945
 spamscore=0 suspectscore=0 phishscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311060000 definitions=main-2311170150
X-Proofpoint-GUID: VCpTMSKHgisx_nbj-sWyF1r9DqeMKJzS
X-Proofpoint-ORIG-GUID: VCpTMSKHgisx_nbj-sWyF1r9DqeMKJzS
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
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 (lipwig.vger.email [0.0.0.0]); Fri, 17 Nov 2023 11:45:22 -0800 (PST)

On Fri, Nov 17, 2023 at 06:41:37AM -0500, Jeff Layton wrote:
> On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > NFS4_CLOSED_DELEG_STID and NFS4_REVOKED_DELEG_STID are similar in
> > purpose.
> > REVOKED is used for NFSv4.1 states which have been revoked because the
> > lease has expired.  CLOSED is used in other cases.
> > The difference has two practical effects.
> > 1/ REVOKED states are on the ->cl_revoked list
> > 2/ REVOKED states result in nfserr_deleg_revoked from
> >    nfsd4_verify_open_stid() asnd nfsd4_validate_stateid while
> >    CLOSED states result in nfserr_bad_stid.
> > 
> > Currently a state that is being revoked is first set to "CLOSED" in
> > unhash_delegation_locked(), then possibly to "REVOKED" in
> > revoke_delegation(), at which point it is added to the cl_revoked list.
> > 
> > It is possible that a stateid test could see the CLOSED state
> > which really should be REVOKED, and so return the wrong error code.  So
> > it is safest to remove this window of inconsistency.
> > 
> > With this patch, unhash_delegation_locked() always set the state
> > correctly, and revoke_delegation() no longer changes the state.
> > 
> > Also remove a redundant test on minorversion when
> > NFS4_REVOKED_DELEG_STID is seen - it can only be seen when minorversion
> > is non-zero.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfsd/nfs4state.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 6368788a7d4e..7469583382fb 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1334,7 +1334,7 @@ static bool delegation_hashed(struct nfs4_delegation *dp)
> >  }
> >  
> >  static bool
> > -unhash_delegation_locked(struct nfs4_delegation *dp)
> > +unhash_delegation_locked(struct nfs4_delegation *dp, unsigned char type)

unsigned short type ?


> >  {
> >  	struct nfs4_file *fp = dp->dl_stid.sc_file;
> >  
> > @@ -1343,7 +1343,9 @@ unhash_delegation_locked(struct nfs4_delegation *dp)
> >  	if (!delegation_hashed(dp))
> >  		return false;
> >  
> > -	dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
> > +	if (dp->dl_stid.sc_client->cl_minorversion == 0)
> > +		type = NFS4_CLOSED_DELEG_STID;
> > +	dp->dl_stid.sc_type = type;
> >  	/* Ensure that deleg break won't try to requeue it */
> >  	++dp->dl_time;
> >  	spin_lock(&fp->fi_lock);
> > @@ -1359,7 +1361,7 @@ static void destroy_delegation(struct nfs4_delegation *dp)
> >  	bool unhashed;
> >  
> >  	spin_lock(&state_lock);
> > -	unhashed = unhash_delegation_locked(dp);
> > +	unhashed = unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID);
> >  	spin_unlock(&state_lock);
> >  	if (unhashed)
> >  		destroy_unhashed_deleg(dp);
> > @@ -1373,9 +1375,8 @@ static void revoke_delegation(struct nfs4_delegation *dp)
> >  
> >  	trace_nfsd_stid_revoke(&dp->dl_stid);
> >  
> > -	if (clp->cl_minorversion) {
> > +	if (dp->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> >  		spin_lock(&clp->cl_lock);
> > -		dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
> >  		refcount_inc(&dp->dl_stid.sc_count);
> >  		list_add(&dp->dl_recall_lru, &clp->cl_revoked);
> >  		spin_unlock(&clp->cl_lock);
> > @@ -2234,7 +2235,7 @@ __destroy_client(struct nfs4_client *clp)
> >  	spin_lock(&state_lock);
> >  	while (!list_empty(&clp->cl_delegations)) {
> >  		dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
> > -		WARN_ON(!unhash_delegation_locked(dp));
> > +		WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));
> >  		list_add(&dp->dl_recall_lru, &reaplist);
> >  	}
> >  	spin_unlock(&state_lock);
> > @@ -5197,8 +5198,7 @@ nfs4_check_deleg(struct nfs4_client *cl, struct nfsd4_open *open,
> >  		goto out;
> >  	if (deleg->dl_stid.sc_type == NFS4_REVOKED_DELEG_STID) {
> >  		nfs4_put_stid(&deleg->dl_stid);
> > -		if (cl->cl_minorversion)
> > -			status = nfserr_deleg_revoked;
> > +		status = nfserr_deleg_revoked;
> >  		goto out;
> >  	}
> >  	flags = share_access_to_flags(open->op_share_access);
> > @@ -6235,7 +6235,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> >  		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> >  		if (!state_expired(&lt, dp->dl_time))
> >  			break;
> > -		WARN_ON(!unhash_delegation_locked(dp));
> > +		WARN_ON(!unhash_delegation_locked(dp, NFS4_REVOKED_DELEG_STID));
> >  		list_add(&dp->dl_recall_lru, &reaplist);
> >  	}
> >  	spin_unlock(&state_lock);
> > @@ -8350,7 +8350,7 @@ nfs4_state_shutdown_net(struct net *net)
> >  	spin_lock(&state_lock);
> >  	list_for_each_safe(pos, next, &nn->del_recall_lru) {
> >  		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> > -		WARN_ON(!unhash_delegation_locked(dp));
> > +		WARN_ON(!unhash_delegation_locked(dp, NFS4_CLOSED_DELEG_STID));

Neil, what say we get rid of these WARN_ONs?


> >  		list_add(&dp->dl_recall_lru, &reaplist);
> >  	}
> >  	spin_unlock(&state_lock);
> 
> Same question here. Should this go to stable? I guess the race is not
> generally fatal...

Again, there's no existing bug report, so no urgency to get this
backported. I would see these changes soak in upstream rather than
pull them into stable quickly only to discover another bug has been
introduced.

We don't have a failing test or a data corruption risk, as far as
I can tell.


> Reviewed-by: Jeff Layton <jlayton@kernel.org>

-- 
Chuck Lever