From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 6B80B5A026F for ; Thu, 16 Nov 2023 06:37:28 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1700113042; bh=8gKzXJVa+pJRZDfV5h4ylNEBwCJ9Qc+nWMi1wMYJYl4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SlQy8b5KrQhykrWj+Sns5qIYe11mExurXjY74HlKgXFvkRaviY1lX4KBmlUXpbczT Aua/z8YAjjC2QwNBB79un07HEsESnR19/RNuLSPhJr5NoEOpdoJQ6InddzCtHIaolA cc9f+kgiLW5LubqN/LLIVGs1Qf+7WiMTmAcyJdiM= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4SW81Z3ZcLz4wx5; Thu, 16 Nov 2023 16:37:22 +1100 (AEDT) Date: Thu, 16 Nov 2023 16:25:26 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH] cppcheck,valgrind: Don't pass NULL to recv() with MSG_TRUNC Message-ID: References: <20231115044124.1496698-1-david@gibson.dropbear.id.au> <20231115063259.2c7bc5b0@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ou0TUuJZtl6DfmBw" Content-Disposition: inline In-Reply-To: Message-ID-Hash: PVEG3N3PDRQYDSSNSYQTBFRDNJ5HOBWQ X-Message-ID-Hash: PVEG3N3PDRQYDSSNSYQTBFRDNJ5HOBWQ 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: --ou0TUuJZtl6DfmBw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 15, 2023 at 07:11:19PM +1100, David Gibson wrote: > On Wed, Nov 15, 2023 at 06:32:59AM +0100, Stefano Brivio wrote: > > On Wed, 15 Nov 2023 15:41:24 +1100 > > David Gibson wrote: > >=20 > > > Usually, of course, it's invalid to pass a NULL buffer to recv(). Ho= wever, > > > it's acceptable when using MSG_TRUNC, because that suppresses actually > > > writing to the buffer. So, we pass NULL in tcp_sock_consume(). > > >=20 > > > Unfortunately, checker tools aren't always aware of that special case= : we > > > already have a suppression for cppcheck to cover this. valgrind-3.22= =2E0 > > > (present in Fedora 39) has a similar problem, generating a spurious w= arning > > > here. > >=20 > > I haven't tried valgrind 3.22 yet, but... do you happen to know why > > test/valgrind.supp doesn't cover this anymore? >=20 > Huh.. I hadn't spotted there was an existing suppression. I don't > know why that's not working any more, I can have a closer look. >=20 > > > We could generate another suppression for valgrind, however, it so ha= ppens > > > that we already have tcp_buf_discard ready to hand. If we pass this > > > instead of NULL it makes both cppcheck and valgrind happy. We're sti= ll > > > using the MSG_TRUNC flag, the kernel doesn't actually have to copy da= ta, > > > so we should still have the performance benefits of it. > >=20 > > I'm not enthusiastic about this, because using tcp_buf_discard there > > might tell an optimising compiler that it's useful to prefetch it. > >=20 > > We would also pass the actual address of tcp_buf_discard to the kernel, > > and I'm not sure if this has further subtle implications on possible > > optimisations in the kernel implementation (even though as you said no > > data is actually copied). >=20 > Ok, fair points. I'll revisit this. Huh.. so.. this actually intersects with the stuff we discussed on the last call about whether it's a good idea to build without optimization for the valgrind tests (we currently do). So, in terms of -g, my understanding is that valgrind doesn't need debug symbols for its actual test mechanisms. But, I realised later, that it obviously does in order to identify meaningfully where the problems occurred - which also includes matching then to suppressions. So it seems the change that's caused the error for me is not in valgrind, but in the compiler. Even with -O0, the compiler in Fedora 39 is inlining tcp_sock_consume() (confirmed with objdump). Since there's no stack frame for it, valgrind doesn't match it against the suppression. I'm now testing a new spin that uses an explicit ((noinline) to fix the suppression. --=20 David Gibson | 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 --ou0TUuJZtl6DfmBw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmVVp7UACgkQzQJF27ox 2GeTsw//ZBT2BkDcgYnvHaYBPmp3yn/pNpha7Vr2XZX13exfDVLvuFtQYsxwtucB TtnCdoEfOlTk6UX670cmiQBZyegnEHYYxDifrFwBXM5G/lk+bgQzEsLiw4sH+Sug 97wtYHGZAbrT0i8u0E10gWg0Kg+9oFXpOFdP8Eixx/8CEbB+2Bdn2ljVMam+jwL4 mkasxu4IeevHZm6KgOxbJN23maxIt/YAaKOU55aUCzOIG0ZcJFp9sZf4OLSq0gtK sIofll6x+uVNm/kXCxKgKgnHT38AZg5H1kHLqjq4niWIMNrt129oTvZz5Mvl+KOD +1QTbvZD2w/oRmuw5QtZEQXB4uKkkcRwdeTPKy//Ep0Xk80Scsx+mOAxRE0O7X/C u9vGroaD3fNTt95CEUWXSKauRepTs6gNGjSrrbD7gFRQ3ffMObyJ2nO39R05seCI bcdRDtNh8I3k0yOF8SqykhvpBmH4roUL0DhZD8f7NK1ZxSQTXw1xAeOVpUFEgB0L LclPsywmThIkRWoH4V3Fe/Kdi3AcapfsEL8rO4x5PN034s4WvmVT5paGdlLr0UZC Y0o3yEqeMnhYMw7YUh9zK+8t5zJX2bm8FukEiMaBY2lE+Tpm2XcUs9By9R4uyBWL U29Tctquk5e4HId7Q6bJ23J4+s8Cvjq2PbC+S6pHqYBqzYOT2tU= =U63V -----END PGP SIGNATURE----- --ou0TUuJZtl6DfmBw--