From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (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 A1ADB3E6DCE for ; Wed, 27 May 2026 10:21:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779877299; cv=none; b=RLZ3+oLQhNMXGagEcq4Orwmql77vx5FjRQ11EDHiRl+eKLOBt5I6mlON6kgf+EBwyL/oeUz2jbm2Ln/rI9+VQHrl9lXMKw3RRpU0Un0tuz2J7qknRCITvUr+AgKheQTluppO1tOHAt2bx3hIuM77569pPUViPYb+qw4B/9FJzpY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779877299; c=relaxed/simple; bh=s79viPu2d2HZuyW0HbWsr7Eave7tm4XA1tl/HWuoK1U=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=tYATHCKmfvyZmZO03EPt/jHPvcOmpR0R2Vlrgf7jQyGXqCC4yMpKkEuFK+RBB7PaQ6+OXvX7DN2De0mPd3OGpe8JkDw6Z9gEgOGiEAdcbwaZLkvu179jXuwNNW/RGzhai5P+wtu0Eu+52jkQEIppN62jSz5wJUDgTmi0n9arogw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=N5wpdlXU; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=tfNe2ZYx; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="N5wpdlXU"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="tfNe2ZYx" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1779877296; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KOfZTr2QhtogmOSr0x0T/QYiQ3eokGu9YlGbUilxeG0=; b=N5wpdlXUAzyInEeNuCTg79K8EkjfQdsdBCj/fNudLFpkkkpAjlDQdrc4TsC3jGDQCh371I qIEVbGh7Zpuyr4izUe5iAjvKZQQgvAli4MhhNMs+F+nlK/HwzeZYlCQevxPzQeTv8oViUO oGpyyAuVg+FFp1QJHpYJ0YRHcu2REVc= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-397-YyGV_DDOPp6FU9e3-aeUXQ-1; Wed, 27 May 2026 06:21:34 -0400 X-MC-Unique: YyGV_DDOPp6FU9e3-aeUXQ-1 X-Mimecast-MFC-AGG-ID: YyGV_DDOPp6FU9e3-aeUXQ_1779877293 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-48fdb2b0cb8so67279075e9.0 for ; Wed, 27 May 2026 03:21:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1779877293; x=1780482093; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:from:to:cc:subject:date:message-id :reply-to; bh=KOfZTr2QhtogmOSr0x0T/QYiQ3eokGu9YlGbUilxeG0=; b=tfNe2ZYxdoTNzeb4MyHzGUH9HKI/Ja6Hni+qQV+fyiv2tzND9ZfUIqlamSF5dCZY3+ SvqrJtw8mVERyvctX+U64lkax631xzxC6BvsgttZwxqMdglB2s/Wcl+mUEAiYEAyUjUg 5PwGWKJT4zfgKTFAv4EpOAuNxRySTn7UGQnTR250e9FPqDZDoi/NiVi7Sk4FYC1QirMA xBhHCAOGb73JWuaN3ks/YKDtUZrH20E1Ys38GVRRVlB7ymp+vQzBp0/y/7S+UZMwrcc/ lz5UzFi/A7M915gLp1Y/EwAcRlkWJk34bhaqK7z+YjuCujhRX8CTU62vabr421kaDDFO PIZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779877293; x=1780482093; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=KOfZTr2QhtogmOSr0x0T/QYiQ3eokGu9YlGbUilxeG0=; b=jbUxjeEC+r3cQTuF2tQt36KsJf6A9FHB8T53TJBDim+0277jlmMsSsz4Pl+WdBhbjO Yuw4mCOBA6G93GovMuXE/s3XERS6r6rbF0G9Iy/s5hOhUv4gIK/MspLRQ77c7lca3uUi cQe4P62CnyTsNUw7U7nZ9UdbGbfp39n44aJMTKm88P1Xu1Txi7tpL43xeqghiEEZ8sZY DogJfYGMOwi3V73+dmBm7+Hoo9UrCMTwZJMmtTmzNpiC2G7qAR0FkwIb7a8rrKHBy93e YPE8aNDUsHDSSxh4rep63sNNijO/HpfUknSqRyGwgHBiUdSgah4n7jHZpMz0wxvJh6kh 1/nA== X-Forwarded-Encrypted: i=1; AFNElJ+Pdk1uTcgZIugeC89z6fcX423MENVIyOZjmpFwzyzZkhfHRYoyZb+1CgRxVbWh1SZuNTYvElyVv6OYsK4=@vger.kernel.org X-Gm-Message-State: AOJu0YxuhLtyW082KUrGy5T+e68f8aJyD9cxiJD9zcKyqcmw+jui/cMp B2cyZL9A46Nuk1k7OwhdA+VQPXztr4iZlkNkNTvw+fpf8XZGskJKQlYuNRqHHTC8CSZ/vvfR3wc PrjvA/I5EZPGa7LEjreamGqAvOFDpyaaXwAz/GZDSOGG/UJNAvJSw8JeVGmDHe7C/Vw== X-Gm-Gg: Acq92OHipjBpPowjSIuQGq4CmyoZOyS4+XC1EF81i3gvw+LmQ4mYetMXMcHMUodZMQO Xb+RXR1qO8cJE34UmZFGnLm4qN45pkWuvjd2QRysWgc4DGCIMgyRIDZsZ5hiRjGnq70QCRAIxcN rcXwJy+JdsQD5mzY39EgDOc13pv0Q6mjT1AGFPRRqcB/iGEfSCDvxEYxBZbiUq+cicjrsiXCl/5 NGbqktEVF9i33hFO1I19KP2SUrCFJvC/scJrKCVfCvBwpisaqodfuoAM68a8Jv9efN4j8Jm7i1U vPEaETtiTQNsYh3CDtMNPC1ckbbnfeGVqBeo+1EFNHibrajmDyrzUugaPsE9gnQEvPKQiq1UVdv rqmjpYbXzofDynsbzlGytGM2uslmSnjCZrgdMfrujb7ZOA/0iShs9QxXHuqY= X-Received: by 2002:a05:600d:640f:10b0:490:3d45:48d7 with SMTP id 5b1f17b1804b1-4904248d296mr268514675e9.6.1779877293242; Wed, 27 May 2026 03:21:33 -0700 (PDT) X-Received: by 2002:a05:600d:640f:10b0:490:3d45:48d7 with SMTP id 5b1f17b1804b1-4904248d296mr268514195e9.6.1779877292777; Wed, 27 May 2026 03:21:32 -0700 (PDT) Received: from alrua-x1.borgediget.toke.dk (alrua-x1.borgediget.toke.dk. [2a0c:4d80:42:443::2]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4904527f7f7sm579202405e9.7.2026.05.27.03.21.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 May 2026 03:21:32 -0700 (PDT) Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 23A317BA3CB; Wed, 27 May 2026 12:21:31 +0200 (CEST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: David Laight Cc: Jamal Hadi Salim , netdev@vger.kernel.org, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, horms@kernel.org, jiri@resnulli.us, victor@mojatatu.com, yimingqian591@gmail.com, keenanat2000@gmail.com, 2045gemini@gmail.com, rollkingzzc@gmail.com, dcaratti@redhat.com, security@kernel.org, linux-kernel@vger.kernel.org, Rajat Gupta Subject: Re: [PATCH net v2 1/1] net/sched: fix pedit partial COW leading to page cache corruption In-Reply-To: <20260527100055.1661c5c9@pumpkin> References: <20260526155913.1060694-1-jhs@mojatatu.com> <87wlwq2cxf.fsf@toke.dk> <20260527100055.1661c5c9@pumpkin> X-Clacks-Overhead: GNU Terry Pratchett Date: Wed, 27 May 2026 12:21:31 +0200 Message-ID: <87tsrt2lw4.fsf@toke.dk> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable David Laight writes: > On Tue, 26 May 2026 21:22:52 +0200 > Toke H=C3=B8iland-J=C3=B8rgensen wrote: > >> Jamal Hadi Salim writes: >>=20 >> > From: Rajat Gupta >> > >> > tcf_pedit_act() computes the COW range for skb_ensure_writable() >> > once before the key loop using tcfp_off_max_hint, but the hint does >> > not account for the runtime header offset added by typed keys. This >> > can leave part of the write region un-COW'd. >> > >> > Fix by moving skb_ensure_writable() inside the per-key loop where >> > the actual write offset is known, and add overflow checking on the >> > offset arithmetic. For negative offsets (e.g. Ethernet header edits >> > at ingress), use skb_cow() to COW the headroom instead. Guard >> > offset_valid() against INT_MIN, where negation is undefined. >> > >> > Additionally, linearize skbs with shared frags upfront to prevent >> > silent data corruption when pedit operates on zero-copy pages >> > (e.g. from sendfile). >> > >> > Fixes: 8b796475fd78 ("net/sched: act_pedit: really ensure the skb is w= ritable") >> > Reported-by: Yiming Qian >> > Reported-by: Keenan Dong >> > Reported-by: Han Guidong <2045gemini@gmail.com> >> > Reported-by: Zhang Cen >> > Tested-by: Victor Nogueira >> > Acked-by: Jamal Hadi Salim >> > Signed-off-by: Rajat Gupta =20=20 >>=20 >> Re-ran the tests, and everything looks good, so: >>=20 >> Tested-by: Toke H=C3=B8iland-J=C3=B8rgensen >>=20 >> Also looked at the code, and I have a few nits below, but I'm really >> nitpicking here, so whether you end up fixing those or not: >>=20 >> Reviewed-by: Toke H=C3=B8iland-J=C3=B8rgensen >>=20 >>=20 >> [...] >>=20 >> > @@ -323,7 +324,7 @@ static bool offset_valid(struct sk_buff *skb, int = offset) >> > if (offset > 0 && offset > skb->len) >> > return false; >> >=20=20 >> > - if (offset < 0 && -offset > skb_headroom(skb)) >> > + if (offset < 0 && offset < -(int)skb_headroom(skb)) >> > return false;=20=20 >>=20 >> This change makes it really obvious that this is really just: >>=20 >> if (offset < -(int)skb_headroom(skb)) >> return false; >>=20 >> so, well, that would be clearer, IMO. >>=20 >> But then I guess the same could be said of the positive case, so: >>=20 >> static bool offset_valid(struct sk_buff *skb, int offset) >> { >> if (offset > skb->len || offset < -(int)skb_headroom(skb)) >> return false; >>=20 >> return true; >> } > > There are all sorts of integer conversions going on. > IIRC Both skb->len and skb_headroom() are 32bit unsigned. > skb_headroom() is relatively small, skb->len can be over 64k but nowhere > near MAX_INT. Right, I had the implicit signed/unsigned conversions the wrong way 'round in my head. So what's missing above is a cast of skb->len to int, i.e.: if (offset > (int)skb->len || offset < -(int)skb_headroom(skb)) return false; right? > offset is signed 32bit and the code is allowing for it being -MAX_INT > (but I'm not at all sure whether that can happen without overflow being l= ikely). > > So I think the single test: > if (offset + skb_headroom(skb) >=3D skb->len + skb_headroom(skb)) > return false; > is correct. > If offset is 'too negative' the LHS will be 'very large postitive' and > the test fails. Yeah, I agree. FWIW, they turn out to be the same number of instructions, the difference being that one contains a jump and the other doesn't. It seems clang needs an unlikely() to put that jump in the 'false' path, though: https://godbolt.org/z/5o5Gxqe3W OTOH, I think the two tests are more readable; the single-test version would need a comment explaining your "too negative" rationale. But given the subtleties of the implicit integer conversions, maybe that's better anyway. And they're both half the instructions of the version currently in the patch, so if we're code golfing (which I guess we are at this point) I guess we should pick one of those :) -Toke