From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=aqKRRCHh; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id ED0E15A0276 for ; Thu, 02 Jan 2025 23:00:11 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1735855210; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=bk5YuhO5IgOLrOw31HSht1xen59zaUfUzjx1qh/tSlI=; b=aqKRRCHhBVBL73pXplx+4c74jtk4rHMu96BVdZPISX2Hg3P7srm0kY7CYJ8cUPTiObxrsR ZgWXoBrQ6OdfYa3LC/eSm4BRzCbY7n5ILGENzlrZL4VW+E/hCDrY5HsDA6MzEBqKXUHNRS FmO5B/3XOCQslpmCONiF/wfR1fffrOg= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-357-_rBlT1lpM8-NcftlaxL0Fg-1; Thu, 02 Jan 2025 17:00:09 -0500 X-MC-Unique: _rBlT1lpM8-NcftlaxL0Fg-1 X-Mimecast-MFC-AGG-ID: _rBlT1lpM8-NcftlaxL0Fg Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-4361efc9dc6so65108605e9.3 for ; Thu, 02 Jan 2025 14:00:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735855207; x=1736460007; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=bk5YuhO5IgOLrOw31HSht1xen59zaUfUzjx1qh/tSlI=; b=g+hXwuhHCLTMbyUUZFyAF1VIca8QCZQDajXrd7IPMJXphZsTFbGVQJYeyCKgjI08OH LIiuzl0FHTeoANK6AnFzOjF1GiEjfaIK8oHZl6Za0pThZ8JxfQhYTHs2C5rZ0OTxmOWV kX+KLk65w4ZsAsLd8ye3qix9j0NITmxAfXSck385CrmGOYY4ueFygywDPR9IOR3qjMyO dl8Y3qfVPKKTDoKOEwTIbc0+GCamKrmBP6WxIBxZVb9QIIO0aNfEbtPPaL1846opI5FH teM62+e39FIWlsb6BcXvEqeWx6wskFjFPaWxqvNWzCRQXRmvJzPBmwn7sjfvNas6DYSc NNWw== X-Gm-Message-State: AOJu0Yy0sPEYfki6pnTnYk3jT1qeaUYj1SAg5BJKtnJozrKUlR+I4qzN fiJqR5NiiXEv7fULLS9xmE+GMMWC+00gtTI7kIjsQyhLl96Axm3xCf9GYbZxs9nmcbnWzPkdHKR PiOlIP5qm8Z+vZHzwhv5asvRfLCp4NAIChGbpzgG0vGiGZiQVYL+tWrujfQ== X-Gm-Gg: ASbGncuFxMif/Ej71F9vLm1TZh9Sj//eFbiSXP34GBoags0xHlVeUUXeR8LTdWZSwm/ vTAkfUOL7zkHa6ptlptbT6YLeM4bGt4nxRnJYIWcPY8UyPawnIoW9GvW8kpe5j41lOM++B92UtO TvEHK/oHxuVZyDjQAhQa3UKZSpXL9uyfJU/eTA88RDHANrVGPfVQ+yWF144/DNCSy4Z1FH/A5WR oVhyKba1rjLaVgUCzziQkWEeqReDGepLayvtO8DB16UTs9uNzXUhq8DhqWpncj43gAsOH6umIsc IhudNAdSWQ== X-Received: by 2002:a05:6000:1544:b0:386:3cfa:62ad with SMTP id ffacd0b85a97d-38a221e2eb3mr42611583f8f.1.1735855207020; Thu, 02 Jan 2025 14:00:07 -0800 (PST) X-Google-Smtp-Source: AGHT+IEMAxscRVAmmtbmjov8J3vNcDCnYLwGJsossOsNKTm9UmIWMHAhttUFeuHfQREJGO9G9dKmKA== X-Received: by 2002:a05:6000:1544:b0:386:3cfa:62ad with SMTP id ffacd0b85a97d-38a221e2eb3mr42611565f8f.1.1735855206588; Thu, 02 Jan 2025 14:00:06 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a1c8475cesm38853406f8f.57.2025.01.02.14.00.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Jan 2025 14:00:05 -0800 (PST) Date: Thu, 2 Jan 2025 23:00:04 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 07/12] packet: Remove unhelpful packet_get_try() macro Message-ID: <20250102230004.04825962@elisabeth> In-Reply-To: References: <20241220083535.1372523-1-david@gibson.dropbear.id.au> <20241220083535.1372523-8-david@gibson.dropbear.id.au> <20250101225437.3fc4f71b@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: hHCWxb1iACD8hAl59wVPKITMklejnN9qj3Gn4pu_Zm4_1735855208 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: JBLCDZWKD7ZM5DF3XCQIVWYSJYTN2MKG X-Message-ID-Hash: JBLCDZWKD7ZM5DF3XCQIVWYSJYTN2MKG X-MailFrom: sbrivio@redhat.com 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: On Thu, 2 Jan 2025 13:15:40 +1100 David Gibson wrote: > 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: > > > > > 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 packet > > > range, even in normal cases. So, suppressing the log messages seems like > > > it makes sense, except: > > > > > > 1) It suppresses log messages on all errors. We (somewhat) expect to hit > > > the case where the requested range is not within the received packet. > > > However, it also suppresses message in cases where the requested packet > > > 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 ever > > > actually hit them. > > > > > > 2) The suppressed messages aren't actually that disruptive. For the case > > > 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 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 transactions > > > aren't that common. Suppressing the message for this case doesn't > > > seem worth the drawbacks of (1). > > > > 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". 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. 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. > > 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. > > > > 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. Yes, I get that, and: - I would be happy if that one were *not* reported as failure - the calls before that one should always be enough to check if we have an actual issue with 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. There, I used packet_get_try() because a missing option or payload doesn't indicate a bad packet at the _data_ level. On the other hand, it's bad at the network level anyway, because option 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. > > 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. I fail to see that much of a complication as a result, and I'd still very much prefer to *not* have a warning for that case in dhcpv6_opt() (and a slight preference to have a different message for ipv6_l4hdr()), but if it really adds a ton of lines for whatever reason I'm missing, so be it... > > > Signed-off-by: David Gibson > > > --- > > > dhcpv6.c | 2 +- > > > ip.c | 2 +- > > > packet.c | 18 +++++------------- > > > packet.h | 3 --- > > > 4 files changed, 7 insertions(+), 18 deletions(-) > > > > > > 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 = sizeof(struct udphdr) + sizeof(struct msg_hdr); > > > > > > - while ((o = packet_get_try(p, 0, *offset, sizeof(*o), &left))) { > > > + while ((o = packet_get(p, 0, *offset, sizeof(*o), &left))) { > > > unsigned int opt_len = ntohs(o->l) + sizeof(*o); > > > > > > 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; > > > > > > - while ((o = packet_get_try(p, idx, offset, sizeof(*o), dlen))) { > > > + while ((o = packet_get(p, idx, offset, sizeof(*o), dlen))) { > > > nh = o->nexthdr; > > > hdrlen = (o->hdrlen + 1) * 8; > > > > > > 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; > > > > > > if (idx >= p->size || idx >= 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); > > > > 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 wrong. > > > > return NULL; > > > } > > > > > > 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; > > > } > > > > > > 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; > > > } > > > > > > 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__) > > > > > > -#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; \ -- Stefano