From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fhigh-b4-smtp.messagingengine.com (fhigh-b4-smtp.messagingengine.com [202.12.124.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F017F3750B5 for ; Thu, 12 Mar 2026 14:36:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=202.12.124.155 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773326190; cv=none; b=WWz8cxBhwh9WpqLH/io1yjt8gXmd6kujGJJjRtZoLZFbENUOXpZBSayohLnepJ4HovGdWjrMRhycY7kwnfsNZab/CsiKRVzIzjmGG3OUQyQzoSDRxbSwSkz7e2oqsrC/JVooBH1NEazthq1B2jSLvj7vv3TdXAVmOzm2jTzmVCU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773326190; c=relaxed/simple; bh=A3V8OpwOzCe0BY10Acyv/ngFsbTr+1i4YCqzcG6H3+Q=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=k8oszPdqHvXP4t6H+wNgUBtPnLWd4iNmx7ut+xgx7q1JzZ2eBkWcWEWsFzEDJQCIhmfPqoHG6+tXsayBJuPnFCY5908+r0fx3VTuTqllV8JAtGW8bc8BW+NjdnSh9exosicmWUadXd1oel0xOcLD2bXG8zH59TrTKpJNtsL7ibQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=queasysnail.net; spf=pass smtp.mailfrom=queasysnail.net; dkim=pass (2048-bit key) header.d=queasysnail.net header.i=@queasysnail.net header.b=AKNi02/u; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=F7vTSxZ2; arc=none smtp.client-ip=202.12.124.155 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=queasysnail.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=queasysnail.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=queasysnail.net header.i=@queasysnail.net header.b="AKNi02/u"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="F7vTSxZ2" Received: from phl-compute-10.internal (phl-compute-10.internal [10.202.2.50]) by mailfhigh.stl.internal (Postfix) with ESMTP id 4B36D7A01A6; Thu, 12 Mar 2026 10:36:25 -0400 (EDT) Received: from phl-frontend-03 ([10.202.2.162]) by phl-compute-10.internal (MEProxy); Thu, 12 Mar 2026 10:36:25 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=queasysnail.net; h=cc:cc:content-type:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:subject:subject:to:to; s=fm2; t=1773326185; x= 1773412585; bh=fg/YChO0KtNDnpjTWabektRbJ4lnU5lXR2RoltovU8o=; b=A KNi02/u2+rWUlo/ZbJtquZ2O6ZaYBVpWyON9qcMR57Dn0ngoMOG09ap7bXknSeg2 Exw1wXPE/D57ncELeYwg553g7yWLAK8KWTU4BPqp/fUzrVJkzzPv7mNVwEAsB4sL 2mdOGAOLy1+43Z+kBDMHLlWTn/1bMledh54vhEQqzm4c8DiU86VEPnNw5ZEeYtk0 JpZL1jNFupKte5gcwPS4LqxHIvPmIIql7y6NNJcMNXcOZi4WHN2EWtJoyoxd1P+4 5GsQB9KgZjkzvJGY+iNa4AsEVKD4nh42Y5MTVbTOg8TqArXJWoqhBt70EoBAJGoo mtB2RL0JhBzhM+GEsVukw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t= 1773326185; x=1773412585; bh=fg/YChO0KtNDnpjTWabektRbJ4lnU5lXR2R oltovU8o=; b=F7vTSxZ2CrjKL5oan21x7zuGtwnNeUjTXD2GwrtB4t9Sc8X3dnc VM36/R2/aPLHxqNghBg4Na1unxVYpZ2NrfsmQl7OdllW5T9rVkkfBYbVo1iv03jv /a9mdc5fxi/xQiaHXNLz71jkpPvE3xPCRqPT0zIcHrXMpWOCTyFSy2gsYXOZ+pJq uAWcSoATLi1sGv4q/mTMqrIwUSCeZ5hqvOljijpfzKg2DEN2fiv92Vd39N8FO8hb kPHig3wURPpqcMdLmJC4qDT/nPgR0o6bP89Rfixr3hszPCLYwxCWVurE2FtJELzr u520yo4wk15bxLoSrSNrpM8nW5Tk3Syi9DQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefgedrtddtgddvkeejtdegucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu rghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf gurhepfffhvfevuffkfhggtggujgesthdtredttddtjeenucfhrhhomhepufgrsghrihhn rgcuffhusghrohgtrgcuoehsugesqhhuvggrshihshhnrghilhdrnhgvtheqnecuggftrf grthhtvghrnhepuefhhfffgfffhfefueeiudegtdefhfekgeetheegheeifffguedvueff fefgudffnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomh epshgusehquhgvrghshihsnhgrihhlrdhnvghtpdhnsggprhgtphhtthhopeeipdhmohgu vgepshhmthhpohhuthdprhgtphhtthhopehlvghonheskhgvrhhnvghlrdhorhhgpdhrtg hpthhtohephhhorhhmsheskhgvrhhnvghlrdhorhhgpdhrtghpthhtohepfhifsehsthhr lhgvnhdruggvpdhrtghpthhtohepnhgvthguvghvsehvghgvrhdrkhgvrhhnvghlrdhorh hgpdhrtghpthhtohepshhtvghffhgvnhdrkhhlrghsshgvrhhtsehsvggtuhhnvghtrdgt ohhmpdhrtghpthhtohephhgvrhgsvghrthesghhonhguohhrrdgrphgrnhgrrdhorhhgrd gruh X-ME-Proxy: Feedback-ID: i934648bf:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 12 Mar 2026 10:36:23 -0400 (EDT) Date: Thu, 12 Mar 2026 15:36:21 +0100 From: Sabrina Dubroca To: Leon Romanovsky Cc: Simon Horman , Florian Westphal , netdev@vger.kernel.org, Steffen Klassert , Herbert Xu Subject: Re: [PATCH ipsec-next 01/10] xfrm: state: fix sparse warnings on xfrm_state_hold_rcu Message-ID: References: <7388df7238672a92be0e4048f0225e6db294e736.1773051558.git.sd@queasysnail.net> <20260310103135.GB12611@unreal> <20260310182012.GF12611@unreal> <20260310194500.GO12611@unreal> <20260310201021.GP12611@unreal> <20260311073635.GR12611@unreal> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20260311073635.GR12611@unreal> 2026-03-11, 09:36:35 +0200, Leon Romanovsky wrote: > On Tue, Mar 10, 2026 at 10:41:46PM +0100, Sabrina Dubroca wrote: > > (adding Simon: maybe you can help with the "comment wording" issue here :)) > > > > 2026-03-10, 22:10:21 +0200, Leon Romanovsky wrote: > > > On Tue, Mar 10, 2026 at 08:49:44PM +0100, Florian Westphal wrote: > > > > Leon Romanovsky wrote: > > > > > > You can only use xfrm_state_hold() if the refcount is already > 0. > > > > > > xfrm_state_hold uses refcount_inc(), so you get a UaF warn splat > > > > > > if this assuption doesn't hold true. > > > > > > > > > > I know it, the thing that bothers me is that it is unclear how > > > > > xfrm_state_hold_rcu() can have refcount equal to 0. > > > > > > > > > > xfrm_state_put() decreases refcount and when it is zero, it calls > > > > > to __xfrm_state_destroy(). The latter assumes that the state was > > > > > already removed from various hlists. > > > > > > > > Yes, insertion in the table means refcount is 1, but userspace > > > > can zap states at any time, e.g.: > > > > > > > > xfrm_del_sa -> xfrm_state_delete -> __xfrm_state_delete (which > > > > unlinks from hash lists). > > > > > > > > The last xfrm_state_put() in that function may cause 1 -> 0 > > > > transition. Parallel lookup can still observe that state, > > > > so it has to pretend it wasn't there to begin with. > > > > Thanks Florian. > > > > > > > Yes, this is possible scenario and this is what is worth to document. > > > > > > We could add something like: > > > > /* Take a reference to @x, when we know the state has a refcount >= 1. > > * In this case, we can avoid refcount_inc_not_zero and the error > > * handling it requires. > > * In contexts where concurrent state deletion is possible and we > > * don't already hold a reference to that state, xfrm_state_hold_rcu > > * must be used. > > */ > > > > Though it may not make much sense to refer to xfrm_state_hold_rcu > > (implemented in net/xfrm/xfrm_state.c) from include/net/xfrm.h. > > > > And if we consider the hashtables to be private to > > net/xfrm/xfrm_state.c, nothing outside of it should ever see a state > > with refcount=0, since they will only ever see states that already > > have one reference held by whatever gave them the pointer. > > > > So maybe it's more xfrm_state_hold_rcu that needs a mention of > > "concurrent state deletion could bring the refcount to 0 while we're > > doing the lookup"? I don't know, for me it's pretty obvious with the > > _rcu suffix that RCU -> unlocked -> could be deleted in parallel. > > It was the case when xfrm_state had __rcu marker, it is less obvious > now. I agree with you that xfrm_state_hold_rcu() needs to be documented > and not xfrm_state_hold(). Ok. Would that work for you? /* Take a reference to @x. * This must be used (and the error handled) for unlocked lookups * where concurrent state deletion could bring the refcount to 0. * * When we know that the state has a refcount >= 1, xfrm_state_hold * can be used. */ If not, what other information do you need? Then I can send the patch separately, as Steffen said. -- Sabrina