Received-SPF: pass (google.com: domain of linux-nfs+bounces-1790-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 5 Feb 2024 10:24:38 -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: don't take fi_lock in nfsd_break_deleg_cb()
Message-ID: <ZcD9tiK5FDcOH0P+@tissot.1015granger.net>
References: <170709975922.13976.3341850918979137018@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <170709975922.13976.3341850918979137018@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?67NXWZ3eerBg0oBnLsq1pUyGza3WSvGN/YS/PBP2Za5SevHFroSNSDA1e/zj?=
 =?us-ascii?Q?sfCEUkNjnr7ZUaDvH129KWpo5qyhEvI0SImmMTds76yGwmhzPXgioWae3Hb0?=
 =?us-ascii?Q?OLJt3gGkgDiUiZ4RIeL02/yfLgXAAxW3O/MxVd8iWayRxRBbCbJCHgQ197zR?=
 =?us-ascii?Q?KfTVyQkzA/fYb7P8gmgsg7QAAAAxSqI9qPLkdBUV21SbpdO7Q4o2bzXT1CJO?=
 =?us-ascii?Q?EY7GRKUzLMCeS7ZlCHmf+IoL6AZPfrS7its6yZzuvnWOb/zgsv2hxgwUzVMU?=
 =?us-ascii?Q?xXdqxAFQtvRtuX5l0oeVV0t6gryloMf9RbsB4gtX+9H7U2nTG5efG/Tw4p07?=
 =?us-ascii?Q?BxAMIzcblo1gTw1OyLtr8jmVwDE7XDxoMHIIuA+5zHA9qXTDeuHRWJ6s42BR?=
 =?us-ascii?Q?dIPKoYiIG3vO5J8R6kEfgVKKeHuNON9zeMGQKOPlQpkFwJuT/gAXAGIlvo3x?=
 =?us-ascii?Q?ryVVey6RENbq9UEM0qsUPaLLNecEF4+rMCJxsSt067sg7//kvg94NqazhqL+?=
 =?us-ascii?Q?Q/Is6SYAIMPNorsl7W/Zr9nYjU/TdkaflLHwL8U7otNtmaqd1aZ3Fyjs/m6s?=
 =?us-ascii?Q?T65GfGjZTmTu3v6jmPidTdKYM+cyCK5jF0Uy/2AHs6/yQs4ZT7vajc6+JxAG?=
 =?us-ascii?Q?HnLaFxvxQbxS16hCON7v+XDmS24xefDMRXGlUvAkjCvF077sRh+BilgrU2gL?=
 =?us-ascii?Q?1g5plTFW7Yqf/qLHxEJHvkxRPrvvPvFNYXJMxbO7RUuReXkaeaykyw6X4sz8?=
 =?us-ascii?Q?442vuTI7uzlAuxtXs1jHFOdPXNbTRF7FG/9K8K+gvPb5LIqBblO6l5zxrNVU?=
 =?us-ascii?Q?+kvU+NeM1dVKugZYqAEzt7V2s2ztRAp//kWPX7JnW3InvdWSY06RtuQ31/Se?=
 =?us-ascii?Q?XMRbCwYeOtrjMiXUa37fVf+g/CDg+YWEeSP9vdTEbqzy2W5AM4fH/eAySF5q?=
 =?us-ascii?Q?20Xs56NzeWD8X7LZ/QRP7XJNrJjAwAv8o7cGBiT5v6Yg6qL+XYjeALut6mKg?=
 =?us-ascii?Q?ZuWk2AxViV3JbDvvBPSKxX4/LbrTmCe2j9kVzip44Ry2yyhKZL55xpNgCoyk?=
 =?us-ascii?Q?sdFxq9zR8w8KHjIIGk7Qjntl8ExYz8zhw1c0TA+inpLqbRCvBHah58IPgYlh?=
 =?us-ascii?Q?NXSnOLcMLr1Bpcp+lPYmf3m61Mvwirt4hwHwdY43iCvvkvnKUkmyOrkcwUIH?=
 =?us-ascii?Q?gnAZn5N/kuqVowbJOHPagQxfetL0Gjw9vBR0o6U+jCjJugs2SkAPWZkwVCKT?=
 =?us-ascii?Q?nC5ZlH2uWQbNwf4ptikgXrNghwW/VPzogLYUcl556pjEg8FcVkzLFdbuR85S?=
 =?us-ascii?Q?WOQLzsmHjWHwe2k0hmqfBcSiFpyEPSmvzjN+HLs96HgTWneVQa164ZIuYPsb?=
 =?us-ascii?Q?6zB2Z+CaIFDzovVUcIh2+L71ioEN3EvXTNH0mK8C6rMzxSLPU7jpYr4/ZL/C?=
 =?us-ascii?Q?ALEXVYf0OmmO3tTwJoXwLfg+BmsT5fhQdUbBQrrmep79k4NZbksdtr8WFT8R?=
 =?us-ascii?Q?dsZg0RqldH4b+jEUik4y9j+tTycDK+I8OvbNrnPc/DtYwprPbvVOPyXQt3dY?=
 =?us-ascii?Q?IYWG2uB6NJORuRShBN28kqmBI3kymVIpP6yBFpIHbK70tHRagNhhAObnXXPE?=
 =?us-ascii?Q?QA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	820g6u/lVAUGLGSGJ9H99LBvJRkH5HAMrkyFg+YyVO109j1991sWv5eNNTYGMUXW89dICdYsF0RthL7SJpJKWx43x01oP2HhCcA5EwPOolKluqhMmjqzkc5q5SiKpOgU8uHlFId9Sowt177YUchcH/r1mLqABL3jlvnmxpC9gnhv62S6gFjeDMUcvdeMxZyDjgfIut6oVqO1aX21jMvHMZcSAxhrLCaSNoHBBaVldbpYA4rDgMUMomkrBDV3sTFb1H9Oevw9IpGgXRxSmcf2KD+HJyeZ956Otiy5YE7WQKZiQ9xX6XO4FFjYd9rWpibbumLJGzpwtiOV253xHmW376CH+sqQl+Jz3cMeqMUHYlOV8bm8InyjjyjGz+Gz4OTDsM5qdTncN3rQdY2zcHnoTv23dhCUCWGALrh59aD9M/AH8eXXNJqW2kQ2Coj2RUtBg311naIKuT1kbV3SG0vSwe1k4SgVsToJDs3nqIiY2/U7erKtj/yhWm/43N48phyaJiD4xDga7K4rGMZED8/97cgMxKFR9oLfSGNVx5sQL5V1mu/66Kvjk/JuWrctFQaxYRO20kyjaYvmacexAxj2NblcNwuSC3u1ZsCZxOWLva8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 82d6b9c5-ba29-4b17-3a9f-08dc265e9301
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Feb 2024 15:24:41.6923
 (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: RohWwLqLNZZ57ciDWJPnr/0Sl2fOEqxPvv45ePkXOeN/QVae7OISXjyg255Xiz8Mja8AMi1XuXQpC3FHyanVDQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB7715
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-02-05_09,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 spamscore=0
 mlxlogscore=999 bulkscore=0 mlxscore=0 phishscore=0 adultscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402050116
X-Proofpoint-ORIG-GUID: ggA3HxZpZZjotql6WfYa-p4PtjEtymTu
X-Proofpoint-GUID: ggA3HxZpZZjotql6WfYa-p4PtjEtymTu

On Mon, Feb 05, 2024 at 01:22:39PM +1100, NeilBrown wrote:
> 
> A recent change to check_for_locks() changed it to take ->flc_lock while
> holding ->fi_lock.  This creates a lock inversion (reported by lockdep)
> because there is a case where ->fi_lock is taken while holding
> ->flc_lock.
> 
> ->flc_lock is held across ->fl_lmops callbacks, and
> nfsd_break_deleg_cb() is one of those and does take ->fi_lock.  However
> it doesn't need to.
> 
> Prior to v4.17-rc1~110^2~22 ("nfsd: create a separate lease for each
> delegation") nfsd_break_deleg_cb() would walk the ->fi_delegations list
> and so needed the lock.  Since then it doesn't walk the list and doesn't
> need the lock.
> 
> Two actions are performed under the lock.  One is to call
> nfsd_break_one_deleg which calls nfsd4_run_cb().  These doesn't act on
> the nfs4_file at all, so don't need the lock.
> 
> The other is to set ->fi_had_conflict which is in the nfs4_file.
> This field is only ever set here (except when initialised to false)
> so there is no possible problem will multiple threads racing when
> setting it.
> 
> The field is tested twice in nfs4_set_delegation().  The first test does
> not hold a lock and is documented as an opportunistic optimisation, so
> it doesn't impose any need to hold ->fi_lock while setting
> ->fi_had_conflict.
> 
> The second test in nfs4_set_delegation() *is* make under ->fi_lock, so
> removing the locking when ->fi_had_conflict is set could make a change.
> The change could only be interesting if ->fi_had_conflict tested as
> false even though nfsd_break_one_deleg() ran before ->fi_lock was
> unlocked.  i.e. while hash_delegation_locked() was running.
> As hash_delegation_lock() doesn't interact in any way with nfs4_run_cb()
> there can be no importance to this interaction.
> 
> So this patch removes the locking from nfsd_break_one_deleg() and moves
> the final test on ->fi_had_conflict out of the locked region to make it
> clear that locking isn't important to the test.  It is still tested
> *after* vfs_setlease() has succeeded.  This might be significant and as
> vfs_setlease() takes ->flc_lock, and nfsd_break_one_deleg() is called
> under ->flc_lock this "after" is a true ordering provided by a spinlock.
> 
> Fixes: edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 12534e12dbb3..8b112673d389 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5154,10 +5154,8 @@ nfsd_break_deleg_cb(struct file_lock *fl)
>  	 */
>  	fl->fl_break_time = 0;
>  
> -	spin_lock(&fp->fi_lock);
>  	fp->fi_had_conflict = true;
>  	nfsd_break_one_deleg(dp);
> -	spin_unlock(&fp->fi_lock);
>  	return false;
>  }
>  
> @@ -5771,13 +5769,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	if (status)
>  		goto out_unlock;
>  
> +	status = -EAGAIN;
> +	if (fp->fi_had_conflict)
> +		goto out_unlock;
> +
>  	spin_lock(&state_lock);
>  	spin_lock(&clp->cl_lock);
>  	spin_lock(&fp->fi_lock);
> -	if (fp->fi_had_conflict)
> -		status = -EAGAIN;
> -	else
> -		status = hash_delegation_locked(dp, fp);
> +	status = hash_delegation_locked(dp, fp);
>  	spin_unlock(&fp->fi_lock);
>  	spin_unlock(&clp->cl_lock);
>  	spin_unlock(&state_lock);
> -- 
> 2.43.0

Thanks for jumping on this issue.

This version of the fix does not apply to nfsd-fixes since the
ADMIN_REVOKED changes in nfsd-next also touch this part of
nfs4_set_delegation().

Because edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER") is now applied
in v6.8-rc3, v6.7.y, v6.6.y, and probably v6.1.y, I've reworked this
fix slightly to apply on nfsd-fixes and have started a round of
testing there.


-- 
Chuck Lever