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

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

[Athira Rajeev]

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>
---
 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

[PATCH 2/2] tools/perf/tests: Fix build id test check for PE file

[Athira Rajeev]

Perf test "build id cache operations" fails for PE
executable.  Logs below from powerpc system.
Same is observed on x86 as well.

<<>>
Adding 5a0fd882b53084224ba47b624c55a469 ./tests/shell/../pe-file.exe: Ok
build id: 5a0fd882b53084224ba47b624c55a469
link: /tmp/perf.debug.w0V/.build-id/5a/0fd882b53084224ba47b624c55a469
file: /tmp/perf.debug.w0V/.build-id/5a/../../root/<user>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf
failed: file /tmp/perf.debug.w0V/.build-id/5a/../../root/<user>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf does not exist
test child finished with -1
---- end ----
build id cache operations: FAILED!
<<>>

The test tries to do:

<<>>
mkdir /tmp/perf.debug.TeY1
perf --buildid-dir /tmp/perf.debug.TeY1 buildid-cache -v -a ./tests/shell/../pe-file.exe
<<>>

The option "--buildid-dir" sets the build id cache
directory as /tmp/perf.debug.TeY1. The option given
to buildid-cahe, ie "-a ./tests/shell/../pe-file.exe",
is to add the pe-file.exe to the cache. The testcase,
sets buildid-dir and adds the file: pe-file.exe to build
id cache. To check if the command is run successfully,
"check" function looks for presence of the file in buildid
cache directory. But the check here expects the added
file to be executable. Snippet below:

<<>>
if [ ! -x $file ]; then
	echo "failed: file ${file} does not exist"
	exit 1
fi
<<>>

Buildid test is done for sha1 binary, md5 binary and also
for PE file. The firt two binaries are created at runtime
by compiling with "--build-id" option and hence the check
for sha1/md5 test should use [ ! -x ]. But in case of PE
file, the permission for this input file is rw-r--r--
Hence the file added to build id cache has same permissoin

Original file:
ls tests/pe-file.exe | xargs stat --printf "%n %A \n"
tests/pe-file.exe -rw-r--r--

buildid cache file:

ls /tmp/perf.debug.w0V/.build-id/5a/../../root/<user>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf | xargs stat --printf "%n %A \n"
/tmp/perf.debug.w0V/.build-id/5a/../../root/<user>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf -rw-r--r--

Fix the test to match with the permission of original file in
case of FE file. ie if the "tests/pe-file.exe" file is not
having exec permission, just check for existence of the buildid
file using [ ! -e <file> ]

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/buildid.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh
index f05670d1e39e..22e5b2225abe 100755
--- a/tools/perf/tests/shell/buildid.sh
+++ b/tools/perf/tests/shell/buildid.sh
@@ -77,7 +77,20 @@ check()
 	file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
 	echo "file: ${file}"
 
-	if [ ! -x $file ]; then
+	# Check for file permission of orginal file
+	# in case of pe-file.exe file
+	echo $1 | grep ".exe"
+	if [ $? -eq 0 ]; then
+		if [ -x $1  -a ! -x $file ]; then
+			echo "failed: file ${file} executable does not exist"
+			exit 1
+		fi
+
+		if [ ! -x $file -a ! -e $file ]; then
+			echo "failed: file ${file} does not exist"
+			exit 1
+		fi
+	elif [ ! -x $file ]; then
 		echo "failed: file ${file} does not exist"
 		exit 1
 	fi
-- 
2.31.1

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

[Arnaldo Carvalho de Melo]

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

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

[Athira Rajeev]

> On 18-Jan-2023, at 7:20 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> 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
> 

Hi Arnaldo,

Thanks for picking up the changes in this patch set.

I re-based the patch from the other series ( for which I had got Acked-by from Ian ) on top of tmp.perf/urgent
and posted it as single V2 here:

https://lore.kernel.org/linux-perf-users/20230119113054.31742-1-atrajeev@linux.vnet.ibm.com/T/#u

Thanks
Athira

>> ---
>> 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