From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 266775A0284 for ; Thu, 30 Nov 2023 01:55:12 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1701305703; bh=Jstmtu3KRks28k+U70P6NyGBeK6Wvva+UMwDaaCi2bI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jCY1V/T9C0estvgZhVN3x1E7JZKqMDywbU4uAREa/qBwUNba+QA2mFUVVsFymT+ha 6+t2EM5eP6RvMMcc9T/8FHOFsd8xOcrpGf0Vo0Vt637+Vxh3f4fSODCUNTJrzPUJK4 a8FUZrSrqc6XJsRfyoZdWym+dTfxLhX8F00pMoZk= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Sgd5M3sgPz4xWL; Thu, 30 Nov 2023 11:55:03 +1100 (AEDT) Date: Thu, 30 Nov 2023 11:42:33 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 08/11] tcp: Remove unneccessary bounds check in tcp_timer_handler() Message-ID: References: <20231126233348.1599864-1-david@gibson.dropbear.id.au> <20231126233348.1599864-9-david@gibson.dropbear.id.au> <20231129153239.2df7ee71@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DJawYnPHCm25NSR8" Content-Disposition: inline In-Reply-To: <20231129153239.2df7ee71@elisabeth> Message-ID-Hash: L5T2M7E3O2IW66UCGYDHJXS5VTKDWIZY X-Message-ID-Hash: L5T2M7E3O2IW66UCGYDHJXS5VTKDWIZY X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --DJawYnPHCm25NSR8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 29, 2023 at 03:32:39PM +0100, Stefano Brivio wrote: > On Mon, 27 Nov 2023 10:33:45 +1100 > David Gibson wrote: >=20 > > In tcp_timer_handler() we use conn_at_idx() to interpret the flow index > > from the epoll reference. However, this will never be NULL - we always > > put a valid index into the epoll_ref. Simplify slightly based on this. >=20 > Sorry, I missed this during review of v1. >=20 > I have mixed feeling about this, and I don't think 11/11 changes > anything in this regard: we have to trust the kernel, as there's no > benefit to security in not doing so. >=20 > At the same time, should we ever get an out-of-bounds index from the > epoll event, we can fail gracefully here. Slightly worse, however: if > we get a timer event for a connection that's already closed, we'll > happily go and try to manipulate its timer (with or without the !conn > check). So, as you note this only verifies that the index is theoretically possible. It doesn't check that it's a valid index in the current size of the connection table, it doesn't check if the connection is already closed and it can't check if it's a stale index for a different connection than the one originally intended, because the table has been compacted. Given all those potential failure modes, I don't see a lot of value in checking if the index is wildly out of bounds, which would require a kernel bug rather more extreme than those other possibilies. > All in all, I think the check is minimally useful, and we should have > something more robust than that. So if this patch helps keeping things > simple later in the series, I don't see an issue with that, but perhaps > we should add back a more sensible set of checks later. Perhaps, yes. > The next patches all look good to me. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --DJawYnPHCm25NSR8 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmVn2ngACgkQzQJF27ox 2GfeDhAAp2j4sYtvwZ8z6iCBUbQawd7naFeCtD7xfdLddoYOoZR+O4CKyM+S5j/H lLn2jlXsCKpKNgrByo5X5Nj6SVqyDbGAc0UV+HpLTi4SmNz7Ti/19qbXAmAWZ8mr jrrui8p76u/1fTcSvV8VQkfiozVvi8zXjWPC9HVePRZ+kYMYbcTsB3g+8O+tD7Fs KDw05IwmTIQrv5sh3gfkN4DznpKHj2lT040TivoHlnGKJRFJkeQZPYwqys1k18iS rocg3ufwFgaAPSknHufBPSoASHov395eFZZatvsejg3yC4dgbbt03zLWPZuN8xZM sqm03aoNywSzz0DvJkefW0SuzeRTFIYI3yaYR4Beoz9aaP/bMvLr3Q91MJBZNjuI CBLavWYNmUca/T0EnPGK78jx6hrkE3C1U5/ik+8bDvBtTKjkDeVfnf7sI2KgkjhN 6oS5Pi5Fkuuy6R5ZL0iVR6YgiJ+CAXwRwBeOV1f7GFWFahm4abScJIfPjHUiCLUq NE6eImGO6E6U16ShrWwdDLrNZeNbb91MfjxKiNZ0reVNbOppNkOVUs4kp39HdjHA 7rHOpz22H+TaZDhEQEIXNKddZi5GalWGL7aOQgiUZwh2u+3nx7Yif4Yw7fOHAn0t 439RTeUx/v6wwMx9AIVop8HxuYrB/LGzOAtSKoxq95LasAy0Zb0= =ojJT -----END PGP SIGNATURE----- --DJawYnPHCm25NSR8--