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=fZXtylho; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 1C52D5A0272 for ; Thu, 02 Jan 2025 04:47:03 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202412; t=1735789606; bh=dRNr9UBXD+sS/bAUVVZuqFYzgQlCJpcTZVEGbU19yMc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fZXtylho9UtapxMwiV4IGqIIe567zgS8DgKic84JbMzaAwYMDJBof4/M+r5sn9VnM vygLb8MvGKZp3O7u81M0V1v42iAzrgpfwxMrUoWFLE2ofuvPYYUZr/JWL+hDLwH32b 3TZS4mGOrip2jZqoXtx4DatKP6lEoN3JqYIQRQ0wviAySejkXyBvr7F7QRW+Q6u8Hp XAsf8zremn5lLzyJrO+X9rLFTfUdSLTsbD/6hlkOe/Vk1CzPxIp7ep+Wu6zJzi2S6e /g+sYcsqksNFn+GXUFfD7ld89sMAh9q6QRoM0JpaD2mvhQDlWJsVq2hVQi1KyICbJe ACrT2iCGKigqA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4YNt1L6wW8z4xTy; Thu, 2 Jan 2025 14:46:46 +1100 (AEDT) Date: Thu, 2 Jan 2025 13:15:40 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 07/12] packet: Remove unhelpful packet_get_try() macro Message-ID: References: <20241220083535.1372523-1-david@gibson.dropbear.id.au> <20241220083535.1372523-8-david@gibson.dropbear.id.au> <20250101225437.3fc4f71b@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="J6TJG5k2h3Gs9aFu" Content-Disposition: inline In-Reply-To: <20250101225437.3fc4f71b@elisabeth> Message-ID-Hash: DAJVWHIOQM6OAAXAQIHBCCFJIYX3X5QL X-Message-ID-Hash: DAJVWHIOQM6OAAXAQIHBCCFJIYX3X5QL 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: --J6TJG5k2h3Gs9aFu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jan 01, 2025 at 10:54:37PM +0100, Stefano Brivio wrote: > On Fri, 20 Dec 2024 19:35:30 +1100 > David Gibson wrote: >=20 > > Two places in the code use the packet_get_try() variant on packet_get(). > > The difference is that packet_get_try() passes a NULL 'func' to > > packet_get_do(), which suppresses log messages. The places we use this > > are ones where we expect to sometimes have a failure retreiving the pac= ket > > range, even in normal cases. So, suppressing the log messages seems li= ke > > it makes sense, except: > >=20 > > 1) It suppresses log messages on all errors. We (somewhat) expect to h= it > > the case where the requested range is not within the received packet. > > However, it also suppresses message in cases where the requested pac= ket > > index doesn't exist, the requested range has a nonsensical length or > > doesn't lie in even the right vague part of memory. None of those > > latter cases are expected, and the message would be helpful if we ev= er > > actually hit them. > >=20 > > 2) The suppressed messages aren't actually that disruptive. For the ca= se > > in ip.c, we'd log only if we run out of IPv6 packet before reaching a > > (non-option) L4 header. That shouldn't be the case in normal operat= ion > > so getting a message (at trave level) is not unreasonable. > > For the case in dhcpv6.c we do suppress a message every time we look= for > > but don't find a specific DHCPv6 option. That can happen in perfect= ly > > ok cases, but, again these are trace() level and DHCPv6 transactions > > aren't that common. Suppressing the message for this case doesn't > > seem worth the drawbacks of (1). >=20 > The reason why I implemented packet_get_try() is that the messages from > packet_get_do() indicate serious issues, and if I'm debugging something > by looking at traces it's not great to have messages indicating that we > hit a serious issue while we're simply validating identity associations. I'm not following your argument here. It's exactly because (most of) the message indicate serious issues that I don't want to suppress them. I don't know what you mean by "validating identity associations". > It's not about the amount of logged messages, it's about the type of > message being logged and the distracting noise possibly resulting in a > substantial time waste. >=20 > About 1): dhcpv6_opt() always picks pool index 0, and the base offset > was already checked by the caller. Right, but dhcpv6_opt() is called in a loop, that only stops when it returns NULL. So, by construction the last call to dhcpv6_opt(), which terminates the loop, _will_ have a failing call to packet_get(). At this point - at least assuming a correctly constructed packet - the offset will point to just past the last option, which should be exactly at the end of the packet. > In ipv6_l4hdr(), the index was > already validated by a previous call to packet_get(), and the starting > offset as well. Not AFAICT, the initial packet_get just validates the basic IPv6 header. The calls to packet_get_try() in the loop validate additional IP options. I don't think it will ever fail on a well-constructed packet, but it could on a bogus (or truncated) packet, where the nexthdr field indicates an option that's actually missing. This is kind of my point: it will only trip on a badly constructed packet, in which case I don't think we want to suppress messages. > I guess the main reason to have this patch in this series is to make a > later change simpler, but I'm not sure which one, so I don't know what > to suggest as an alternative. Mostly the next one - making more distinction between the different error severities that can occur here. Having to deal with the possibility of func being NULL complicates things. > > Signed-off-by: David Gibson > > --- > > dhcpv6.c | 2 +- > > ip.c | 2 +- > > packet.c | 18 +++++------------- > > packet.h | 3 --- > > 4 files changed, 7 insertions(+), 18 deletions(-) > >=20 > > diff --git a/dhcpv6.c b/dhcpv6.c > > index 0523bba8..c0d05131 100644 > > --- a/dhcpv6.c > > +++ b/dhcpv6.c > > @@ -271,7 +271,7 @@ static struct opt_hdr *dhcpv6_opt(const struct pool= *p, size_t *offset, > > if (!*offset) > > *offset =3D sizeof(struct udphdr) + sizeof(struct msg_hdr); > > =20 > > - while ((o =3D packet_get_try(p, 0, *offset, sizeof(*o), &left))) { > > + while ((o =3D packet_get(p, 0, *offset, sizeof(*o), &left))) { > > unsigned int opt_len =3D ntohs(o->l) + sizeof(*o); > > =20 > > if (ntohs(o->l) > left) > > diff --git a/ip.c b/ip.c > > index 2cc7f654..e391f81b 100644 > > --- a/ip.c > > +++ b/ip.c > > @@ -51,7 +51,7 @@ char *ipv6_l4hdr(const struct pool *p, int idx, size_= t offset, uint8_t *proto, > > if (!IPV6_NH_OPT(nh)) > > goto found; > > =20 > > - while ((o =3D packet_get_try(p, idx, offset, sizeof(*o), dlen))) { > > + while ((o =3D packet_get(p, idx, offset, sizeof(*o), dlen))) { > > nh =3D o->nexthdr; > > hdrlen =3D (o->hdrlen + 1) * 8; > > =20 > > diff --git a/packet.c b/packet.c > > index bcac0375..c921aa15 100644 > > --- a/packet.c > > +++ b/packet.c > > @@ -112,27 +112,19 @@ void *packet_get_do(const struct pool *p, size_t = idx, size_t offset, > > char *ptr; > > =20 > > if (idx >=3D p->size || idx >=3D p->count) { > > - if (func) { > > - trace("packet %zu from pool size: %zu, count: %zu, " > > - "%s:%i", idx, p->size, p->count, func, line); > > - } > > + trace("packet %zu from pool size: %zu, count: %zu, %s:%i", > > + idx, p->size, p->count, func, line); >=20 > If you change this, the documentation of the arguments for > packet_get_do() should be updated as well. Fixed. The doc for packet_add_do() also changed, where it was already wron= g. > > return NULL; > > } > > =20 > > if (len > PACKET_MAX_LEN) { > > - if (func) { > > - trace("packet data length %zu, %s:%i", > > - len, func, line); > > - } > > + trace("packet data length %zu, %s:%i", len, func, line); > > return NULL; > > } > > =20 > > if (len + offset > p->pkt[idx].iov_len) { > > - if (func) { > > - trace("data length %zu, offset %zu from length %zu, " > > - "%s:%i", len, offset, p->pkt[idx].iov_len, > > - func, line); > > - } > > + trace("data length %zu, offset %zu from length %zu, %s:%i", > > + len, offset, p->pkt[idx].iov_len, func, line); > > return NULL; > > } > > =20 > > diff --git a/packet.h b/packet.h > > index 05d37695..f95cda08 100644 > > --- a/packet.h > > +++ b/packet.h > > @@ -45,9 +45,6 @@ void pool_flush(struct pool *p); > > #define packet_get(p, idx, offset, len, left) \ > > packet_get_do(p, idx, offset, len, left, __func__, __LINE__) > > =20 > > -#define packet_get_try(p, idx, offset, len, left) \ > > - packet_get_do(p, idx, offset, len, left, NULL, 0) > > - > > #define PACKET_POOL_DECL(_name, _size, _buf) \ > > struct _name ## _t { \ > > char *buf; \ >=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 --J6TJG5k2h3Gs9aFu Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmd19ssACgkQzQJF27ox 2GcYPw//aW5QXOxtOcQt82v/N135Vx7KTQBqDERhmw8e7XBVQ4j/G+bDd08EX8/g so0hqcJRl9k75+NPcioZxi5CxQAJQwQ2/5xBeTzlFdFRj6iGNfL1GjZaTe9s6t3g cadf7LDv9/+g8Q/oyAwylV/3HewTAkLRWVKT+3ZtktF/XR0nPRvxIOh0raNMN9P3 DkC6+DNVVbDwabRCw/rjjTwTHZ9NUCe0sKdhhcd/mFBT55dSR6YeTNHajttO/YHL k2rbLDZ+OPA4o/pPvJk3wyPIv0m5bHgQb2SbWSga34MDXoVJdzzAGmg0QWgjEjYf WL0+19u6u6pH0L9N18RJYNHCtqfEDwsMoYIC7BWrQvlRodZae1mgUDQdRN43WXBK fBfLy9AwInversoG9zQ8yEigBQTRbxGssYMUUKdgyXf1BFyI9DOds6MO9kPEB9F8 va9IAhOczFbpKOa+u4Ween69FV8wO0SA5j3K8OfzCkq2HlcjOavNgq9GgzClkBaw mPFFboyqt/jJ2jn033hjhiT63kLyaj9jkGOJ0rx3Pvg5Jon87CO4lhyxFSpX00t/ XQVoCfpJq/tGiF0qnyMyZM7Nani5aPn7wJr4sz/aDas+Ea2/ah7xFLXWr8kgEJ95 E74zrnrmC24ZQ920pzwgxynuAqVvnwBog7k86/mGIjS+sTFwP0M= =nr53 -----END PGP SIGNATURE----- --J6TJG5k2h3Gs9aFu--