From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202412 header.b=XXW0xBKm; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id B57725A0276 for ; Sat, 21 Dec 2024 08:21:16 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202412; t=1734765667; bh=yPlDA2GkFQ+R88aDmP3GqPZCJxo7vGgDZAR8vChzU9M=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XXW0xBKmZFDPZuD8y3+HFqbR8axVlt7kqgFgYzQccv6GXGHuMSFtwvyi2IYYbxG1f +UHxJmR62e8qFJBT+DhXrg1yqCbtbF565CeSE6BRC0f155HW/o3u6V6FnUOvA6Ws5Y yo5IUYES4nXEZcjerXwvWLi12LHZc884ozPQSlx3Z+DOVnP1lkVx58V9EPf5XsWck9 qiMsi3PJ4tI7hWseubvgxbtcSppTLh0vEBDILxyah7hGgUeZ0FdMUJJAiGRR9PJE9i RGOMwr0LpvRLa/vhoA0xzPomN6d7nIvLlPw+2A5u3KfBDnFUrVRx6Gw+y/GP28f8T9 Fvv+QojMXdH+g== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4YFbLC2qbyz4x11; Sat, 21 Dec 2024 18:21:07 +1100 (AEDT) Date: Sat, 21 Dec 2024 17:59:59 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 2/3] packet: Don't have struct pool specify its buffer Message-ID: References: <20241213120156.4123972-1-david@gibson.dropbear.id.au> <20241213120156.4123972-3-david@gibson.dropbear.id.au> <20241219100011.5faed9fd@elisabeth> <20241220105133.6f6ee3d6@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yUTerA0DZXQXEc4w" Content-Disposition: inline In-Reply-To: <20241220105133.6f6ee3d6@elisabeth> Message-ID-Hash: B54TPDPFLY7IYCGFNX6FKDJD3DI4MN5B X-Message-ID-Hash: B54TPDPFLY7IYCGFNX6FKDJD3DI4MN5B 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: --yUTerA0DZXQXEc4w Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Dec 20, 2024 at 10:51:33AM +0100, Stefano Brivio wrote: > On Fri, 20 Dec 2024 11:59:09 +1100 > David Gibson wrote: >=20 > > On Thu, Dec 19, 2024 at 10:00:11AM +0100, Stefano Brivio wrote: > > > On Fri, 13 Dec 2024 23:01:55 +1100 > > > David Gibson wrote: > > > =20 > > > > struct pool, which represents a batch of packets includes values gi= ving > > > > the buffer in which all the packets lie - or for vhost_user a link = to the > > > > vu_dev_region array in which the packets sit. Originally that made= sense > > > > because we stored each packet as an offset and length within that b= uffer. > > > >=20 > > > > However dd143e389 ("packet: replace struct desc by struct iovec") r= eplaced > > > > the offset and length with a struct iovec which can directly refere= nce a > > > > packet anywhere in memory. This means we no longer need the buffer > > > > reference to interpret packets from the pool. So there's really no= need > > > > to check where the packet sits. We can remove the buf reference an= d all > > > > checks associated with it. As a bonus this removes the special cas= e for > > > > vhost-user. > > > >=20 > > > > Similarly the old representation used a 16-bit length, so there wer= e some > > > > checks that packets didn't exceed that. That's also no longer nece= ssary > > > > with the struct iovec which uses a size_t length. > > > >=20 > > > > I think under an unlikely set of circumstances it might have been p= ossible > > > > to hit that 16-bit limit for a legitimate packet: other parts of th= e code > > > > place a limit of 65535 bytes on the L2 frame, however that doesn't = include > > > > the length tag used by the qemu socket protocol. That tag *is* inc= luded in > > > > the packet as stored in the pool, however, meaning we could get a 6= 5539 > > > > byte packet at this level. =20 > > >=20 > > > As I mentioned in the call on Monday: sure, we need to fix this, but = at > > > the same time I'm not quite convinced that it's a good idea to drop a= ll > > > these sanity checks. > > >=20 > > > Even if they're not based on offsets anymore, I think it's still > > > valuable to ensure that the packets are not exactly _anywhere_ in > > > memory, but only where we expect them to be. > > >=20 > > > If it's doable, I would rather keep these checks, and change the ones > > > on the length to allow a maximum value of 65539 bytes. I mean, there's > > > a big difference between 65539 and, say, 4294967296. =20 > >=20 > > Right, I have draft patches that do basically this. > >=20 > > > By the way, I haven't checked what happens with MTUs slightly bigger > > > than 65520 bytes: virtio-net (at least with QEMU) doesn't budge if I > > > set more than 65520, but I didn't actually send big packets. I'll try > > > to have a look (also with muvm) unless you already checked. =20 > >=20 > > I'm not sure what you mean by "doesn't budge". No, I haven't checked > > with either qemu or muvm. There could of course be limits applied by > > either VMM, or by the guest virtio-net driver. >=20 > Oh, sorry, I was deep in the perspective of trying to make things > crash... and it didn't do anything, just accepted the setting and kept > sending packets out. Right. Even without my packet pool changes, I'm not aware of a way to make it crash, just ways to cause packets to be dropped when they shouldn't. > Let me try that then, with and without your new series... >=20 --=20 David Gibson (he or they) | 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 --yUTerA0DZXQXEc4w Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmdmZ2QACgkQzQJF27ox 2Ge0RxAAkMpL2cY6t/3DpL58isr+5/Ku8On3bL/GU+h4uIeAb6RTu8V5zhKGk/7I Q2J76ar8UIpyLdr0w0K9yr0X2jrM7wyV3nZ+t7wdFa/fJGtLKkAQLTRhOXGn2un+ wotGL5FP9U85jmabDHQX3EMbugBww1BOMIw0yJWiSKxHacSJUVbEm6dStdZy2tWi X/zBIsz4KUykuhc2fQroz1K0o/9ZHaDB3QomOhWDMDPd6QB9LN461GB7rPq8ZCJ1 dj8OmDdmEqF1/KgfqNyTdPjd0EfnGzg+tEGrHH1uMuSjZ7TzKzWooOepgqRZonhD svr5oEeDe2BRWowB5LK4Ojs0giL8kONaWdBGJ3CtuNy22nETivwsk5I7OAag6+2K b51PaoM/jp1BiNBprK5xyvftyhhJlzUSCTgVYEX0JeUtwyQfd4SzGSQFQLtMAJO+ j+BDDzJqt1HPAFo1Z2omFowKTCnIpA4PXtHAqoU0OFkrh3AiWyR5MfmvjZtexdUa vvS5AvLK6F1brWKAhZCCWLRjWPmpgtUZ0YVFRxbaRqFvqFTojbUQHHOD9wZQTQ9r 991zKR/ZencPR+9SwGFuilTcwo+1EAjGizFUy7dcE37o7iPqT6XtrtogZnprC3Og AUZBl98Rvv/oNYr6ULjIMEncjE96mHQK6AqwwKHNtdlVwMdNO/Y= =O33r -----END PGP SIGNATURE----- --yUTerA0DZXQXEc4w--