From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 9177A5A0320 for ; Fri, 05 Jul 2024 04:07:40 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1720145250; bh=pIskTqQukpd2jJL8hEsXGsMODhy9wwYYDxe60rF1BO4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SkxcbtPmqy8+tM8frR1MBL9Mod/o7vXB7YRaP+GvJiG4K9NuuheJAwabCL3Aurj/c r/psJLAesv5EW/halZA8EHVwCC0CqnS/rWEO/hXo6QCHHNzxgHg3wXLU/60fZy3Ssi Yq0kY9qyE8q0XHs/tyA+ee8/WzqFk95pq9ZTEQyo4y2Zjgjk1LswnsssolGcUhT0/Y VB85pGz8qBsKOVYnyvJ0e3cnbDBA7tS/wDrgffE88l7vFYR+s2euL3twkuQF+4+EgM SdDKahXB5GiXiPDMvCwCKSSSI2KMXPr5PuIg0gFTpqV8hRJ4V+J9slp7VV9Fc3+37s i8tjTyyjXw4jg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4WFcNL5w4Yz4wc1; Fri, 5 Jul 2024 12:07:30 +1000 (AEST) Date: Fri, 5 Jul 2024 11:28:07 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH 1/5] packet: replace struct desc by struct iovec Message-ID: References: <20240621145640.1914287-1-lvivier@redhat.com> <20240621145640.1914287-2-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Flqu+dbEJMfEmSIi" Content-Disposition: inline In-Reply-To: Message-ID-Hash: VWDDNPR5VWUKJ2RES26MBGYERT4U2A6B X-Message-ID-Hash: VWDDNPR5VWUKJ2RES26MBGYERT4U2A6B 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: --Flqu+dbEJMfEmSIi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jul 04, 2024 at 05:52:09PM +0200, Laurent Vivier wrote: > On 24/06/2024 04:48, David Gibson wrote: > > On Fri, Jun 21, 2024 at 04:56:36PM +0200, Laurent Vivier wrote: > >=20 > > Needs a commit message. > >=20 > > > Signed-off-by: Laurent Vivier > > > --- > > > packet.c | 75 +++++++++++++++++++++++++++++++----------------------= --- > > > packet.h | 14 ++--------- > > > 2 files changed, 43 insertions(+), 46 deletions(-) > > >=20 > > > diff --git a/packet.c b/packet.c > > > index ccfc84607709..af2a539a1794 100644 > > > --- a/packet.c > > > +++ b/packet.c > ... > > > + } > > > + > > > + if (start + len + offset > p->buf + p->buf_size) { > >=20 > > Also pre-existing, but I wonder if we should check for overflow of > > (Start + len + offset). >=20 > Originally, I didn't want to change the existing behaviour. Only to move > code, and to use a common function for packet_add_do() and packet_get_do(= ). > But if you think it should be better I can update the code for that: Well, I think we should be more careful here, but as you say I don't think it necessarily belongs as part of this series. > > > + if (func) { > > > + trace("packet offset plus length %lu from size %lu, " > > > + "%s:%i", start - p->buf + len + offset, > > > + p->buf_size, func, line); > > > + } > > > + return -1; > > > + } > > > + > > > +#if UINTPTR_MAX =3D=3D UINT64_MAX > > > + if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) { > >=20 > > I don't think this check is relevant any more if we're going to iovecs > > - this was just because the offset in struct desc was only 32-bit. >=20 > I agree. >=20 > >=20 > > > + trace("add packet start %p, buffer start %p, %s:%i", > > > + (void *)start, (void *)p->buf, func, line); > > > + return -1; > > > + } > > > +#endif > > > + > > > + return 0; > > > +} > > > /** > > > * packet_add_do() - Add data as packet descriptor to given pool > > > * @p: Existing pool > > > @@ -41,34 +71,16 @@ void packet_add_do(struct pool *p, size_t len, co= nst char *start, > > > return; > > > } > > > - if (start < p->buf) { > > > - trace("add packet start %p before buffer start %p, %s:%i", > > > - (void *)start, (void *)p->buf, func, line); > > > + if (packet_check_range(p, 0, len, start, func, line)) > > > return; > > > - } > > > - > > > - if (start + len > p->buf + p->buf_size) { > > > - trace("add packet start %p, length: %zu, buffer end %p, %s:%i", > > > - (void *)start, len, (void *)(p->buf + p->buf_size), > > > - func, line); > > > - return; > > > - } > > > if (len > UINT16_MAX) { > > > trace("add packet length %zu, %s:%i", len, func, line); > > > return; > > > } > > > -#if UINTPTR_MAX =3D=3D UINT64_MAX > > > - if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) { > > > - trace("add packet start %p, buffer start %p, %s:%i", > > > - (void *)start, (void *)p->buf, func, line); > > > - return; > > > - } > > > -#endif > > > - > > > - p->pkt[idx].offset =3D start - p->buf; > > > - p->pkt[idx].len =3D len; > > > + p->pkt[idx].iov_base =3D (void *)start; > > > + p->pkt[idx].iov_len =3D len; > > > p->count++; > > > } > > > @@ -104,28 +116,23 @@ void *packet_get_do(const struct pool *p, size_= t idx, size_t offset, > > > return NULL; > > > } > > > - if (p->pkt[idx].offset + len + offset > p->buf_size) { > > > + if (len + offset > p->pkt[idx].iov_len) { > > > if (func) { > > > - trace("packet offset plus length %zu from size %zu, " > > > - "%s:%i", p->pkt[idx].offset + len + offset, > > > - p->buf_size, func, line); > > > + trace("data length %zu, offset %zu from length %zu, " > > > + "%s:%i", len, offset, p->pkt[idx].iov_len, > > > + func, line); > >=20 > > I'm not sure either the old or new message is particularly descriptive > > here :/ >=20 > I think the func and line parameters will help to understand the problem, > and the others why the trace is triggered. Hmm, yeah, I guess so. > >=20 > > > } > > > return NULL; > > > } > > > - if (len + offset > p->pkt[idx].len) { > > > - if (func) { > > > - trace("data length %zu, offset %zu from length %u, " > > > - "%s:%i", len, offset, p->pkt[idx].len, > > > - func, line); > > > - } > > > + if (packet_check_range(p, offset, len, p->pkt[idx].iov_base, > > > + func, line)) > >=20 > > Ah.. right.. in this case we certainly don't want ASSERT()s in > > packet_check_range(). Still wonder if that would make more sense for > > the packet add case, however. > >=20 > > A couple of other points: > > * You've effectively switched the order of the two different tests he= re > > (one range checking against the entire buffer, one range checking > > against a single packet). Any reason for that? >=20 > The idea is to check the parameters are valid before checking the buffer = is valid. Ok, makes sense. > > * Do we actually need the entire-buffer check here on the _get() > > side? Isn't it enough to ensure that packets lie within the buffer > > when they're inserted? Pre-existing, again, AFAICT. >=20 > I wanted to keep the idea introduced in bb708111833e ("treewide: Packet > abstraction with mandatory boundary checks") and checking we don't read > outside of the buffer. Hm, ok. --=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 --Flqu+dbEJMfEmSIi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmaHTCYACgkQzQJF27ox 2Gc2ow/6AnSFkY2gRNrB9VizIt71aiQl0WM7967kYviwHN7GT/ef2UizBXsLBEKC M+uoTmFM8gQs+t+fQlEuH8w0jb1xOs8ofoRS0BsVvHr0eaAWDbny8qFQWMQ0paF+ 3oiJGct793G8Pl00Wnibb7k9uhnhcxdFEJs/erm94pBybGihZvbpMRCMyFtWefxv WUJyD2QoLLPlpn67mQeTQjUnp96SBPWOMAXs6tktmmwIKtPtSZZvj9TzH9LswAaJ EjphZeZ9D0SJnuYfwo++BRKjvTqkIv6c34hf0LQ/pu4iAb24e6UyG9lyc3mMxOnG mQrc+N9yBu33DqqlpNn5IyrBL0iOZJT0CbhV8A2l03O4fF4e1JySrFYMMjNzm9pG 0mFoiq91tIa6F7A6/mas2s9/d537tlwkIr8U31UD2isWxhBh3oZaLHy3Y+UezcY2 jWl5tWS7tV7KoJ9ANlRicdHOy4FECV8xbnbn/RxQXg6BcTvarJTU9JXxe5k/vDJ5 wJBwUH+S20yhXdRIEr3rBG0VO3675NoxuDPon4ShT3XLsUDbukYuS5hz+5fygGlu B2PHZegJJdFKAIiMM4gBLmd6oOBfwpJ1lvBJYBBRRSTb+FR5gk2ZMgBkMnnUAA1V j3GtkHH4UnfkBU9HbGNNqIOshWDgSr4D1NZcVuUi++Vwif34hn4= =xq5S -----END PGP SIGNATURE----- --Flqu+dbEJMfEmSIi--