Received-SPF: pass (google.com: domain of linux-nfs+bounces-1240-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 22 Jan 2024 09:27:53 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: NeilBrown <neilb@suse.de>
Cc: Jeff Layton <jlayton@kernel.org>, Dai Ngo <Dai.Ngo@oracle.com>,
        Olga Kornievskaia <kolga@netapp.com>, Tom Talpey <tom@talpey.com>,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH] nfsd: fix RELEASE_LOCKOWNER
Message-ID: <Za57adpDbKJavMRO@tissot.1015granger.net>
References: <170589589641.23031.16356786177193106749@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <170589589641.23031.16356786177193106749@noble.neil.brown.name>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?jzggSwlJN80pDcpCZAxX9QPVuiGSbAmkAeuEn2VhQ+iVsHlOFLZ5K2Ya9Jrv?=
 =?us-ascii?Q?N3eYH1ceFEMMGMRponcagaHIKzH6VA9SGF/ZQjcFLBV5QkMWCc80qN+aJkjC?=
 =?us-ascii?Q?ysYXq8PQklnAtAY4ldd/wHhNVLHhW+NxqanfrluohMV5Bmbesuv13nZA8ZU4?=
 =?us-ascii?Q?iWPgWAAg2Md/Zu3AEl5SayEs/QNu3xfABNVLJAygMpkk2tuDPWwNuW62dwLh?=
 =?us-ascii?Q?5appA0uhEDjVfZzB6/nBTuoYoEkZLH2Vgjex9yrYkrIh3Qmg2xZ9N6DYiqwY?=
 =?us-ascii?Q?iniKbs08cdMpkFBxJLAdcV56RoEGZgFiovcGDD6MFEXKY3tf2fpmzE+YvSey?=
 =?us-ascii?Q?s5DjQXH4pzVGl3UrXrXiz13LdKmtN18L83MVdRPv4Srac3wVJ15xe109Lk1K?=
 =?us-ascii?Q?Ugr44dLbn08nH12wszN5gjTZZ96TX0CKsJMBcmN1m/xo6gwzEYK7Ne8dKxXs?=
 =?us-ascii?Q?atZlHoE5H9+P6N51kyJvS/TqsztESHXKIOLBM/9mZywAMFg2OVddrDDlZVtg?=
 =?us-ascii?Q?G3pXQ/q+viyHYPOQWPT9nH4OehzuOEJflW+417yXIIl/X5dKIVO8EHRbOmIv?=
 =?us-ascii?Q?8mqGbd/8UxGHT2Y330B/BB9zlhs8rTV8UMcElFyz4M0DvwNIyGeOzfi6FQmx?=
 =?us-ascii?Q?hVL0QoRNPmA0HQVlQhqohH/66uBLrAUgjLsIulHLMv0QGVFaW1G2+vyFfeS6?=
 =?us-ascii?Q?H9k9ynz+UmYub3ZC6PsxJ0+6qEsTqah8UmG6E6aZoFQpGcjfmbh2COB3C6/L?=
 =?us-ascii?Q?I1+yNpd++S8U8HRWP/nL3zZeXu+ufpXro7w9rCnRfxubFHEMDiMW0XtwXOil?=
 =?us-ascii?Q?ssVpWOjSEFCDJViLCUYYXlnlYvy+IoJBrPDJ797D190qg+nXjsgkekU8Mmo5?=
 =?us-ascii?Q?3dlUyoE7AtQW9MT42keC7mq8Q++ibnff9EOvaZiG9UPnhM76AOxr7bmIs4V8?=
 =?us-ascii?Q?N6rhuD8mMUAxEL3VVBF10pV4dp8k59Hv3BWlytvp1bb7/u7Sp4PWbOQxIS+t?=
 =?us-ascii?Q?iRHhg2RpECoZ1zhCKaPwTSm9vRJb4k3vpurW/DWebIQzXsZSv/E01kN1ofjk?=
 =?us-ascii?Q?GOGOfuQXpiBTmQ9VV+4BNrEKm9yKhvlz6x7ZibpldD7jidTY/Oskbsjd7hV9?=
 =?us-ascii?Q?2Iw7s0ueyPbWxWFaXfJWlNC9ZoxXqVW+YI4mbtfqpcspvMrl8uIZiU2Jc/ow?=
 =?us-ascii?Q?Yk1oEyHVVzj9NaVNFieDaBGMCqVIUxMq0Z7ji4DpXk3A6wAbDX93t4ZgjGRS?=
 =?us-ascii?Q?AGNWwmKN8GNuiKRioGWeZCN+usZvKM2F0p4NwrFjf+PwhV0C0IjcjgBkZxAX?=
 =?us-ascii?Q?+zXIfo3es5XI+LYa5gXGOPXQavP7Kae7V94SF/fJGoACOFDuHhpjcBwQ9rYA?=
 =?us-ascii?Q?PUp2w9M7LyYoQton3eY5b66eV36taZPJwVRRKxrlnqkYRS4DJPvNwd5FW489?=
 =?us-ascii?Q?4Qjl+zxWr/4TwyYD5naoaHX2vLky9LxhBN+jMfUFF49jDibC3hE2XbzKs7Ed?=
 =?us-ascii?Q?hUbMFpipjtI/i8tiF2zF10FZ7dIzbWgS93QyFQrgU1+a7N2HJIG1f9onaAnD?=
 =?us-ascii?Q?fTvN5fe2udUjz+2pGxOwczmgQ9mViSygyIXf6QDhlNk5sQcptOK0btlNVJP1?=
 =?us-ascii?Q?zg=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	RDhzialYMzNBD0Mwr/AEgcyyLnR8jvHnv34341nGXUQFYZDYalFjAxHZ+UFp6DV4CPRwVbnRQvDHfhLvVsWlwRjqXgYuN7ZqgY10sHalDQB+6XIGoGGHMdac789H3Wa5WPMLE3Cfo/NBBp8NCqIPCEb2JCC8dm1KEwerYu2sjB6uCH924YP3RqX/Mv7iO9FaBS4lAT+3dfUXlpPih3KXMZQsg5M+teXy0zqdM5FwdZad8a9yOjOzzqkXGN8H+vnDiEY0wls7Bp/cKP6OVnR9i0KNvKLD50Kq3zvCCdptbjUNoJEe8RKFADw+8CpMf57xYEp8HUqDG+GMLTzqyoVsdkOsq2dqXam8W5EcyqffTmAqv/wzhsMWw5wDv+LjnIe6WngXZ3wIf5lQaBWlIzGcitN3vA16YmHAPOWAAOpENOVi3h/+ARlW1+xih8j+jfg/rSHmkAiyyFQeV1BffwdwGbwU+WEP68Y4igoR7/vnL31sa3c9h8maschLzCO960n+Fanmd+ivovta5MgumI8mdFD5MYHntfIPicSX0E/SmeRuo3bTMTKn8r28iMScOoveHtkihS7rfcqN9dbU9kKRiAQGKzHuuGFrKm1gThbbmqE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 29edb409-b080-4ce3-eff6-08dc1b5653dc
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jan 2024 14:27:57.1108
 (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: NUDrhfYm4IxUYgbzxlm8oelS3MeAFxJneqISRblNyxMkSEojrWBRdSuKxH+O2xOVTOoS/jqurNJThNk4AyKH9g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4898
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-22_05,2024-01-22_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=999
 spamscore=0 mlxscore=0 adultscore=0 bulkscore=0 phishscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401220097
X-Proofpoint-ORIG-GUID: SJ3JAfXrHs96M5cBx4dgwZNt1BzJNKCU
X-Proofpoint-GUID: SJ3JAfXrHs96M5cBx4dgwZNt1BzJNKCU

On Mon, Jan 22, 2024 at 02:58:16PM +1100, NeilBrown wrote:
> 
> The test on so_count in nfsd4_release_lockowner() is nonsense and
> harmful.  Revert to using check_for_locks(), changing that to not sleep.
> 
> First: harmful.
> As is documented in the kdoc comment for nfsd4_release_lockowner(), the
> test on so_count can transiently return a false positive resulting in a
> return of NFS4ERR_LOCKS_HELD when in fact no locks are held.  This is
> clearly a protocol violation and with the Linux NFS client it can cause
> incorrect behaviour.
> 
> If NFS4_RELEASE_LOCKOWNER is sent while some other thread is still
> processing a LOCK request which failed because, at the time that request
> was received, the given owner held a conflicting lock, then the nfsd
> thread processing that LOCK request can hold a reference (conflock) to
> the lock owner that causes nfsd4_release_lockowner() to return an
> incorrect error.
> 
> The Linux NFS client ignores that NFS4ERR_LOCKS_HELD error because it
> never sends NFS4_RELEASE_LOCKOWNER without first releasing any locks, so
> it knows that the error is impossible.  It assumes the lock owner was in
> fact released so it feels free to use the same lock owner identifier in
> some later locking request.
> 
> When it does reuse a lock owner identifier for which a previous RELEASE
> failed, it will naturally use a lock_seqid of zero.  However the server,
> which didn't release the lock owner, will expect a larger lock_seqid and
> so will respond with NFS4ERR_BAD_SEQID.
> 
> So clearly it is harmful to allow a false positive, which testing
> so_count allows.
> 
> The test is nonsense because ... well... it doesn't mean anything.
> 
> so_count is the sum of three different counts.
> 1/ the set of states listed on so_stateids
> 2/ the set of active vfs locks owned by any of those states
> 3/ various transient counts such as for conflicting locks.
> 
> When it is tested against '2' it is clear that one of these is the
> transient reference obtained by find_lockowner_str_locked().  It is not
> clear what the other one is expected to be.
> 
> In practice, the count is often 2 because there is precisely one state
> on so_stateids.  If there were more, this would fail.
> 
> It my testing I see two circumstances when RELEASE_LOCKOWNER is called.
> In one case, CLOSE is called before RELEASE_LOCKOWNER.  That results in
> all the lock states being removed, and so the lockowner being discarded
> (it is removed when there are no more references which usually happens
> when the lock state is discarded).  When nfsd4_release_lockowner() finds
> that the lock owner doesn't exist, it returns success.
> 
> The other case shows an so_count of '2' and precisely one state listed
> in so_stateid.  It appears that the Linux client uses a separate lock
> owner for each file resulting in one lock state per lock owner, so this
> test on '2' is safe.  For another client it might not be safe.
> 
> So this patch changes check_for_locks() to use the (newish)
> find_any_file_locked() so that it doesn't take a reference on the
> nfs4_file and so never calls nfsd_file_put(), and so never sleeps.

More to the point, find_any_file_locked() was added by commit
e0aa651068bf ("nfsd: don't call nfsd_file_put from client states
seqfile display"), which was merged several months /after/ commit
ce3c4ad7f4ce ("NFSD: Fix possible sleep during
nfsd4_release_lockowner()").

Not having to deal with nfsd_file_put() in check_for_locks is a Good
Thing.

Am I correct in observing that the new check_for_locks() is the only
place where flc_lock and fi_lock are held concurrently?


> With
> this check is it safe to restore the use of check_for_locks() rather
> than testing so_count against the mysterious '2'.
> 
> Fixes: ce3c4ad7f4ce ("NFSD: Fix possible sleep during nfsd4_release_lockowner()")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2fa54cfd4882..6dc6340e2852 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7911,14 +7911,16 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner)
>  {
>  	struct file_lock *fl;
>  	int status = false;
> -	struct nfsd_file *nf = find_any_file(fp);
> +	struct nfsd_file *nf;
>  	struct inode *inode;
>  	struct file_lock_context *flctx;
>  
> +	spin_lock(&fp->fi_lock);
> +	nf = find_any_file_locked(fp);
>  	if (!nf) {
>  		/* Any valid lock stateid should have some sort of access */
>  		WARN_ON_ONCE(1);
> -		return status;
> +		goto out;
>  	}
>  
>  	inode = file_inode(nf->nf_file);
> @@ -7934,7 +7936,8 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner)
>  		}
>  		spin_unlock(&flctx->flc_lock);
>  	}
> -	nfsd_file_put(nf);
> +out:
> +	spin_unlock(&fp->fi_lock);
>  	return status;
>  }
>  
> @@ -7944,10 +7947,8 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner)
>   * @cstate: NFSv4 COMPOUND state
>   * @u: RELEASE_LOCKOWNER arguments
>   *
> - * The lockowner's so_count is bumped when a lock record is added
> - * or when copying a conflicting lock. The latter case is brief,
> - * but can lead to fleeting false positives when looking for
> - * locks-in-use.
> + * Check if theree are any locks still held and if not - free the lockowner
> + * and any lock state that is owned.
>   *
>   * Return values:
>   *   %nfs_ok: lockowner released or not found
> @@ -7983,10 +7984,13 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>  		spin_unlock(&clp->cl_lock);
>  		return nfs_ok;
>  	}
> -	if (atomic_read(&lo->lo_owner.so_count) != 2) {
> -		spin_unlock(&clp->cl_lock);
> -		nfs4_put_stateowner(&lo->lo_owner);
> -		return nfserr_locks_held;
> +
> +	list_for_each_entry(stp, &lo->lo_owner.so_stateids, st_perstateowner) {
> +		if (check_for_locks(stp->st_stid.sc_file, lo)) {
> +			spin_unlock(&clp->cl_lock);
> +			nfs4_put_stateowner(&lo->lo_owner);
> +			return nfserr_locks_held;
> +		}
>  	}
>  	unhash_lockowner_locked(lo);
>  	while (!list_empty(&lo->lo_owner.so_stateids)) {
> -- 
> 2.43.0
> 

-- 
Chuck Lever