Received-SPF: pass (google.com: domain of linux-nfs+bounces-786-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <620255dc-f16b-400f-bc53-cb7298d1d0e9@oracle.com>
Date: Sat, 23 Dec 2023 10:07:30 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] nfsd: drop st_mutex and rp_mutex before calling
 move_to_close_lru()
Content-Language: en-US
To: NeilBrown <neilb@suse.de>
Cc: Chuck Lever <chuck.lever@oracle.com>, Jeff Layton <jlayton@kernel.org>,
        "J. Bruce Fields" <bfields@fieldses.org>,
        Olga Kornievskaia <kolga@netapp.com>, Tom Talpey <tom@talpey.com>,
        linux-nfs@vger.kernel.org
References: <170320926037.11005.9834662167645370066@noble.neil.brown.name>
 <170321113026.11005.17173312563294650530@noble.neil.brown.name>
 <eab63c76-6425-40c9-a078-ab6c4bdf10f8@oracle.com>
 <170328606817.11005.12343520715219354379@noble.neil.brown.name>
From: dai.ngo@oracle.com
In-Reply-To: <170328606817.11005.12343520715219354379@noble.neil.brown.name>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?TmF5SDNiT3FpamtKbXovL0paMHhPdGQzQ2pqNWc2MkQ5bDRGSkZjQVNIL0dN?=
 =?utf-8?B?Q0V5YWhmVTZrRlMwSjRvNkdPRC9tbk8xV01OalVBMStKT3dWcW94ZTJIT0sw?=
 =?utf-8?B?Z1ZXeWVZK2JURHl2RENXb3hHRlh0dlJNcU5mOEd2SGtBV3VoWWIwU3J6Njcx?=
 =?utf-8?B?YndGN0puYnFEeGhPWjhZV3llOG1QQkRoRUtZN3cwSjcvSHdvdkROaVVhSHJ5?=
 =?utf-8?B?RDdna2dPM1pxNkNhSDJuMUxvalg4N21kcTY4NlRQcE9PRVhWS0Z1S2lYWTVK?=
 =?utf-8?B?amwzRXhCZXh4bTNEaGlWTlI5NzJUc1gxdWVwUzZsUUFYSEd2OVBaK3dBaitY?=
 =?utf-8?B?MzJKaC80Wm5OMENnU1ZqYnFZMmNJbG9Zcy9KTG8ybEhPVlpPYVBMMEdwOEt6?=
 =?utf-8?B?VUV2WkVxUkw2aHRjUXJGTVczYnFoMVdMMEVMNEdSWjRTUG9GVUdPaEtZTGIr?=
 =?utf-8?B?YmxoTVdSUEFHempoU0liNGRrZ1N3WVhWK3p3NUdiMWpMd0psOFgwOGJQYkdU?=
 =?utf-8?B?VHk1aWhzK1NKZG0wbnNKcmJqWHNFdzdCZ3F4Zk9MREk5Tk5QNzk0Yys4cEI4?=
 =?utf-8?B?cTJneGM2YVVKU3FwNExwTDVJNWNSdVdMZW50eWpNRndTcmtDVTRqNEhjWHVS?=
 =?utf-8?B?ak9iK3lacFJMWmRObVJMTEg5VUtNdlBYK2srSEpjYXloanplcE8zMVNZcjhU?=
 =?utf-8?B?V2RHRXVlckdLRFR0NUpUbCtKeUVTekNOWlo1RC9VUlpEbVlMRmFpZHorNnNC?=
 =?utf-8?B?Si9aZWlDVXljNWZpYVZJTVR5b2FBUFlZS04zZm5LTHg0TU1JVGJuRUV4RHlZ?=
 =?utf-8?B?R0IvV3RHYkNmTXhNcWMvTXlwREVRQWU1ZWt5aTA1bEJaaVVmTGh3TWR3RGpu?=
 =?utf-8?B?dnA4dTRFVHJtRWx0RjdkM0RoRmE0OWUrcHlicDR0WXZXV1RHbDMzb09BZXRN?=
 =?utf-8?B?R1VnTmQxd1pac05FcUEwclV3eFJLaUJxWVRZUnZWY2NVb2RIaStxNTd5UXhs?=
 =?utf-8?B?WTNETlduRldMdVNyYTE3K1g1OTVlb3h0N283TmdrbitaRFdKUEluK1JVVHlq?=
 =?utf-8?B?bWZOa1QxSGdUcnl5cmtoSnZmL1FGdDlERU9mRDV3N0MrSTkzUEJxVnZBeHFH?=
 =?utf-8?B?V2l3V0FHS2YvczA5ZHJMMnR0SXZOMFhtTHFDSVVuSEFlMWc3TVp5SkFON1BQ?=
 =?utf-8?B?N3Iwa0RkdWRoWitjSWNYcFpVYlRRVGp0OVRRSDVFZXh6U3Y2YzVwNExDZlJk?=
 =?utf-8?B?ZTFFZ2dzaldiNXI2N0ZCdzRwVHZGOWRhMWl6MjdxNkxNOHV4dUZldzA2cGtv?=
 =?utf-8?B?MFhHMGUvaGFVMUNvQWFmNmVVYWlZYytvdzVSSWlUMEw5SkJSK2dPcUs5QU9Q?=
 =?utf-8?B?ajYyL0xnWGo2UFpIZ3pwcXJ1Y1FOYnc0bGZ5akFBMlZvK0gwUGFIWHluZTVj?=
 =?utf-8?B?bjBJcVlSVnY5U2RwMFJUS2tPWUFwNGRDUEhUdThvd0doNTVMKzBmZk4reTlU?=
 =?utf-8?B?WDEwYk5UemIyR1c2MVZVUmlFT212d1ZqZCtpTk1UVy9iZmg2VEJ5T1JxNXZW?=
 =?utf-8?B?SE90RVlpck9CRkFubmxyOHRJQzdpQnJ4UTJiQzUzdFU4YmtzbWZockZyZVhL?=
 =?utf-8?B?VUlIV2d0SWNZeUtlK3pSSHBDZWZFWEpqRjBQNENZbm82aUwzWnNhQzVHdUxH?=
 =?utf-8?B?VHdsTGVPb200VnRHTnpKdEVDdFl0SmtDUHJNQm8wVTlGSFNmMnRCWENTdlZW?=
 =?utf-8?B?MGVLbTVyT0hMVWJ5ZVJFdUwxZ2NrMks2aDcyWmJmaVZoQWtGMzNxT0Rzc0NJ?=
 =?utf-8?B?d3lWK3RyTUFvMUlGZjlXZ2xGLzZwa0RoTUhrVnZCOFJZaGc4c3NNY1VyclJZ?=
 =?utf-8?B?MUI1YXNtUlhFcDVPVU5RVExDM2Jrc2YxZ3ArRkZIMlBNTG41WFowb1V0WDE5?=
 =?utf-8?B?R0hiZGJkdVZDcGxScFMzMTFFdHFqcHEyUGV1Kzl6dHRuak9QcThXRFY5bWJj?=
 =?utf-8?B?dlVSODBGMmJ1TUVZTndoV2JmRi8weGFTalNaTEZkOXNOa0U5S2lEM2Fha0Jy?=
 =?utf-8?B?cHJZdzdVcWVTc0ZQaW43dkhYckJ5Tjl4Z1gzR2I0NDRMUXNJSWNzMFR0MmtV?=
 =?utf-8?Q?obGOt6sfpk253dwxG+CfUB/8o?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	yORx0vqJUXmztoiqLWOCHwNdzZme8gC6us/xe+ez709R4BK3ioNpg6IRjyPiYIUkHdgwt+igucni8MoIW6r8xmntu1UlRox8N0TprJsVYDZDBFxgB4C5BK6iKnCnJN9Mcta3t2wiRMCUP634Gsv6J37V2nMpFeVlq9lruP+TDrL8isQgRBY97xukekmdzDo6+RHRTH7jrdskrYV2+6r4DvE+4nqsKvstXQcoP1gn0XhD1pktYZAZ6BJKkZPCwYNkI3Z3kKfvm7UkEsTfhl8EU/0P38oCuSmcbeP9btyHQQFxyxtOPy6HVnSoP2OeRRljTWCa2ohjJE7fqdoZvrHyZ1Z/WUlAEyw7E3yQP7TaTvRoEOtZw6VIK5iqjzgPvDCsvNKeIHKNdGo28pevFhcO2MOgt4Y1WYTPYZq7wsWnIVMv3VQra2wuGXGTZKVE/N3fYQPgcRKAdhFK4WNC6Jk+8ez6Fy+4UGYxF1qfVswbK/u/IzDMZwFXhaCFswHzjl0xbaZ86AzBmB0EN+qHtzQS93h81W2qOsbXYSKt9GGdY55eQzgKkcznKIkWFmV5XtxanrGIeHyk63VIFTVI/T4Tj42qCZl14d/OK39/qsHePLQ=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c1d51486-14e4-4b15-77c3-08dc03e20a5c
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Dec 2023 18:07:35.2890
 (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: db3FASX8QUXz6I3HaQ5Z4xDM5f7BFj2JF1oBA7XrA1gYqI7lRRGxf8wMMbCtqKMJdVCx7JTiiJtF8XoxPfxVhA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4164
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-23_08,2023-12-22_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 bulkscore=0
 suspectscore=0 adultscore=0 malwarescore=0 spamscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312230143
X-Proofpoint-GUID: IJc213dkptzxPk5JuvvAgIUvSwAYp4bv
X-Proofpoint-ORIG-GUID: IJc213dkptzxPk5JuvvAgIUvSwAYp4bv


On 12/22/23 3:01 PM, NeilBrown wrote:
> On Sat, 23 Dec 2023, dai.ngo@oracle.com wrote:
>> On 12/21/23 6:12 PM, NeilBrown wrote:
>>> move_to_close_lru() is currently called with ->st_mutex and .rp_mutex held.
>>> This can lead to a deadlock as move_to_close_lru() waits for sc_count to
>>> drop to 2, and some threads holding a reference might be waiting for either
>>> mutex.  These references will never be dropped so sc_count will never
>>> reach 2.
>> Yes, I think there is potential deadlock here since both nfs4_preprocess_seqid_op
>> and nfsd4_find_and_lock_existing_open can increment the sc_count but then
>> have to wait for the st_mutex which being held by move_to_close_lru.
>>
>>> There can be no harm in dropping ->st_mutex to before
>>> move_to_close_lru() because the only place that takes the mutex is
>>> nfsd4_lock_ol_stateid(), and it quickly aborts if sc_type is
>>> NFS4_CLOSED_STID, which it will be before move_to_close_lru() is called.
>>>
>>> Similarly dropping .rp_mutex is safe after the state is closed and so
>>> no longer usable.  Another way to look at this is that nothing
>>> significant happens between when nfsd4_close() now calls
>>> nfsd4_cstate_clear_replay(), and where nfsd4_proc_compound calls
>>> nfsd4_cstate_clear_replay() a little later.
>>>
>>> See also
>>>    https://lore.kernel.org/lkml/4dd1fe21e11344e5969bb112e954affb@jd.com/T/
>>> where this problem was raised but not successfully resolved.
>>>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>> ---
>>>
>>> Sorry - I posted v1 a little hastily.  I need to drop rp_mutex as well
>>> to avoid the deadlock.  This also is safe.
>>>
>>>    fs/nfsd/nfs4state.c | 12 ++++++++----
>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 40415929e2ae..453714fbcd66 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -7055,7 +7055,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
>>>    	return status;
>>>    }
>>>    
>>> -static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>> +static bool nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>>    {
>>>    	struct nfs4_client *clp = s->st_stid.sc_client;
>>>    	bool unhashed;
>>> @@ -7072,11 +7072,11 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>>    		list_for_each_entry(stp, &reaplist, st_locks)
>>>    			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
>>>    		free_ol_stateid_reaplist(&reaplist);
>>> +		return false;
>>>    	} else {
>>>    		spin_unlock(&clp->cl_lock);
>>>    		free_ol_stateid_reaplist(&reaplist);
>>> -		if (unhashed)
>>> -			move_to_close_lru(s, clp->net);
>>> +		return unhashed;
>>>    	}
>>>    }
>>>    
>>> @@ -7092,6 +7092,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>    	struct nfs4_ol_stateid *stp;
>>>    	struct net *net = SVC_NET(rqstp);
>>>    	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>> +	bool need_move_to_close_list;
>>>    
>>>    	dprintk("NFSD: nfsd4_close on file %pd\n",
>>>    			cstate->current_fh.fh_dentry);
>>> @@ -7114,8 +7115,11 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>    	 */
>>>    	nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
>>>    
>>> -	nfsd4_close_open_stateid(stp);
>>> +	need_move_to_close_list = nfsd4_close_open_stateid(stp);
>>>    	mutex_unlock(&stp->st_mutex);
>>> +	nfsd4_cstate_clear_replay(cstate);
>> should nfsd4_cstate_clear_replay be called only if need_move_to_close_list
>> is true?
> It certain could be done like that.
>
>     if (need_move_to_close_list) {
>           nfsd4_cstate_clear_replay(cstate);
>           move_to_close_lru(stp, net);
>     }
>
> It would make almost no behavioural difference as
> need_to_move_close_list is never true for v4.1 and later and almost
> always true for v4.0, and nfsd4_cstate_clear_replay() does nothing for
> v4.1 and later.
> The only time behaviour would interrestingly different is when
> nfsd4_close_open_stateid() found the state was already unlocked.  Then
> need_move_to_close_list would be false, but nfsd4_cstate_clear_replay()
> wouldn't be a no-op.
>
> I thought the code was a little simpler the way I wrote it.  We don't
> need the need_move_to_close_list guard on nfsd4_cstate_clear_replay(),
> so I left it unguarded.
> But I'm happy to change it if you can give a good reason - or even if
> you just think it is clearer the other way.

My thinking is that if move_to_close_lru is not called then why bother
to do nfsd4_cstate_clear_replay. It's just easier to understand and
safer (against future changes) than having to go through all possible
scenarios to make sure it's safe to call nfsd4_cstate_clear_replay
regardless.

-Dai

>
> Thanks,
> NeilBrown
>
>> -Dai
>>
>>> +	if (need_move_to_close_list)
>>> +		move_to_close_lru(stp, net);
>>>    
>>>    	/* v4.1+ suggests that we send a special stateid in here, since the
>>>    	 * clients should just ignore this anyway. Since this is not useful