Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Sun, 19 Nov 2023 18:38:48 -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 1/9] nfsd: hold ->cl_lock for hash_delegation_locked()
Message-ID: <ZVqciNeBnfAaHZ6a@tissot.1015granger.net>
References: <20231117022121.23310-1-neilb@suse.de>
 <20231117022121.23310-2-neilb@suse.de>
 <40e1bf417c635ce303f9e42ddb0e3dbd90022477.camel@kernel.org>
 <170042987584.19300.7721851585544522693@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <170042987584.19300.7721851585544522693@noble.neil.brown.name>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Wt60rVysZ955iBCeP2nlzfGKTtsjVtgHXtkkoQigs1KNAKeiBLXa2KiXoeyc?=
 =?us-ascii?Q?CSy5GDiBzCElMAlRyvZezFYqUNNKGeU+G3ORuM9VmxGn+GDEqn9Ysi8gWmLG?=
 =?us-ascii?Q?ygH+hVKXCqDgy1yqijd+rU8Myh9ChU2sDFUt2wjctM4NXvVAfNsUuXuEvtHm?=
 =?us-ascii?Q?DoFqtaM5mw9Ox/WApQpJeGPGjFoPOHX6Gf/9XcPSv7gCu4FHUMYp07oB0jpr?=
 =?us-ascii?Q?Iy9qKkCtmdjcKJJPAtyiLMhn+BE695sIBDv+iifG1J9AUj2bschq9u3B3HUp?=
 =?us-ascii?Q?vwu/z8Kw2HA8zXnzVoEReMXAKsyA2HxDhvhMFoK20m6WLZg+/jcLg47qkeBT?=
 =?us-ascii?Q?eu6U9zTlQpxc88w3lharYnz2ZroiL+9N1jMLfq8wcJLjyV9oWJczwghpshYX?=
 =?us-ascii?Q?86tSeD8RDg1+huajgWHE5LrIs8tPH9Tz+cASLse3MdymY2XvWL5/WuD9tHdj?=
 =?us-ascii?Q?bjlLiOwyn36L50j91TckDDjGCb1nuH5DCZTmoqAaJ2vZm32x/BN7uW/fWgq4?=
 =?us-ascii?Q?xmXjhVlvea6DFfZTm2lWxFQi7nbXmCNpH5BNTdDdimR6M1DC7bRWSsvR4oHw?=
 =?us-ascii?Q?X1rOUJUoMEsndPWu0SFx9KKezlK7SsY7INtPr4KQGkH69+KA96zKsoA4Drxz?=
 =?us-ascii?Q?zc4n+in050MZe6NtOO+yWqueWhYMS1KN09lSRKE3VT0i9Xp7RNfIW1DOvqcL?=
 =?us-ascii?Q?NJF1f4jvgCxHnXkTozVPAbjvNk/UUvaW4U4AkkSm3bA7/iQMl8o93MiZvEPR?=
 =?us-ascii?Q?xxISS+fnn7EeNAKfHfUQ7qiRFqYQhL7Ekc1UXiqlshxMuPnPVFBxP1/GQD3C?=
 =?us-ascii?Q?IGWLkC9E61KStyCFayGoT3ITEFF3j5PcH6T7aK/llo94E0eJqxpvEK558sJk?=
 =?us-ascii?Q?hjrr9TXAflXcUiTaFybuDOrl5B1fjJA+i8ha44vUEoWgHimYz+TOfnQxZWA8?=
 =?us-ascii?Q?7dMaXFTedBnxjC13kR9q+BBgWXLeFUxW3zsurzBLZj/f/kKJuybFEKTp+LDn?=
 =?us-ascii?Q?9yVLtdE0iTw85ao9UgvH9BvLl7PCcyGDj7AlAgnwopfZKzcCtc9dVI0ond0u?=
 =?us-ascii?Q?0mpn4ZpIKAPqmPYgfKNfHK5/i+qcokB24KIifbjXn6kkgLkJFWL9JNDG/Zrd?=
 =?us-ascii?Q?pccw9joSf4oEe92Ty39awu1mq3zwCFuJWu3w8yADZEKxU/m9RTXYEErzJaRW?=
 =?us-ascii?Q?Vp6vJsmmzgBS/xQWqnwNcJvbZN8wQaCKYxXtlHN7YQ4BfzTZ+46OOWlNOmAo?=
 =?us-ascii?Q?NJHcbxsgDbO2c1Ru6GvjSMzWds5Xue7MYZP/5VcbXunGNkW1v5w1xDz3DOMP?=
 =?us-ascii?Q?U1qkuWTFmEZGix28of9BcuZtAuDSuBsnB4jDLgd/C3lSczkgSjL3Sj2W1VEu?=
 =?us-ascii?Q?0xGbTnue/Vh8Vhy1F9FiGFUbhKklgiGtPJaQtOzaE+OvBFslqwELSTZNHmD+?=
 =?us-ascii?Q?qoP03V1HrGHNB3tffRKBSkChUnZVVGvo+O37RlLFRmm5Jr5ZWrvYwcOyhxuG?=
 =?us-ascii?Q?B6U3KHxzpGY6eY8MMK17CFbtK9eeYf5V56QgQSpM/sQ8Zq6npHDck8knBqBE?=
 =?us-ascii?Q?xln/SOQxq8HL19aJswNJU3TGBXNQID9PxbA8ERJjg116RmWpnyjaYU5uObCt?=
 =?us-ascii?Q?uw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 3lQjykj2h6Ed0gDVsJxCQEkFYuccD7B38rFjlWMLRFK2XrzEeQfZeH7msLBIJgip4Ucdmy5jEM9IwWgkFfYc4ZTttLItvy6w24tmv80gbJSvfpX1eOJxKFnYYQ1T+BiJ/OVvky8cCMr4yOwP0c8DMICFbBH/rWc2gpd3hXyr2IjTv1fSQ8Z+w7vs9u0jgqX/xYIZe+VcDo2x5eSj5VfWcN2phLK932GI9ADUN/MUcaMuarJJCz3GrlW0I+cMuIacptOamHbf0nEmZiyuVmUuubeegNU/fFUBiPNhThSzy1u2xky1w+w0x0igwHkLvyza5ARERQppB219oYwoZntIecFeY5zsRnYeHAt787ry0bkpL5OXW3Qsu4dwx611cEQptRfya3T7F402VubMIB2GBf1f8xPQs91NKFbSyCvz9AQr2ceLPkencYOAMnGHKYN1K/vcs5L73xCaYEQvaPuhAWyIhbfVgn2aIOXhqKa59vgUTHm67F68eNUOGo0bjRDZ74lM4MKvfJxp6/wYgqWPoXumL+olBFndHthFo9/uXR5aqIQpzGhZtvapRIfrYj5PxlBxPaw3T7uzdCEJT+hu4Tjy4As5qIj2bEEGf5qngcDAi5b6zEv8+jivIRMa5i9sfn9Fu/kPsQp5djXrRDHJNOK8x0ruCUpizyCB6bsoe/A0UP662Y1MmIKkuNtPvpOCurbQjDkZ+Rz5sW/ddKR2SH642VNyvIk2Lxhk4MlPQ0W1X0XFI/LQ5ksgRL1eLYNbQqpVmoDqglJtix5lpSUp++821BC0xORNis/5ZpT6WeZ54QSkLo7s0HaW0kx5CTCKMqSRbn4uDWKpnP4MrgbauAoVmmygWe+Fea3aEfsYgReN40cTSJk9Tw0ZqzW3ogNj
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 744407cd-6efa-47ca-b7f9-08dbe958af56
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Nov 2023 23:38:51.2493
 (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: WDoOlIDL74zVbNODbBIpHU1q4duNNtXwCQFjVn+GBNl71LO5BPdRcE7lIdirTj/BPZkJTwCcBG5aOLhtyE6XSA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR10MB7990
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-11-19_19,2023-11-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 spamscore=0 phishscore=0
 mlxlogscore=834 adultscore=0 bulkscore=0 malwarescore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311060000
 definitions=main-2311190181
X-Proofpoint-GUID: BfAXvUArXkTdTXnLw-ySO66yJ-MSy0RN
X-Proofpoint-ORIG-GUID: BfAXvUArXkTdTXnLw-ySO66yJ-MSy0RN
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Sun, 19 Nov 2023 15:44:58 -0800 (PST)

On Mon, Nov 20, 2023 at 08:37:55AM +1100, NeilBrown wrote:
> On Fri, 17 Nov 2023, Jeff Layton wrote:
> > On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > > The protocol for creating a new state in nfsd is to allocated the state
> > > leaving it largely uninitialised, add that state to the ->cl_stateids
> > > idr so as to reserve a state id, then complete initialisation of the
> > > state and only set ->sc_type to non-zero once the state is fully
> > > initialised.
> > > 
> > > If a state is found in the idr with ->sc_type == 0, it is ignored.
> > > The ->cl_lock list is used to avoid races - it is held while checking
> > > sc_type during lookup, and held when a non-zero value is stored in
> > > ->sc_type.
> > > 
> > > ... except... hash_delegation_locked() finalises the initialisation of a
> > > delegation state, but does NOT hold ->cl_lock.
> > > 
> > > So this patch takes ->cl_lock at the appropriate time w.r.t other locks,
> > > and so ensures there are no races (which are extremely unlikely in any
> > > case).
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > >  fs/nfsd/nfs4state.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 65fd5510323a..6368788a7d4e 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -1317,6 +1317,7 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
> > >  
> > >  	lockdep_assert_held(&state_lock);
> > >  	lockdep_assert_held(&fp->fi_lock);
> > > +	lockdep_assert_held(&clp->cl_lock);
> > >  
> > >  	if (nfs4_delegation_exists(clp, fp))
> > >  		return -EAGAIN;
> > > @@ -5609,12 +5610,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > >  		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);
> > >  	spin_unlock(&fp->fi_lock);
> > > +	spin_unlock(&clp->cl_lock);
> > >  	spin_unlock(&state_lock);
> > >  
> > >  	if (status)
> > 
> > I know it's (supposedly) an unlikely race, but should we send this to
> > stable?
> 
> I don't know.  Once upon a time, "stable" meant something.  There was a
> clear list of rules.  Those seem to have been torn up.  Now it seems to
> be whatever some machine-learning tool chooses.
> If that tool chooses this patch (which I suspect it will), I won't
> object.  But I don't think it is worth encouraging it.

We've asked Sasha and GregKH not to use AUTOSEL on NFSD patches,
promising that we will explicitly mark anything that should be
back-ported.


-- 
Chuck Lever