Re: [PATCH 1/2] tools/perf: Fix the file mode with copyfile while adding file to build-id cache

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

Em Mon, Jan 16, 2023 at 10:31:30AM +0530, Athira Rajeev escreveu:
> The test "build id cache operations" fails on powerpc
> As below:
> 
> 	Adding 5a0fd882b53084224ba47b624c55a469 ./tests/shell/../pe-file.exe: Ok
> 	build id: 5a0fd882b53084224ba47b624c55a469
> 	link: /tmp/perf.debug.ZTu/.build-id/5a/0fd882b53084224ba47b624c55a469
> 	file: /tmp/perf.debug.ZTu/.build-id/5a/../../root/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf
> 	failed: file /tmp/perf.debug.ZTu/.build-id/5a/../../root/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf does not exist
> 	test child finished with -1
> 	---- end ----
> 	build id cache operations: FAILED!
> 
> The failing test is when trying to add pe-file.exe to
> build id cache.
> 
> Perf buildid-cache can be used to add/remove/manage
> files from the build-id cache. "-a" option is used to
> add a file to the build-id cache.
> 
> Simple command to do so for a PE exe file:
>  # ls -ltr tests/pe-file.exe
>  -rw-r--r--. 1 root root 75595 Jan 10 23:35 tests/pe-file.exe
>  The file is in home directory.
> 
>  # mkdir  /tmp/perf.debug.TeY1
>  # perf --buildid-dir /tmp/perf.debug.TeY1 buildid-cache -v
>    -a tests/pe-file.exe
> 
> The above will create ".build-id" folder in build id
> directory, which is /tmp/perf.debug.TeY1. Also adds file
> to this folder under build id. Example:
> 
>  # ls -ltr /tmp/perf.debug.TeY1/.build-id/5a/0fd882b53084224ba47b624c55a469/
>  total 76
>  -rw-r--r--. 1 root root     0 Jan 11 00:38 probes
>  -rwxr-xr-x. 1 root root 75595 Jan 11 00:38 elf
> 
> We can see in the results that file mode for original
> file and file in build id directory is different. ie,
> build id file has executable permission whereas original
> file doesn’t have.
> 
> The code path and function ( build_id_cache__add ) to
> add file to cache is in "util/build-id.c". In
> build_id_cache__add() function, it first attempts to link
> the original file to destination cache folder. If linking
> the file fails ( which can happen if the destination and
> source is on a different mount points ), it will copy the
> file to destination. Here copyfile() routine explicitly uses
> mode as "755" and hence file in the destination will have
> executable permission.
> 
> Code snippet:
> 
>  if (link(realname, filename) && errno != EEXIST &&
>                                copyfile(name, filename))
> 
> Strace logs:
> 
> 	172285 link("/home/<user_name>/linux/tools/perf/tests/pe-file.exe", "/tmp/perf.debug.TeY1/home/<user_name>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf") = -1 EXDEV (Invalid cross-device link)
> 	172285 newfstatat(AT_FDCWD, "tests/pe-file.exe", {st_mode=S_IFREG|0644, st_size=75595, ...}, 0) = 0
> 	172285 openat(AT_FDCWD, "/tmp/perf.debug.TeY1/home/<user_name>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/.elf.KbAnsl", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
> 	172285 fchmod(3, 0755)                  = 0
> 	172285 openat(AT_FDCWD, "tests/pe-file.exe", O_RDONLY) = 4
> 	172285 mmap(NULL, 75595, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fffa5cd0000
> 	172285 pwrite64(3, "MZ\220\0\3\0\0\0\4\0\0\0\377\377\0\0\270\0\0\0\0\0\0\0@\0\0\0\0\0\0\0"..., 75595, 0) = 75595
> 
> Whereas if the link succeeds, it succeeds in the first
> attempt itself and the file in the build-id dir will
> have same permission as original file.
> 
> Example, above uses /tmp. Instead if we use
> "--buildid-dir /home/build", linking will work here
> since mount points are same. Hence the destination file
> will not have executable permission.
> 
> Since the testcase "tests/shell/buildid.sh" always looks
> for executable file, test fails in powerpc environment
> when test is run from /root.
> 
> The patch adds a change in build_id_cache__add to use
> copyfile_mode which also passes the file’s original mode as
> argument. This way the destination file mode also will
> be same as original file.
> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

Thanks, applied both patches, IIRC there were an Acked-by from Ian for
this one? Or was that reference a cut'n'paste error with that other
series for the #/bin/bash changes?

- Arnaldo

> ---
>  tools/perf/util/build-id.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index a839b30c981b..ea9c083ab1e3 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -715,9 +715,13 @@ build_id_cache__add(const char *sbuild_id, const char *name, const char *realnam
>  		} else if (nsi && nsinfo__need_setns(nsi)) {
>  			if (copyfile_ns(name, filename, nsi))
>  				goto out_free;
> -		} else if (link(realname, filename) && errno != EEXIST &&
> -				copyfile(name, filename))
> -			goto out_free;
> +		} else if (link(realname, filename) && errno != EEXIST) {
> +			struct stat f_stat;
> +
> +			if (!(stat(name, &f_stat) < 0) &&
> +					copyfile_mode(name, filename, f_stat.st_mode))
> +				goto out_free;
> +		}
>  	}
>  
>  	/* Some binaries are stripped, but have .debug files with their symbol
> -- 
> 2.31.1

-- 

- Arnaldo