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=T7Gg9WIC; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id DEB975A004E for ; Wed, 08 Jan 2025 06:13:15 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202412; t=1736313172; bh=uylnYcOxpgqlyN8sxkjKUrqmOverQVgDsWoCwpsM8Vs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=T7Gg9WICDD8pewLKIdiQHZJMBYrsUbvamYMqZuETjb6nTYzVIS1O1As5bV4qG5WiC eOvrxSteMjlQ1+uW+Fx8i60fyfV05tEWK4VSzH8yVU+sayhbp3NLt5p4U9LcONGeO4 AUOFJZjggxZM7NdNZOw+C0vzAWFc7HJsyNRtq4StxzFUoMJZzBEXG5l1ebgvi1bx6f gSWAXfvulpmtbA2y8PAqtK363ZrGwf3QgscqaSP41ri7JppUgbO0k4YIWikqu2k8t4 xs2FeDMhVINSH4uEdaohzWZG+3I3uU40CEqj48eaD1v4J0PN7nenGxtub9Zqb8Jj90 OqRDW8lpUf6NQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4YSbdw1z8fz4wxm; Wed, 8 Jan 2025 16:12:52 +1100 (AEDT) Date: Wed, 8 Jan 2025 15:44:09 +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> <20250102230004.04825962@elisabeth> <20250106115522.0944b44c@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qSqwfeJzCfBNwDiN" Content-Disposition: inline In-Reply-To: <20250106115522.0944b44c@elisabeth> Message-ID-Hash: UMDRDSA6ZU4ROUXFBCCRN74RNMSM3TOU X-Message-ID-Hash: UMDRDSA6ZU4ROUXFBCCRN74RNMSM3TOU 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: --qSqwfeJzCfBNwDiN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 06, 2025 at 11:55:22AM +0100, Stefano Brivio wrote: > On Fri, 3 Jan 2025 15:48:47 +1100 > David Gibson wrote: >=20 > > On Thu, Jan 02, 2025 at 11:00:04PM +0100, Stefano Brivio wrote: > > > On Thu, 2 Jan 2025 13:15:40 +1100 > > > David Gibson wrote: > > > =20 > > > > On Wed, Jan 01, 2025 at 10:54:37PM +0100, Stefano Brivio wrote: =20 > > > > > 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 pack= et_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 packet > > > > > > range, even in normal cases. So, suppressing the log messages = seems like > > > > > > it makes sense, except: > > > > > >=20 > > > > > > 1) It suppresses log messages on all errors. We (somewhat) exp= ect to hit > > > > > > the case where the requested range is not within the receive= d packet. > > > > > > However, it also suppresses message in cases where the reque= sted packet > > > > > > index doesn't exist, the requested range has a nonsensical l= ength 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 ever > > > > > > actually hit them. > > > > > >=20 > > > > > > 2) The suppressed messages aren't actually that disruptive. Fo= r the case > > > > > > in ip.c, we'd log only if we run out of IPv6 packet before r= eaching a > > > > > > (non-option) L4 header. That shouldn't be the case in norma= l operation > > > > > > 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= perfectly > > > > > > ok cases, but, again these are trace() level and DHCPv6 tran= sactions > > > > > > aren't that common. Suppressing the message for this case d= oesn't > > > > > > seem worth the drawbacks of (1). =20 > > > > >=20 > > > > > The reason why I implemented packet_get_try() is that the message= s from > > > > > packet_get_do() indicate serious issues, and if I'm debugging som= ething > > > > > by looking at traces it's not great to have messages indicating t= hat we > > > > > hit a serious issue while we're simply validating identity associ= ations. =20 > > > >=20 > > > > I'm not following your argument here. It's exactly because (most o= f) > > > > the message indicate serious issues that I don't want to suppress > > > > them. I don't know what you mean by "validating identity > > > > associations". =20 > > >=20 > > > But dhcpv6_opt() trying to get data that doesn't exist is *not* an > > > issue, including not a serious one, so if I'm debugging something with > > > --trace and I see one of these messages I'll shout at "memory" or > > > "packet" badness and waste time thinking it's an actual issue. =20 > >=20 > > Oh.. I think I see the confusion. dhcpv6_opt() trying to get data > > that's not in the packet is not an issue. dhcpv6_opt() trying to get > > data that is (theoretically) within the packet, but *not* in the > > buffer indicates something very bad has happened. The former is > > exactly one check, every other one is the second class - trying to > > separate those cases is the purpose of the later "different > > severities" patch. > >=20 > > The difficulty is that passing func=3D=3DNULL to indicate the "try" case > > doesn't work if we want to still give useful errors for the serious > > cases: we need the function name for those too. > >=20 > > I had been considering printing occasional trace level messages for > > the ok case an acceptable tradeoff for not suppressing the messages > > which are serious. But I see your case or that being too confusing > > when debugging. I did have a draft where I used an explicit boolean > > flag to enable/disable the non-serious errors, but gave up on it for > > simplicity. > >=20 > > I'll look into a way to continue suppressing the non-serious error > > here. Maybe moving the (single) non-serious error case message into > > the caller with a wrapper. >=20 > Okay, yes, thanks, that would be helpful. >=20 > > > Validating identity associations (IA_NA, IA_TA, RFC 3315) is what > > > dhcpv6_ia_notonlink() does. That's the most common case where we'll > > > routinely call dhcpv6_opt() to fetch data which isn't there. =20 > >=20 > > Ok. > >=20 > > > > > 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 of= fset > > > > > was already checked by the caller. =20 > > > >=20 > > > > 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. =20 > > >=20 > > > Yes, I get that, and: > > >=20 > > > - I would be happy if that one were *not* reported as failure =20 > >=20 > > Right, that's also my preference, but as above I compromised on this > > to simplify preserving the error cases that do matter. > >=20 > > > - the calls before that one should always be enough to check if we ha= ve > > > an actual issue with the packet =20 > >=20 > > Yes, in this case I think that's correct. > >=20 > > > > > In ipv6_l4hdr(), the index was > > > > > already validated by a previous call to packet_get(), and the sta= rting > > > > > offset as well. =20 > > > >=20 > > > > Not AFAICT, the initial packet_get just validates the basic IPv6 > > > > header. The calls to packet_get_try() in the loop validate additio= nal > > > > 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. > > > >=20 > > > > 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. = =20 > > >=20 > > > There, I used packet_get_try() because a missing option or payload > > > doesn't indicate a bad packet at the _data_ level. =20 > >=20 > > Not really sure what you mean by the data level, here. >=20 > I meant that in the sense of Layer-2: the packet is fine at Layer 2 in > the sense that there's no missing data, but it's bad at Layer-3 level > because IPv6 doesn't allow a missing next-option. Oh, right. That's true, but in this case the caller is code dealing with L3 structure, so it seems reasonable a failure should give an error. > > > On the other hand, it's bad at the network level anyway, because opti= on > > > 59 *must* be there otherwise (I just realised), so while I'd still > > > prefer another wording of the warning (not mentioning packet/buffer > > > ranges... something more network-y), I would be fine with it. =20 > >=20 > > That sounds like another argument for moving the message for the > > "requested range is outside packet" case into the caller. >=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 --qSqwfeJzCfBNwDiN Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmd+ApgACgkQzQJF27ox 2Gestw/9G+FWICzQYGJEnFAW9CRvi1YPRCnI8WJlHHV30P54DXaTLOGSOkMqzVgb RZed9wjWYq+io87vgZFVGRh0a0KDPYMfS5BbzhHUEvy6pkkWaaM4JN+igoM/gD7o uApOhijuZSsKpBUTA4MFyImx53z1rY9cDr5Z7sxKn88MV3/CUqL6nmiJVvuxgzns 9YZyscjAxQ/A88VhPH6o3G1//TV+bps4bmTV1PTmk0GHKMHVniJ2ikRkdQ0Qu0S6 Ui9IEjks5Q/LnkyKiQLURtvCYhUrWUnEZbUWBeGHokYczo2fQv1eZjCqssrOqU1O KGHTOKYEI0Pvonsxn3yQ5Mde4jaPO3nPBBg2XDiUrSOo81x4TYFMonR48jBjIvFD wu40EuIQG8qNV5e6zo7rB+lBjLaMkn2fyNYMu0cLnK+noLFa++GPcmIhjddcqiTa kE7R8XGt7I+u8rMEPNeqSGc/ox0cqVLJXeoP6r4a0VWOKMMx3Qv4NZPavjOu8xK3 z3Yzd6aFXZ9JRetD0+CiNX6XfetsH5nVmvO9Km/8bVKb4CauQrWmWlXY9RwMsLK7 MggaUBbrVdBm8A5arweFndm/rjrVdfq0yIOJtWYT3Xh86+0P2KmrQofT4bA6layx u3cAwUwRkJy96UWOmuqpMQ4TcHV1brZQ3e/cHq/enstALWshIPI= =HV2C -----END PGP SIGNATURE----- --qSqwfeJzCfBNwDiN--