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 06C4B3AC0D3 for ; Tue, 12 May 2026 10:07:07 +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=1778580430; cv=none; b=nxIlY+IVweQfj3xefgP4ipNu1Nb6ULYYNTwQp4iZ4B0wuS3s/rF6xH3Qs6dkjSWKNuDkxjwm592xMRwr0lHUs8CqHpnmTlkg38w5hw65DqjrxPSM9P5G/BVB8ia2XBdj77K4+DvuhM5xQNYHf5Gr1GP0QsSyi6qTZo3QnwAMof8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778580430; c=relaxed/simple; bh=h0i4ieS/BkyXiFtwoxy7yihh3Nl6Xo9jU/58MwAo/R8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=NbvJpYrPRkLh/C9g2ZP+d7QuS1hkzgJLNx0BOd5hMs1RFBkwJf2z05kP4ulZQ7H+ksKn+l8i8DLj+yrpHKGjCuUg8RTyIoTqicZxHLSWqFFKR5YthI23YGGnFnu/8T5+jSYp9ypwZdiSgBhZFiPJfSkT1KtLZleVMK/qPNHJYbc= 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=aCR5cVer; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=eNRpbl//; 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="aCR5cVer"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="eNRpbl//" Received: from phl-compute-03.internal (phl-compute-03.internal [10.202.2.43]) by mailfhigh.stl.internal (Postfix) with ESMTP id A29667A011F; Tue, 12 May 2026 06:07:06 -0400 (EDT) Received: from phl-frontend-04 ([10.202.2.163]) by phl-compute-03.internal (MEProxy); Tue, 12 May 2026 06:07:07 -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=fm1; t=1778580426; x= 1778666826; bh=fvByL/bwiqxIfeftWHoaHQZg8eSZadJNGAhfU4qTq98=; b=a CR5cVerUaogJl1xlcSx10/CikMgqf8P8uM/lPwsAU5i0NNSHSJ5QQzbvUxqBDjR9 /3LemY/e5Xf3g+0jTcWKjXe2cxzTxhw06JzxSwb3b1DTRWqnCfpe2xlsl/jG/AK1 4f2bgLfafklgGWUtcs7ZO5heCsU7gERUomS9SkT1RPh/1eqtN6H1r1RQXgP4pEL6 TF+U9aG1iomr+YUJG9u7RQQSjkCa3kOi3omPJe/+s4J2Kd3BM0emjbohtPPyiE+F xiKm5/YASpG6lcPpzzdRwLff1nZcMyJLYjM/RZ0plpiTYdPD4k2JqembNO1jUjBJ ZjKwIt66KyetXHZ7e/K6w== 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=fm3; t= 1778580426; x=1778666826; bh=fvByL/bwiqxIfeftWHoaHQZg8eSZadJNGAh fU4qTq98=; b=eNRpbl//U+XeKYdqgvWJJAWRii0ByUl7B7f2B8wol9cuQmcw6fE emgSol+UUvxAnVEtKI6cewZcYRzbsmU4OdjbUZPhkeg/vcFyN2rggnC0Ae3NcAL2 5j+vopNBIP02+xImPdQIRkT8GJKnrmcBO2L0FkkC/eToZAB8kbCmV3tCl67pu3X3 z2WC+M8qpyPz7uaQ8RtYzRGJQOCD4rLg5PGmSt3besOsUw+u3s0cwYpX21rqeAGk KaN6h6Mw6UmO42kXf0CtfHgEB4hBVV9OzEQB2vNdCk+cVj8B69I3g1V/mqESk+PY i5wuL0H5Q46+gWA+obRZR+0cIGwrdNJ2XRA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefhedrtddtgdduvdduhedvucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu rghilhhouhhtmecufedttdenucenucfjughrpeffhffvvefukfhfgggtuggjsehttdertd dttdejnecuhfhrohhmpefurggsrhhinhgrucffuhgsrhhotggruceoshgusehquhgvrghs hihsnhgrihhlrdhnvghtqeenucggtffrrghtthgvrhhnpeeuhffhfffgfffhfeeuieduge dtfefhkeegteehgeehieffgfeuvdeuffefgfduffenucevlhhushhtvghrufhiiigvpedt necurfgrrhgrmhepmhgrihhlfhhrohhmpehsugesqhhuvggrshihshhnrghilhdrnhgvth dpnhgspghrtghpthhtohepudehpdhmohguvgepshhmthhpohhuthdprhgtphhtthhopehn tdehvggtsehliihurdgvughurdgtnhdprhgtphhtthhopehnvghtuggvvhesvhhgvghrrd hkvghrnhgvlhdrohhrghdprhgtphhtthhopegurghvvghmsegurghvvghmlhhofhhtrdhn vghtpdhrtghpthhtohepvgguuhhmrgiivghtsehgohhoghhlvgdrtghomhdprhgtphhtth hopehkuhgsrgeskhgvrhhnvghlrdhorhhgpdhrtghpthhtohepphgrsggvnhhisehrvggu hhgrthdrtghomhdprhgtphhtthhopehhohhrmhhssehkvghrnhgvlhdrohhrghdprhgtph htthhopehsthgvfhhfvghnrdhklhgrshhsvghrthesshgvtghunhgvthdrtghomhdprhgt phhtthhopehhvghrsggvrhhtsehgohhnughorhdrrghprghnrgdrohhrghdrrghu X-ME-Proxy: Feedback-ID: i934648bf:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 12 May 2026 06:07:04 -0400 (EDT) Date: Tue, 12 May 2026 12:07:02 +0200 From: Sabrina Dubroca To: Ren Wei Cc: netdev@vger.kernel.org, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, horms@kernel.org, steffen.klassert@secunet.com, herbert@gondor.apana.org.au, yuantan098@gmail.com, yifanwucs@gmail.com, tomapufckgml@gmail.com, bird@lzu.edu.cn, ronbogo@outlook.com, zylzyl2333@gmail.com Subject: Re: [PATCH net 1/1] xfrm: espintcp: publish ULP context before entry points Message-ID: References: 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: Thanks for the fix. A small note: IPsec fixes go through the "ipsec" tree, not "net", so the prefix should be [PATCH ipsec] Some comments inline: 2026-05-11, 21:40:58 +0800, Ren Wei wrote: > diff --git a/include/net/espintcp.h b/include/net/espintcp.h > index c70efd704b6d..034be559786b 100644 > --- a/include/net/espintcp.h > +++ b/include/net/espintcp.h > @@ -34,7 +34,16 @@ static inline struct espintcp_ctx *espintcp_getctx(const struct sock *sk) > { > const struct inet_connection_sock *icsk = inet_csk(sk); > > - /* RCU is only needed for diag */ > - return (__force void *)icsk->icsk_ulp_data; > + /* > + * The caller reached an ESP entry point by observing sk_prot, > + * sk_socket->ops, or one of the socket callbacks. Keep the ctx > + * load after that observation so the caller cannot see the new > + * entry point while still seeing stale icsk_ulp_data. I don't think this comment is really helpful. > + * > + * Pairs with smp_wmb() in espintcp_init_sk(). > + */ > + smp_rmb(); > + > + return (__force void *)READ_ONCE(icsk->icsk_ulp_data); I think smp_store_release/smp_load_acquire is the "standard spelling" for this now. [...] > @@ -472,34 +476,46 @@ static int espintcp_init_sk(struct sock *sk) > > __sk_dst_reset(sk); > > - strp_check_rcv(&ctx->strp); > skb_queue_head_init(&ctx->ike_queue); > skb_queue_head_init(&ctx->out_queue); > + ctx->saved_data_ready = READ_ONCE(sk->sk_data_ready); > + ctx->saved_write_space = READ_ONCE(sk->sk_write_space); > + ctx->saved_destruct = READ_ONCE(sk->sk_destruct); If something is changing those while espintcp_init_sk is running, READ_ONCE won't help us. We'll end up with the wrong saved_* values. Can this actually happen here? > + INIT_WORK(&ctx->work, espintcp_tx_work); > + > + /* avoid using task_frag */ > + sk->sk_allocation = GFP_ATOMIC; > + sk->sk_use_task_frag = false; > > if (sk->sk_family == AF_INET) { > - sk->sk_prot = &espintcp_prot; > - sk->sk_socket->ops = &espintcp_ops; > + prot = &espintcp_prot; > + ops = &espintcp_ops; > } else { > mutex_lock(&tcpv6_prot_mutex); > if (!espintcp6_prot.recvmsg) > - build_protos(&espintcp6_prot, &espintcp6_ops, sk->sk_prot, sk->sk_socket->ops); > + build_protos(&espintcp6_prot, &espintcp6_ops, > + READ_ONCE(sk->sk_prot), > + READ_ONCE(sk->sk_socket->ops)); And similar here. Those should always be tcpv6_prot/inet6_stream_ops, but I wrote it this way to avoid having to use stubs, back when IPv6 could be built as a module. This could now be moved into espintcp_init like the ipv4 variant of this. > mutex_unlock(&tcpv6_prot_mutex); > > - sk->sk_prot = &espintcp6_prot; > - sk->sk_socket->ops = &espintcp6_ops; > + prot = &espintcp6_prot; > + ops = &espintcp6_ops; > } Or just move the whole block to the end, instead of introducing those temporary variables? > - ctx->saved_data_ready = sk->sk_data_ready; > - ctx->saved_write_space = sk->sk_write_space; > - ctx->saved_destruct = sk->sk_destruct; > - sk->sk_data_ready = espintcp_data_ready; > - sk->sk_write_space = espintcp_write_space; > - sk->sk_destruct = espintcp_destruct; > rcu_assign_pointer(icsk->icsk_ulp_data, ctx); > - INIT_WORK(&ctx->work, espintcp_tx_work); > > - /* avoid using task_frag */ > - sk->sk_allocation = GFP_ATOMIC; > - sk->sk_use_task_frag = false; > + /* > + * Publish the fully initialized ctx before publishing any entry point > + * that can call espintcp_getctx(). The read barrier there runs after > + * the caller has observed one of these pointers. > + */ > + smp_wmb(); > + WRITE_ONCE(sk->sk_prot, prot); > + WRITE_ONCE(sk->sk_socket->ops, ops); > + WRITE_ONCE(sk->sk_data_ready, espintcp_data_ready); > + WRITE_ONCE(sk->sk_write_space, espintcp_write_space); > + WRITE_ONCE(sk->sk_destruct, espintcp_destruct); > + > + strp_check_rcv(&ctx->strp); > > return 0; > > @@ -530,7 +546,7 @@ static void espintcp_close(struct sock *sk, long timeout) > > strp_stop(&ctx->strp); > > - sk->sk_prot = &tcp_prot; > + WRITE_ONCE(sk->sk_prot, &tcp_prot); Actually this should be the original sk_prot, which could be tcpv6_prot. I'm not sure how much the WRITE_ONCE matters here. What is it protecting against/synchronizing with? -- Sabrina