Re: [PATCH v1 3/4] perf genelf: Remove libcrypto dependency and use sha1 utils

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Thu, May 22, 2025 at 02:05:17PM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, May 21, 2025 at 03:53:06PM -0700, Yuzhuo Jing wrote:
> > genelf is the only file in perf that depends on libcrypto (or openssl)
> > which only calculates a Build ID (SHA1, MD5, or URANDOM).  SHA1 was
> > expected to be the default option, but MD5 was used by default due to
> > previous issues when linking against Java.  This commit switches genelf
> > to use in-house SHA1 utils, and also removes MD5 and URANDOM options
> > since we have a reliable SHA1 implementation to rely on.  It passes the
> > tools/perf/tests/shell/test_java_symbol.sh test.

> Cool, I was going to ask if there was some 'perf test' this could be
> tested with, there is, good.

I applied the series locally and while testing on a new machine I
noticed some issues with the existing test_java_symbol.sh test, namely:

root@number:~# perf test -vvvv "Test java symbol"
105: Test java symbol:
--- start ---
test child forked, pid 85204
skip: no jshell, install JDK
---- end(-2) ----
105: Test java symbol                                                : Skip
root@number:~# 

So I had to re-figure out where that thing was, after I installed
java-latest-openjdk-devel the test passed, so I'll write a follow up
patch that will add a the reason for the skip suggesting that the needed
devel file be installed.

Also using -vvv:

root@number:~# perf test "Test java symbol"
105: Test java symbol                                                : Ok
root@number:~# perf test -vvvvv "Test java symbol"
105: Test java symbol:
--- start ---
test child forked, pid 86083
jshell: jvmti: jitdump in /root/.debug/jit/java-jit-20250522.XXYVwxbq/jit-86090.dump
-> int fib(int x) {
>>      return x > 1 ? fib(x - 2) + fib(x - 1) : 1;
>>     -> -> -> -> -> -> 143
-> [ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.436 MB /tmp/__perf_test.perf.data.zt9Jv (10113 samples) ]
Cleaning up files...
---- end(0) ----
105: Test java symbol                                                : Ok
root@number:~# 
root@number:~# ls -la /root/.debug/jit/java-jit-20250522.XXYVwxbq/jit-86090.dump
-rw-r--r--. 1 root root 13065557 May 22 14:18 /root/.debug/jit/java-jit-20250522.XXYVwxbq/jit-86090.dump
root@number:~#

It creates that jit-86090.dump file but leaves it in the ~/.debug/
directory, polluting it, it should remove it at the test exit.

None are issues introduced by your patch set, so should be fixed as
followup patches that I'll do after processing more outstanding patches
unless someone beats me to it ;-)

I'm keeping your patches in my local repo to give some more time for
reviewing, they will go thru container tests in lots of distros
meanwhile.

Thanks,

- Arnaldo
 
> - Arnaldo
>  
> > Signed-off-by: Yuzhuo Jing <yuzhuo@google.com>
> > ---
> >  tools/perf/util/genelf.c | 72 ++++------------------------------------
> >  1 file changed, 6 insertions(+), 66 deletions(-)
> > 
> > diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> > index cdce7f173d00..cfedb29260ef 100644
> > --- a/tools/perf/util/genelf.c
> > +++ b/tools/perf/util/genelf.c
> > @@ -28,24 +28,7 @@
> >  #define NT_GNU_BUILD_ID 3
> >  #endif
> >  
> > -#define BUILD_ID_URANDOM /* different uuid for each run */
> > -
> > -#ifdef HAVE_LIBCRYPTO_SUPPORT
> > -
> > -#define BUILD_ID_MD5
> > -#undef BUILD_ID_SHA	/* does not seem to work well when linked with Java */
> > -#undef BUILD_ID_URANDOM /* different uuid for each run */
> > -
> > -#ifdef BUILD_ID_SHA
> > -#include <openssl/sha.h>
> > -#endif
> > -
> > -#ifdef BUILD_ID_MD5
> > -#include <openssl/evp.h>
> > -#include <openssl/md5.h>
> > -#endif
> > -#endif
> > -
> > +#include "sha1_base.h"
> >  
> >  typedef struct {
> >    unsigned int namesz;  /* Size of entry's owner string */
> > @@ -92,64 +75,21 @@ static Elf_Sym symtab[]={
> >  	}
> >  };
> >  
> > -#ifdef BUILD_ID_URANDOM
> > -static void
> > -gen_build_id(struct buildid_note *note,
> > -	     unsigned long load_addr __maybe_unused,
> > -	     const void *code __maybe_unused,
> > -	     size_t csize __maybe_unused)
> > -{
> > -	int fd;
> > -	size_t sz = sizeof(note->build_id);
> > -	ssize_t sret;
> > -
> > -	fd = open("/dev/urandom", O_RDONLY);
> > -	if (fd == -1)
> > -		err(1, "cannot access /dev/urandom for buildid");
> > -
> > -	sret = read(fd, note->build_id, sz);
> > -
> > -	close(fd);
> > -
> > -	if (sret != (ssize_t)sz)
> > -		memset(note->build_id, 0, sz);
> > -}
> > -#endif
> > -
> > -#ifdef BUILD_ID_SHA
> >  static void
> >  gen_build_id(struct buildid_note *note,
> >  	     unsigned long load_addr __maybe_unused,
> >  	     const void *code,
> >  	     size_t csize)
> >  {
> > -	if (sizeof(note->build_id) < SHA_DIGEST_LENGTH)
> > -		errx(1, "build_id too small for SHA1");
> > -
> > -	SHA1(code, csize, (unsigned char *)note->build_id);
> > -}
> > -#endif
> > -
> > -#ifdef BUILD_ID_MD5
> > -static void
> > -gen_build_id(struct buildid_note *note, unsigned long load_addr, const void *code, size_t csize)
> > -{
> > -	EVP_MD_CTX *mdctx;
> > +	struct sha1_state sctx;
> >  
> > -	if (sizeof(note->build_id) < 16)
> > -		errx(1, "build_id too small for MD5");
> > +	if (sizeof(note->build_id) < SHA1_DIGEST_SIZE)
> > +		errx(1, "build_id too small for SHA1");
> >  
> > -	mdctx = EVP_MD_CTX_new();
> > -	if (!mdctx)
> > -		errx(2, "failed to create EVP_MD_CTX");
> > +	sha1_base_init(&sctx);
> >  
> > -	EVP_DigestInit_ex(mdctx, EVP_md5(), NULL);
> > -	EVP_DigestUpdate(mdctx, &load_addr, sizeof(load_addr));
> > -	EVP_DigestUpdate(mdctx, code, csize);
> > -	EVP_DigestFinal_ex(mdctx, (unsigned char *)note->build_id, NULL);
> > -	EVP_MD_CTX_free(mdctx);
> > +	crypto_sha1_finup(&sctx, code, csize, (unsigned char *)note->build_id);
> >  }
> > -#endif
> >  
> >  static int
> >  jit_add_eh_frame_info(Elf *e, void* unwinding, uint64_t unwinding_header_size,
> > -- 
> > 2.49.0.1164.gab81da1b16-goog