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=PuVI56Kb; 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 0CAFC5A061C for ; Mon, 06 Jan 2025 11:55:28 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736160928; 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=ylKeFIB//XVVMQ30OCcahgriaRxDIsXq8Ygw94jlndg=; b=PuVI56KbM2zDMIGl3uq/bHhjcZPg/zAgeO2/wTYLWJVNwyYms008MJ3+WRoYutoymI019y aFeyrHJz8TxyduHdc2vTJU/StH1Z+Pz5VrvLmmMoTDwG0Lckp9TWr+5SM50OB9boKgMfqN HxExKugiXGNJyI9D9SwzdZCwmKzKm1A= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-193-wXpXa3zeN9mTnbvoyKYiIw-1; Mon, 06 Jan 2025 05:55:26 -0500 X-MC-Unique: wXpXa3zeN9mTnbvoyKYiIw-1 X-Mimecast-MFC-AGG-ID: wXpXa3zeN9mTnbvoyKYiIw Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-385e00ebb16so3230142f8f.3 for ; Mon, 06 Jan 2025 02:55:26 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736160925; x=1736765725; 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=ylKeFIB//XVVMQ30OCcahgriaRxDIsXq8Ygw94jlndg=; b=jTqgMIEWn7JauzlyB+bXsCSy8JM5fYPeuJOkTd+K7hSc96zblr4BmrgUQRh8RHp2Ip b+qTlRl2Emtj2kZ8Ir6rQne98kK9Q8IO588CNOk/X1BVY1EnOtxuxari+CWGrCgxqxdq Yqr2/kQO5L2lp7fgxjHlK+VApk6GEMgFDcs4K6rdz4vjZhZ6oO7d4vwx+npccq+o8fn2 wtn/qtj640FYmruKW/Bfon/SgYohvA1Py964iqhkBTdGA0W+dDFDT5uvyU13MxYDyXpz RHssql3wCrp6wOuJacixyJprgS1WLBu18MF2uvl85wslUxpPGI3ESwN9gFE0lhOBR9zY ZV7Q== X-Gm-Message-State: AOJu0YwMzbIg0H43mn3YpTcwnfuVowJ+6CY+t5apfNtYvIPvA9DyxE6s CKZcLd3if/3ftFYoF3A4axbJhhilN5hv347ZPOr2KYwf8mS2nHAE3aRXPnwr5uEUWFmteZl3fSQ yxhhzvtO4dmmbBTU4rf5ez916f7KtgZbbFxvi8Jw7VRM8PHBYckSUxOAbFg== X-Gm-Gg: ASbGnctXOBVYf8mzwl/CIwiYxoh/ysRMOWUlTYFnMQNAsL9dyao0TAG17KfO17EviLg xVmElI3IcQ9k7sNLgDPVKM+grxQill5RK1VWraHtMEUrKuqEivbawCR6ImDlV7aveqZ9+697hzh EeVglPe0M99M0DVWls1j/425Y1OkS2Zh0O7XP16QsNg3p11jvrIb6eSBo4V3Sj8GTQ5/0uZzS5Q swe/Fo6yEwgLwxgwn/P3Ruyj/jJOIixct0cZ5Fzvn50hZlXegtqrirQoUts78mZRo+l78OVveCm nKORTmeByw== X-Received: by 2002:a5d:5f4a:0:b0:385:f19f:5a8f with SMTP id ffacd0b85a97d-38a221f2d90mr43461450f8f.4.1736160924969; Mon, 06 Jan 2025 02:55:24 -0800 (PST) X-Google-Smtp-Source: AGHT+IFxigxpmVMLmwsI+FpEJCfEpoXwNaGPsza06LsA+Do3JDVXt9kBopxXVySKYc0WYlbeWTlJeA== X-Received: by 2002:a5d:5f4a:0:b0:385:f19f:5a8f with SMTP id ffacd0b85a97d-38a221f2d90mr43461429f8f.4.1736160924487; Mon, 06 Jan 2025 02:55:24 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-436604e9c2csm565118905e9.43.2025.01.06.02.55.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jan 2025 02:55:23 -0800 (PST) Date: Mon, 6 Jan 2025 11:55:22 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 07/12] packet: Remove unhelpful packet_get_try() macro Message-ID: <20250106115522.0944b44c@elisabeth> In-Reply-To: References: <20241220083535.1372523-1-david@gibson.dropbear.id.au> <20241220083535.1372523-8-david@gibson.dropbear.id.au> <20250101225437.3fc4f71b@elisabeth> <20250102230004.04825962@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: KQ3WVikYfEocjubMmGRlRMb3mbAsuD3oplxaw9RcW9E_1736160926 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: CW24KJ3KERUJQZ4KV3EKK2TGEQLQNX63 X-Message-ID-Hash: CW24KJ3KERUJQZ4KV3EKK2TGEQLQNX63 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 Fri, 3 Jan 2025 15:48:47 +1100 David Gibson wrote: > 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: > > > > > 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. > > 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. > > The difficulty is that passing func==NULL 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. > > 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. > > 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. Okay, yes, thanks, that would be helpful. > > 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. > > Ok. > > > > > 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 > > Right, that's also my preference, but as above I compromised on this > to simplify preserving the error cases that do matter. > > > - the calls before that one should always be enough to check if we have > > an actual issue with the packet > > Yes, in this case I think that's correct. > > > > > 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. > > Not really sure what you mean by the data level, here. 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. > > 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. > > That sounds like another argument for moving the message for the > "requested range is outside packet" case into the caller. -- Stefano