Received-SPF: pass (google.com: domain of linux-nfs+bounces-2194-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 4 Mar 2024 17:54:03 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: NeilBrown <neilb@suse.de>
Cc: Jeff Layton <jlayton@kernel.org>, linux-nfs@vger.kernel.org,
        Olga Kornievskaia <kolga@netapp.com>, Dai Ngo <Dai.Ngo@oracle.com>,
        Tom Talpey <tom@talpey.com>
Subject: Re: [PATCH 3/4] nfsd: replace rp_mutex to avoid deadlock in
 move_to_close_lru()
Message-ID: <ZeZRC21DdOkuKroo@manet.1015granger.net>
References: <20240304044304.3657-1-neilb@suse.de>
 <20240304044304.3657-4-neilb@suse.de>
 <ZeXWGJqreYH8aayB@klimt.1015granger.net>
 <170958874536.24797.2684794071853900422@noble.neil.brown.name>
 <ZeZFGdOD3KWkF1Zf@manet.1015granger.net>
 <170959178935.24797.7531672348129457687@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <170959178935.24797.7531672348129457687@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?9yIvVV0KDdnbRt9BMkNJSd7xbNwxdRBBhVm5YXF5dZUMrzOXaDrAnjQENqRj?=
 =?us-ascii?Q?d/QmqTF6tsdVRQ4WB4ZeuM/aGXVsZGCclyvOsTThLeu1Pq1ylPCDWGF6OZRI?=
 =?us-ascii?Q?wXb4+nHIq++N+/YjUKr3Ph/W+X9KRTbunRI6HOX5j3jluNHGn2X4wdSVXb5J?=
 =?us-ascii?Q?tAL4aGwQg2d9fDgLsguRhr8x0h8dq+W3ZBqTewhbo9Hn5NUeWNfuqD2ZstFD?=
 =?us-ascii?Q?a6AxB/FICWR+FCsg7wQVWvfV1m1/9kqmwXiNMs6ic0EIBOss6fsm7EO6ctlK?=
 =?us-ascii?Q?BltVk6DfwZfk/h+3ZxpoLSWs27emOxOu3n8nqdw5bY2RglycCuH4l0OIjh25?=
 =?us-ascii?Q?1NJcQMhsRmltXZFc8lTHqKdpnt65TVuyfgOvVoUrY5O3eXGqKFYd9LUd3UwC?=
 =?us-ascii?Q?sunxInOdfZRDGBLnENsPzLGniI094GtSXfkai378F0zSZV1AXA3E5zufAX+7?=
 =?us-ascii?Q?1/vk6QwHDUBeHPBfMA4JAtxx8Wg04R4mVyIdB6vV5z79aNKZ1eXZUtaFJ0GM?=
 =?us-ascii?Q?DoKMUdmSCSlJSo0ivfgx9NQyZp8faEGWXl6/5BetrHGZlwS2enphybGvMf83?=
 =?us-ascii?Q?j7sGhhbq85Kh+IYFmNQwmGUmxmtf7CrlEexWvYUvHBrLf7ojjwP7PZ67E3Ka?=
 =?us-ascii?Q?LQdh+28pqLoOCJiBuIHlaSo5OqxtwsKEkYA0b0ObKkLGPnQL5cKVpNHEI8Pk?=
 =?us-ascii?Q?QySVlIkPHsJtOpCXSFrkMfAQU/Q3Bck7tyybd65b6sLB5FFJoA8MwhMjs7F2?=
 =?us-ascii?Q?VlHQr13Ct14yHfiZAqF85ghQbHVPNCAlPbRZCba5gjjiRhp8cuqBurJfy+fE?=
 =?us-ascii?Q?QX22HYJAJaXHWfVgvOw2tnqV2NX+qzWgmMdTHsQNIBuz8l77PA0zQfE2zVj5?=
 =?us-ascii?Q?1shUFKD2s4TO36GWgDY6E9Zhq54JcxfTshdAyPedpm6hjnvbrt7JP8nVh98m?=
 =?us-ascii?Q?/R4rxKK8ZLzuDw5eT3oqUTB5xZjgE0JSdGOFoTrB0IHRlK39+w8SZQW+1l9x?=
 =?us-ascii?Q?+fMDWaIpxiXdpKdEeDsZGYDXPXHcOwvoFqAAWjFO/UYAEOFfgpwwM4x5t1rS?=
 =?us-ascii?Q?EeZbeKWhC2sUf+ZLd7CZXdq4HjgTBt2rbClZYyAS7lUhIlmWaVVai36MYPks?=
 =?us-ascii?Q?OIKi1avNXO8a+U3g3/3Pco3okXcT0Nnh4Wq1BVSAHDWJ9ltX0PLBNI4X35uJ?=
 =?us-ascii?Q?q6C0+/bfcupDy4IkDW22EYj0QqjpI1ScIiN2aSflWqO0qTxFC9WXAqAjoU5k?=
 =?us-ascii?Q?KPlWiXRl8flSoNapM16dbL8+qxW23ks07V9kM95h37W3+jeb5RLm8zvCBDvz?=
 =?us-ascii?Q?yLwhnqq3bO068VdZBtIx382uybvv8V0z7e3OCDXJKCJM2Iwp9ovtTAFgjRXa?=
 =?us-ascii?Q?vuC2c7isSwjIERjYSoV0UZgh8kMASP/f9H9pQH1tLQeHp67nx1j5EpJlk5V2?=
 =?us-ascii?Q?PxT+YuFWN1Emnkw6+j4j6aVTemJBHaGqzJKzKby0euipd9P5MTirJwjTk2MK?=
 =?us-ascii?Q?5rwN62tf8UzIm3Y5x+11jvR2pOmwlzjcPvZM30gwqp2Ky0cZ3sva7qqgBA/p?=
 =?us-ascii?Q?KwYS7XDpGF4tF+lGuT2u1HPdFdQDhQOh2Zp8FMo6irF6lnF2+PIFN3a538un?=
 =?us-ascii?Q?aA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	5T22VSWaXIA3/IlWTyKCZxpxb5YbBIhisZZw/0JhSZnvXc8cUkvqC1rjVyX3waKPdkL109hAD1qjZLQ6LtsfrFvOmr1OuMlOem07w57cGndhc2k+S1SmRIYeZfzDEKYdEfKsoqGfPRxIWMARSrVWoJDOjEM94xcRD9x+bzMDLx8LqrCkKbzBTnfDDeQhsb99ctDvUMUY14WoVNnhqULXx5QqDYRY7YpUKXWiISegLFDbjvgvJfaVhBwBALyMLHy5aX3UZACSA6r/GkN4jKRvPdaL9UTpNCi7+/6LT90R5yTv7q7iT2zMJGOIbMrk9AZSFpP472k5YHCtR2louEmTjx40XaWolDqpYW1IGOFM9YVmROWgr5SHrW4ltojW1ZOGoAr2veHgmaZrhAxlURXdkwD8UYHLyHgxxK8shJxYPW+LOopnD9zbGNNeb8hYC9buHHogNaxcRox+OfpRrwLGx1d8wFmaRImHk6bIejjZfPltRFd8zjKjNnyx8b0l6Tsmn0FC0qFzWWPVNxx/1BimqD2CbgqhPzapEBd8JFFr/dWFh99YpI9TloRNGy6Jk3nFHqAQAHy/q9yTDe8BD25323lNzWHll3r8UVIWMNeC7cs=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5988f3cd-6f24-4848-393e-08dc3c9e02d9
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2024 22:54:13.0998
 (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: rMJCXF6YbqwIW74yyDEiI0ptnRhNnm/E4p/y8Qb3mzSlsOqNlzM3pc47syt/L3vd5QHO+7PrU9AXWDcpHeppdQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5176
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-03-04_18,2024-03-04_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999
 spamscore=0 phishscore=0 mlxscore=0 malwarescore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2403040178
X-Proofpoint-GUID: R97WrVpHQ6mZY6Z28eFpyK1_-v_uowJd
X-Proofpoint-ORIG-GUID: R97WrVpHQ6mZY6Z28eFpyK1_-v_uowJd

On Tue, Mar 05, 2024 at 09:36:29AM +1100, NeilBrown wrote:
> On Tue, 05 Mar 2024, Chuck Lever wrote:
> > On Tue, Mar 05, 2024 at 08:45:45AM +1100, NeilBrown wrote:
> > > On Tue, 05 Mar 2024, Chuck Lever wrote:
> > > > On Mon, Mar 04, 2024 at 03:40:21PM +1100, NeilBrown wrote:
> > > > > move_to_close_lru() waits for sc_count to become zero while holding
> > > > > rp_mutex.  This can deadlock if another thread holds a reference and is
> > > > > waiting for rp_mutex.
> > > > > 
> > > > > By the time we get to move_to_close_lru() the openowner is unhashed and
> > > > > cannot be found any more.  So code waiting for the mutex can safely
> > > > > retry the lookup if move_to_close_lru() has started.
> > > > > 
> > > > > So change rp_mutex to an atomic_t with three states:
> > > > > 
> > > > >  RP_UNLOCK   - state is still hashed, not locked for reply
> > > > >  RP_LOCKED   - state is still hashed, is locked for reply
> > > > >  RP_UNHASHED - state is not hashed, no code can get a lock.
> > > > > 
> > > > > Use wait_var_event() to wait for either a lock, or for the owner to be
> > > > > unhashed.  In the latter case, retry the lookup.
> > > > > 
> > > > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > > > ---
> > > > >  fs/nfsd/nfs4state.c | 38 +++++++++++++++++++++++++++++++-------
> > > > >  fs/nfsd/state.h     |  2 +-
> > > > >  2 files changed, 32 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > index 690d0e697320..47e879d5d68a 100644
> > > > > --- a/fs/nfsd/nfs4state.c
> > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > @@ -4430,21 +4430,32 @@ nfsd4_init_leases_net(struct nfsd_net *nn)
> > > > >  	atomic_set(&nn->nfsd_courtesy_clients, 0);
> > > > >  }
> > > > >  
> > > > > +enum rp_lock {
> > > > > +	RP_UNLOCKED,
> > > > > +	RP_LOCKED,
> > > > > +	RP_UNHASHED,
> > > > > +};
> > > > > +
> > > > >  static void init_nfs4_replay(struct nfs4_replay *rp)
> > > > >  {
> > > > >  	rp->rp_status = nfserr_serverfault;
> > > > >  	rp->rp_buflen = 0;
> > > > >  	rp->rp_buf = rp->rp_ibuf;
> > > > > -	mutex_init(&rp->rp_mutex);
> > > > > +	atomic_set(&rp->rp_locked, RP_UNLOCKED);
> > > > >  }
> > > > >  
> > > > > -static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> > > > > -		struct nfs4_stateowner *so)
> > > > > +static int nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> > > > > +				      struct nfs4_stateowner *so)
> > > > >  {
> > > > >  	if (!nfsd4_has_session(cstate)) {
> > > > > -		mutex_lock(&so->so_replay.rp_mutex);
> > > > > +		wait_var_event(&so->so_replay.rp_locked,
> > > > > +			       atomic_cmpxchg(&so->so_replay.rp_locked,
> > > > > +					      RP_UNLOCKED, RP_LOCKED) != RP_LOCKED);
> > > > 
> > > > What reliably prevents this from being a "wait forever" ?
> > > 
> > > That same thing that reliably prevented the original mutex_lock from
> > > waiting forever.
> > > It waits until rp_locked is set to RP_UNLOCKED, which is precisely when
> > > we previously called mutex_unlock.  But it *also* aborts the wait if
> > > rp_locked is set to RP_UNHASHED - so it is now more reliable.
> > > 
> > > Does that answer the question?
> > 
> > Hm. I guess then we are no worse off with wait_var_event().
> > 
> > I'm not as familiar with this logic as perhaps I should be. How long
> > does it take for the wake-up to occur, typically?
> 
> wait_var_event() is paired with wake_up_var().
> The wake up happens when wake_up_var() is called, which in this code is
> always immediately after atomic_set() updates the variable.

I'm trying to ascertain the actual wall-clock time that the nfsd thread
is sleeping, at most. Is this going to be a possible DoS vector? Can
it impact the ability for the server to shut down without hanging?


-- 
Chuck Lever