Received-SPF: pass (google.com: domain of linux-nfs+bounces-1628-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 31 Jan 2024 10:32:12 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: NeilBrown <neilb@suse.de>
Cc: Jeff Layton <jlayton@kernel.org>, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        linux-nfs@vger.kernel.org, Vasily Averin <vasily.averin@linux.dev>
Subject: Re: [PATCH] nfsd: don't call locks_release_private() twice
 concurrently
Message-ID: <Zbpn/DjauPliPh/t@tissot.1015granger.net>
References: <170666026049.21664.14325649754684255655@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <170666026049.21664.14325649754684255655@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?ZoqqsrL1nmRaE7PCuopC1QgZqzanHeJNiDhgZYp8zdM6h2hdoJm+zA1ZT+3W?=
 =?us-ascii?Q?QID4I3Ur44rchDpZX0Y26ixZZyhvVxmdP1lNE0dYDE0spTqU+LUavIyg8E31?=
 =?us-ascii?Q?MRdGopxtWrYAD0EzFbaftk3iNO427xncOst2MLO1reOif5vWG7p7PsWYFjBG?=
 =?us-ascii?Q?2OCr5un6W/WWGhih3Qpcwu0u+iXb+HdgPgh6JtoAA/hZsNASOD3NhxZh8W2I?=
 =?us-ascii?Q?45lQUrt+wZa1o+3qiIH7H3B0jxecherdFOnQ/duFtvc8EtNS7apRy8t3+6xy?=
 =?us-ascii?Q?8ZGf+eCGHL2Np6GMmzKQiqamHhhkKxngLU7mR4+B71PTb+KPQ8HtPw2MSv1m?=
 =?us-ascii?Q?nCcwWKHrxRtOy0szN3u6jPQP7GsNuqSb+Rb6QDZCOWwDPrGrlWz/558dq334?=
 =?us-ascii?Q?OXMhwZpZTfS6VZa95dZNzR4wc7eQmFKkFnt94eN4SNe1iszuOU8RhalIDd7j?=
 =?us-ascii?Q?3eYjO13nVqsq88r1SLe0VHWEAiSy57Mn3wZAP2Z4he2PyzgSHmkfKwrEd7TI?=
 =?us-ascii?Q?amSl/0oQe7vN8+28JpHjPa362pfDIMIM1j7WDmaICa3RMK2rphAkJNQKYVnd?=
 =?us-ascii?Q?49+sn2Rlh6Li4h6Sr6nKHhL5dcjIXtExio3BpEV4qcFFi1jz76fNbOooPoGS?=
 =?us-ascii?Q?l0ePahP8BvgEA8jnJ4sL/Twnf9FjbL++K8gW078tVtANk0iRuLDWsCwBeojw?=
 =?us-ascii?Q?sZNYtQcqkPsc8EiXNBVbWdXJp/bilEhJh4XWAamhUS9x+8+k4uVorACWteTo?=
 =?us-ascii?Q?swGoYLh08kKzMOcbd9ljTc9DeYgBGFEAnsAZJNtwM9Ds3PkeCDWIXQqhgeJL?=
 =?us-ascii?Q?skiIHsIikqqlpy+gBVZh9ejAzcfP1/7wgsCZqsxUVYJ5uv57NbPQVDofsBdz?=
 =?us-ascii?Q?EXtPqvvEwM8k+GB7efMSk4VRQp7h9hi2ebMpHBjFiB2jmbFjpFN1Dp3gCyZW?=
 =?us-ascii?Q?XEm7VDzi75KVT+0WZzc2wFDtMdFZTP80MdMW3feQR6luUl5wPD54rgfJCUng?=
 =?us-ascii?Q?CFWYrKc8z4lzKPHt0DnILxzlXOlyc6jzkZDcksEVwOoUAr1ZZlPdJsbvvTL5?=
 =?us-ascii?Q?TrdkjHJQyUMdp8wKFeGuwZ/pU312TQF7PxEBJXp1mJsuKggbj9Wxc/+JN3rU?=
 =?us-ascii?Q?D6goKqdGvF1ECUDG3zZr6IyUMqA1ORSR2Q+nzsYNeYsP77SWGOtZFrHgZM1O?=
 =?us-ascii?Q?+15zdOTyXH/QigpKfkude1bb94ddzb7tZTQtbfXyAcUckpnsnJSdcEZTkkFX?=
 =?us-ascii?Q?dMSJ4XRLYvwlYg9qYjRUz7ZhJWi+Th1bPY4w4M3rjVSYcvn6/TIdnHbmuyhO?=
 =?us-ascii?Q?nz4q0I+PUF2GGpswVQhRRDhtnkpsZ0ABF0Vg8u8ZE4hVdsVx3uYBK0wYCdUB?=
 =?us-ascii?Q?c/clWL2hryRindCgi1CFG+AXn40RDVsIYTW9kPSysioY3HW9hilJyvkidXeW?=
 =?us-ascii?Q?LmP0b5VKdNr+h+hhyE0CJieAWEYfAirJ0r8b1yvYJONwG/vzqg7hUU0zr6PM?=
 =?us-ascii?Q?tKS+KJlKrdhkOAMKXEh0eRSNzf0M40qoGdbeK1b5mq9fuxiBLao+PRQ+QoaH?=
 =?us-ascii?Q?/vNBucSXQpTT5dcLzDTKmYQhykTtrB6SU6QMZRJf0lwDFyHZrpT3J6Bbe9eW?=
 =?us-ascii?Q?sA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	/4M7pE4hKYC0Ag9BK63Thy9la8CEQde0c4N40RuOEn8OsJdtXHQ9CgugCf08VMl3rjCUrsExEwfLDzivoNQR9YF7HPVzlq09TcJY1ka2GYe9/CjMSJhwktaFkZIm/ESCtwLoO6TnSooE4KHJ4wA5vG4IuJ9bP4tJ2guYG4t2V60wgfsVhrbAGKkVmstFJ9jUba3gggfhVHpsSXkr23DkOxJS+XSiasqmSpKdzaPPydTmmY4XHeJTFKH+BWE8No8BxAHyDK2HCt8Bx7rh1P0qG4yGi+S+ixL401/+nAlC1CBmKKp3p0CfYZ90G31KUUYD47slnpu3VT8fWo9jfVi+W+GLylw8XMpgVm3fZSZNZA7DClAnRTv3CjSGU2RV5+kCOZ9sToIpbcUYVx3/QVk7170doJemeEWKJkQxyj/CNPjBhXlLE8YGSf/jOI3DtALV+1r7I5+F1GMyaynPFVF34E91BgXKTOHn0Wh08JaUbhOR94ExQjRtli+YePcbH29Pu2bYFVxPLS2NZcCfrn+gzF/ByLrNkoMQOrdOLXEJ85aA4298GRiX3dbPUsGG5i4TRn/Xt6XsZF95eD/h2NEJJNebuYTe5kIWs+WVYdPr01Y=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fd45e517-3473-4807-85f5-08dc2271cd38
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 15:32:15.1134
 (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: 7eD9bkDdp0Y3q9TBxy1xdQU3iJuxIHryjV9Qge7BY6ZcbYs5/6xcPO6aEDIIreAZ8pAwgJc92pKpUKWgaxQeLg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5163
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-31_08,2024-01-31_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=828 adultscore=0
 bulkscore=0 malwarescore=0 suspectscore=0 phishscore=0 spamscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401310119
X-Proofpoint-ORIG-GUID: LaM7NIsUVrLrLjy6rhA7uHQ18JOcJgYm
X-Proofpoint-GUID: LaM7NIsUVrLrLjy6rhA7uHQ18JOcJgYm

On Wed, Jan 31, 2024 at 11:17:40AM +1100, NeilBrown wrote:
> 
> It is possible for free_blocked_lock() to be called twice concurrently,
> once from nfsd4_lock() and once from nfsd4_release_lockowner() calling
> remove_blocked_locks().  This is why a kref was added.
> 
> It is perfectly safe for locks_delete_block() and kref_put() to be
> called in parallel as they use locking or atomicity respectively as
> protection.  However locks_release_private() has no locking.  It is
> safe for it to be called twice sequentially, but not concurrently.
> 
> This patch moves that call from free_blocked_lock() where it could race
> with itself, to free_nbl() where it cannot.  This will slightly delay
> the freeing of private info or release of the owner - but not by much.
> It is arguably more natural for this freeing to happen in free_nbl()
> where the structure itself is freed.
> 
> This bug was found by code inspection - it has not been seen in practice.
> 
> Fixes: 47446d74f170 ("nfsd4: add refcount for nfsd4_blocked_lock")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a66d66b9f769..12534e12dbb3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -318,6 +318,7 @@ free_nbl(struct kref *kref)
>  	struct nfsd4_blocked_lock *nbl;
>  
>  	nbl = container_of(kref, struct nfsd4_blocked_lock, nbl_kref);
> +	locks_release_private(&nbl->nbl_lock);
>  	kfree(nbl);
>  }
>  
> @@ -325,7 +326,6 @@ static void
>  free_blocked_lock(struct nfsd4_blocked_lock *nbl)
>  {
>  	locks_delete_block(&nbl->nbl_lock);
> -	locks_release_private(&nbl->nbl_lock);
>  	kref_put(&nbl->nbl_kref, free_nbl);
>  }
>  
> -- 
> 2.43.0

Applied to nfsd-next (for v6.9).


-- 
Chuck Lever