Tweak self-armouring

PATCH In review [email protected] 3 participants 14 messages Oct 4, 2024 – Sep 15, 2025

lore

Share & tools

lore

[email protected]

author

2024-10-04 04:44 UTC #0 lore raw

PATCH 1/4 Tweak self-armouring

From: Eugene Loh <[email protected]>

Commit ea592d60 ("proc: improve armouring against self-grabs") has no tests, but it breaks test/unittest/usdt/tst.multitrace.sh. The patch makes the following change in libdtrace/dt_proc.c dt_proc_control():

if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
dtp->dt_sysslice) > 0) ||
- ((tracer_pid != 0) &&
- (tracer_pid != getpid())) ||
+ (tracer_pid == getpid()) ||
- (dpr->dpr_pid == getpid()))
+ (tgid == getpid()))
noninvasive = 2;

We change a

        (tracer_pid != getpid())
into a
        (tracer_pid == getpid())

Changing that == back into a != makes tst.multitrace.sh work again.

Signed-off-by Eugene Loh <[email protected]>

---
 libdtrace/dt_proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

libdtrace/dt_proc.c +1 -1

index a052abbac..7c3eb2a24 100644
@@ -954,7 +954,7 @@ dt_proc_control(void *arg)
 
 			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
 				    dtp->dt_sysslice) > 0) ||
-			    (tracer_pid == getpid()) ||
+			    (tracer_pid != getpid()) ||
 			    (tgid == getpid()))
 				noninvasive = 2;
 		}

Signature · 1 line

2.43.5

[email protected]

author

2024-10-04 04:44 UTC #1 lore raw

PATCH 2/4 test: Increase timeout due to long shutdown

From: Eugene Loh <[email protected]>

Signed-off-by Eugene Loh <[email protected]>

---
 test/unittest/pid/tst.entry_off0.sh | 4 ++++
 1 file changed, 4 insertions(+)

test/unittest/pid/tst.entry_off0.sh +4 -0

index f1a75b6ae..1a4c3d207 100755
@@ -6,6 +6,10 @@
 # http://oss.oracle.com/licenses/upl.
 #
 
+# Although the D script takes only "one second," it takes a long time to
+# shut down.  Until that has been solved, increase the timeout for the test.
+# @@timeout: 120
+
 dtrace=$1
 
 trig=`pwd`/test/triggers/ustack-tst-basic

Signature · 1 line

2.43.5

Kris Van Hees

Reviewed-by on v1 2/4

2024-10-25 21:05 UTC #2 lore raw

Re: [PATCH 2/4] test: Increase timeout due to long shutdown

Quoted 3 lines · On Fri, Oct 04, 2024 at 12:43:53AM -0400, [email protected] wrote:

From: Eugene Loh <[email protected]>
Signed-off-by Eugene Loh <[email protected]>

Reviewed-by Kris Van Hees <[email protected]>

Quoted 22 lines

---
 test/unittest/pid/tst.entry_off0.sh | 4 ++++
 1 file changed, 4 insertions(+)

test/unittest/pid/tst.entry_off0.sh +4 -0

index f1a75b6ae..1a4c3d207 100755
@@ -6,6 +6,10 @@
 # http://oss.oracle.com/licenses/upl.
 #
 
+# Although the D script takes only "one second," it takes a long time to
+# shut down.  Until that has been solved, increase the timeout for the test.
+# @@timeout: 120
+
 dtrace=$1
 
 trig=`pwd`/test/triggers/ustack-tst-basic

Signature · 1 line

2.43.5

[email protected]

author

2024-10-04 04:44 UTC #3 lore raw

PATCH 3/4 test: Do not return 1 for err.*.x checks

In reply to [email protected]

From: Eugene Loh <[email protected]>

We can mark tests as expected to fail. In fact, with .x files, we can mark XFAIL based on a runtime check.

For err.* tests, however, we are expected to fail anyhow. So, having an err.*.x return 1 is at least confusing... certainly for the runtest.sh script.

In practice, err.*.x files usually return only 0 or 2 -- that is, we might elect to skip the test.

The sole exception is err.unloaded_var.x, which checks for /lib/modules/$(uname -r)/kernel/fs/gfs2/gfs2.ko*. If this module is missing, the test is supposed to fail, but as it is err.* it is expected to fail anyhow. So the counterintuitive result is that if the module is missing, the test produces XPASS.

Change this err.*.x file to skip the test if the module is missing.

Signed-off-by Eugene Loh <[email protected]>

---
 test/unittest/types/err.unloaded_var.x | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

test/unittest/types/err.unloaded_var.x +2 -2

index 54c61d57d..cc63265a4 100755
@@ -2,7 +2,7 @@
 # Licensed under the Universal Permissive License v 1.0 as shown at
 # http://oss.oracle.com/licenses/upl.
 #
-# XFAIL if gfs2.ko not found (exit 1)
+# SKIP if gfs2.ko not found (exit 2)
 [[ ! -e /lib/modules/$(uname -r)/kernel/fs/gfs2/gfs2.ko ]] &&
-[[ ! -e /lib/modules/$(uname -r)/kernel/fs/gfs2/gfs2.ko.xz ]] && exit 1;
+[[ ! -e /lib/modules/$(uname -r)/kernel/fs/gfs2/gfs2.ko.xz ]] && exit 2;
 exit 0

Signature · 1 line

2.43.5

Kris Van Hees

Reviewed-by on v1 3/4

2024-10-25 21:10 UTC #4 lore raw

Re: [PATCH 3/4] test: Do not return 1 for err.*.x checks

Quoted 18 lines · On Fri, Oct 04, 2024 at 12:43:54AM -0400, [email protected] wrote:

From: Eugene Loh <[email protected]>
We can mark tests as expected to fail. In fact, with .x files, we can mark XFAIL based on a runtime check.
For err.* tests, however, we are expected to fail anyhow. So, having an err.*.x return 1 is at least confusing... certainly for the runtest.sh script.
In practice, err.*.x files usually return only 0 or 2 -- that is, we might elect to skip the test.
The sole exception is err.unloaded_var.x, which checks for /lib/modules/$(uname -r)/kernel/fs/gfs2/gfs2.ko*. If this module is missing, the test is supposed to fail, but as it is err.* it is expected to fail anyhow. So the counterintuitive result is that if the module is missing, the test produces XPASS.

Quoted 3 lines

Change this err.*.x file to skip the test if the module is missing.
Signed-off-by Eugene Loh <[email protected]>

Reviewed-by Kris Van Hees <[email protected]>

Quoted 21 lines

---
 test/unittest/types/err.unloaded_var.x | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

test/unittest/types/err.unloaded_var.x +2 -2

index 54c61d57d..cc63265a4 100755
@@ -2,7 +2,7 @@
 # Licensed under the Universal Permissive License v 1.0 as shown at
 # http://oss.oracle.com/licenses/upl.
 #
-# XFAIL if gfs2.ko not found (exit 1)
+# SKIP if gfs2.ko not found (exit 2)
 [[ ! -e /lib/modules/$(uname -r)/kernel/fs/gfs2/gfs2.ko ]] &&
-[[ ! -e /lib/modules/$(uname -r)/kernel/fs/gfs2/gfs2.ko.xz ]] && exit 1;
+[[ ! -e /lib/modules/$(uname -r)/kernel/fs/gfs2/gfs2.ko.xz ]] && exit 2;
 exit 0

Signature · 1 line

2.43.5

[email protected]

author

2024-10-04 04:44 UTC #5 lore raw

PATCH 4/4 test: Do not depend on ext4

In reply to [email protected]

From: Eugene Loh <[email protected]>

It is possible that there is no ext4 module, whether built-in or otherwise, even if its symbols are present. E.g.,

    # grep ext4_dir_operations /proc/kallmodsyms
    ffffc45a59d9ee88 100 D ext4_dir_operations
    # grep -w ext4 /proc/kallmodsyms
    #

Meanwhile, in
ab883bae "tests, io, scalars: use kallsyms instead of kallmodsyms where possible"
we read:
scalars/tst.misc.x needs adjusting to check for the presence of the actual symbols we are looking up, since the modules might well be built-in, and thus not show up in /proc/kallsyms.

With that patch, in test/unittest/scalars/tst.misc.x, we check:
-if ! $(grep -qw ext4 /proc/kallmodsyms); then
+if ! grep -qw ext4_dir_operations /proc/kallsyms; then
exit 1
fi

So it is possible for us to find
`ext4_dir_operations
but not
ext4`ext4_dir_operations

Change the .d script to look simply for `ext4_dir_operations.

Signed-off-by Eugene Loh <[email protected]>

---
 test/unittest/scalars/tst.misc.d | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

test/unittest/scalars/tst.misc.d +1 -1

index 60edab45e..6a5f4ae2e 100644
@@ -20,7 +20,7 @@
 BEGIN
 {
 	printf("\nr_cpu_ids = 0x%x\n", `nr_cpu_ids);
-	printf("ext4`ext4_dir_operations = %p\n", &ext4`ext4_dir_operations);
+	printf("ext4`ext4_dir_operations = %p\n", &`ext4_dir_operations);
 	printf("isofs`isofs_dir_operations = %p\n", &isofs`isofs_dir_operations);
 	printf("vmlinux`major_names = %p\n", &vmlinux`major_names);
 	x = 123;

Signature · 1 line

2.43.5

Kris Van Hees

2024-10-25 21:16 UTC #6 lore raw

Re: [PATCH 4/4] test: Do not depend on ext4

I am not too sure about this patch... We are not using kallmodsyms anymore for newer kernels, and on older kernels that symbol certainly be listed as [ext4]. So, if ext4_dir_operations *is* in the ext4 module code (compiled in or loadable), then the proper ref to it would be ext4`ext4_dir_operations. So the test should be able to use that - if not, that seems like a bug rather than something to change in the test?

Quoted 50 lines · On Fri, Oct 04, 2024 at 12:43:55AM -0400, [email protected] wrote:

From: Eugene Loh <[email protected]>
It is possible that there is no ext4 module, whether built-in or otherwise, even if its symbols are present. E.g.,
    # grep ext4_dir_operations /proc/kallmodsyms
    ffffc45a59d9ee88 100 D ext4_dir_operations
    # grep -w ext4 /proc/kallmodsyms
    #
Meanwhile, in
ab883bae "tests, io, scalars: use kallsyms instead of kallmodsyms where possible"
we read:
scalars/tst.misc.x needs adjusting to check for the presence of the actual symbols we are looking up, since the modules might well be built-in, and thus not show up in /proc/kallsyms.
With that patch, in test/unittest/scalars/tst.misc.x, we check:
-if ! $(grep -qw ext4 /proc/kallmodsyms); then
+if ! grep -qw ext4_dir_operations /proc/kallsyms; then
exit 1
fi
So it is possible for us to find
`ext4_dir_operations
but not
ext4`ext4_dir_operations
Change the .d script to look simply for `ext4_dir_operations.
Signed-off-by Eugene Loh <[email protected]>
---
 test/unittest/scalars/tst.misc.d | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
test/unittest/scalars/tst.misc.d +1 -1
index 60edab45e..6a5f4ae2e 100644
@@ -20,7 +20,7 @@
 BEGIN
 {
 	printf("\nr_cpu_ids = 0x%x\n", `nr_cpu_ids);
-	printf("ext4`ext4_dir_operations = %p\n", &ext4`ext4_dir_operations);
+	printf("ext4`ext4_dir_operations = %p\n", &`ext4_dir_operations);
 	printf("isofs`isofs_dir_operations = %p\n", &isofs`isofs_dir_operations);
 	printf("vmlinux`major_names = %p\n", &vmlinux`major_names);
 	x = 123;
Signature · 1 line
2.43.5

Eugene Loh

author

2024-10-30 20:15 UTC #7 lore raw

Re: [PATCH 4/4] test: Do not depend on ext4

E.g.,

    $ uname -r
    5.15.0-205.149.5.1.el9uek.aarch64
    $ sudo grep ext4_dir_operations /proc/kallmodsyms
     ffffc45a59d9ee88 100 D ext4_dir_operations
     $ sudo awk '/ext4/ { print NF }' /proc/kallmodsyms | uniq -c
       2342 4
    $ sudo grep -w ext4 /proc/kallmodsyms

So, lots of ext4 symbols (including ext4_dir_operations) but none of them are [ext4]. Bug in... kallmodsyms?

Quoted 58 lines · Kris Van Hees

I am not too sure about this patch... We are not using kallmodsyms anymore for newer kernels, and on older kernels that symbol certainly be listed as [ext4]. So, if ext4_dir_operations *is* in the ext4 module code (compiled in or loadable), then the proper ref to it would be ext4`ext4_dir_operations. So the test should be able to use that - if not, that seems like a bug rather than something to change in the test?
Quoted 50 lines · On Fri, Oct 04, 2024 at 12:43:55AM -0400, [email protected] wrote:
From: Eugene Loh <[email protected]>
It is possible that there is no ext4 module, whether built-in or otherwise, even if its symbols are present. E.g.,
     # grep ext4_dir_operations /proc/kallmodsyms
     ffffc45a59d9ee88 100 D ext4_dir_operations
     # grep -w ext4 /proc/kallmodsyms
     #
Meanwhile, in
ab883bae "tests, io, scalars: use kallsyms instead of kallmodsyms where possible"
we read:
scalars/tst.misc.x needs adjusting to check for the presence of the actual symbols we are looking up, since the modules might well be built-in, and thus not show up in /proc/kallsyms.
With that patch, in test/unittest/scalars/tst.misc.x, we check:
-if ! $(grep -qw ext4 /proc/kallmodsyms); then +if ! grep -qw ext4_dir_operations /proc/kallsyms; then
exit 1
fi
So it is possible for us to find
`ext4_dir_operations
but not
ext4`ext4_dir_operations
Change the .d script to look simply for `ext4_dir_operations.
Signed-off-by Eugene Loh <[email protected]>
---
  test/unittest/scalars/tst.misc.d | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
test/unittest/scalars/tst.misc.d +1 -1
index 60edab45e..6a5f4ae2e 100644
@@ -20,7 +20,7 @@
  BEGIN
  {
  	printf("\nr_cpu_ids = 0x%x\n", `nr_cpu_ids);
-	printf("ext4`ext4_dir_operations = %p\n", &ext4`ext4_dir_operations);
+	printf("ext4`ext4_dir_operations = %p\n", &`ext4_dir_operations);
  	printf("isofs`isofs_dir_operations = %p\n", &isofs`isofs_dir_operations);
  	printf("vmlinux`major_names = %p\n", &vmlinux`major_names);
  	x = 123;
Signature · 1 line
2.43.5

Kris Van Hees

2024-11-27 16:51 UTC #8 lore raw

Re: [PATCH 4/4] test: Do not depend on ext4

Resend to the list for Nick

Quoted 71 lines · Eugene Loh

E.g.,
    $ uname -r
    5.15.0-205.149.5.1.el9uek.aarch64
    $ sudo grep ext4_dir_operations /proc/kallmodsyms
     ffffc45a59d9ee88 100 D ext4_dir_operations
     $ sudo awk '/ext4/ { print NF }' /proc/kallmodsyms | uniq -c
       2342 4
    $ sudo grep -w ext4 /proc/kallmodsyms
So, lots of ext4 symbols (including ext4_dir_operations) but none of them are [ext4]. Bug in... kallmodsyms?
Quoted 58 lines · Kris Van Hees
I am not too sure about this patch... We are not using kallmodsyms anymore for newer kernels, and on older kernels that symbol certainly be listed as [ext4]. So, if ext4_dir_operations *is* in the ext4 module code (compiled in or loadable), then the proper ref to it would be ext4`ext4_dir_operations. So the test should be able to use that - if not, that seems like a bug rather than something to change in the test?
Quoted 50 lines · On Fri, Oct 04, 2024 at 12:43:55AM -0400, [email protected] wrote:
From: Eugene Loh <[email protected]>
It is possible that there is no ext4 module, whether built-in or otherwise, even if its symbols are present. E.g.,
     # grep ext4_dir_operations /proc/kallmodsyms
     ffffc45a59d9ee88 100 D ext4_dir_operations
     # grep -w ext4 /proc/kallmodsyms
     #
Meanwhile, in
ab883bae "tests, io, scalars: use kallsyms instead of kallmodsyms where possible"
we read:
scalars/tst.misc.x needs adjusting to check for the presence of the actual symbols we are looking up, since the modules might well be built-in, and thus not show up in /proc/kallsyms.
With that patch, in test/unittest/scalars/tst.misc.x, we check:
-if ! $(grep -qw ext4 /proc/kallmodsyms); then +if ! grep -qw ext4_dir_operations /proc/kallsyms; then
exit 1
fi
So it is possible for us to find
`ext4_dir_operations
but not
ext4`ext4_dir_operations
Change the .d script to look simply for `ext4_dir_operations.
Signed-off-by Eugene Loh <[email protected]>
---
  test/unittest/scalars/tst.misc.d | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
test/unittest/scalars/tst.misc.d +1 -1
index 60edab45e..6a5f4ae2e 100644
@@ -20,7 +20,7 @@
  BEGIN
  {
  	printf("\nr_cpu_ids = 0x%x\n", `nr_cpu_ids);
-	printf("ext4`ext4_dir_operations = %p\n", &ext4`ext4_dir_operations);
+	printf("ext4`ext4_dir_operations = %p\n", &`ext4_dir_operations);
  	printf("isofs`isofs_dir_operations = %p\n", &isofs`isofs_dir_operations);
  	printf("vmlinux`major_names = %p\n", &vmlinux`major_names);
  	x = 123;
Signature · 1 line
2.43.5

Nick Alcock

2025-09-14 11:18 UTC #9 lore raw

Re: [PATCH 4/4] test: Do not depend on ext4

In reply to Eugene Loh

On 30 Oct 2024, Eugene Loh stated:

Quoted 8 lines

E.g.,

    $ uname -r
    5.15.0-205.149.5.1.el9uek.aarch64
    $ sudo grep ext4_dir_operations /proc/kallmodsyms
     ffffc45a59d9ee88 100 D ext4_dir_operations
     $ sudo awk '/ext4/ { print NF }' /proc/kallmodsyms | uniq -c
       2342 4
    $ sudo grep -w ext4 /proc/kallmodsyms

Something is presumably wrong there -- ext4 should always be considered a built-in module if it's built in, that's what built-in modules *are*, things that might be being built as modules but aren't.

i.e. this test is working as intended and pointing out a real bug :) whether it's one we care about is another matter.

Quoted 9 lines

So, lots of ext4 symbols (including ext4_dir_operations) but none of them are [ext4]. Bug in... kallmodsyms?
Quoted 6 lines · Kris Van Hees
I am not too sure about this patch... We are not using kallmodsyms anymore for newer kernels, and on older kernels that symbol certainly be listed as [ext4]. So, if ext4_dir_operations *is* in the ext4 module code (compiled in or loadable), then the proper ref to it would be ext4`ext4_dir_operations. So the test should be able to use that - if not, that seems like a bug rather than something to change in the test?

Yes.

Quoted 5 lines

Quoted 5 lines · On Fri, Oct 04, 2024 at 12:43:55AM -0400, [email protected] wrote:
Quoted 4 lines
From: Eugene Loh <[email protected]>
It is possible that there is no ext4 module, whether built-in or otherwise, even if its symbols are present. E.g.,

That wasn't true in the kallmodsyms world I wrote this for.

Quoted 45 lines

Quoted 45 lines
Quoted 45 lines
     # grep ext4_dir_operations /proc/kallmodsyms
     ffffc45a59d9ee88 100 D ext4_dir_operations
     # grep -w ext4 /proc/kallmodsyms
     #
Meanwhile, in
ab883bae "tests, io, scalars: use kallsyms instead of kallmodsyms where possible"
we read:
scalars/tst.misc.x needs adjusting to check for the presence of the actual symbols we are looking up, since the modules might well be built-in, and thus not show up in /proc/kallsyms.
With that patch, in test/unittest/scalars/tst.misc.x, we check:
-if ! $(grep -qw ext4 /proc/kallmodsyms); then +if ! grep -qw ext4_dir_operations /proc/kallsyms; then
exit 1
fi
So it is possible for us to find
`ext4_dir_operations
but not
ext4`ext4_dir_operations
Change the .d script to look simply for `ext4_dir_operations.
Signed-off-by Eugene Loh <[email protected]>
---
  test/unittest/scalars/tst.misc.d | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
test/unittest/scalars/tst.misc.d +1 -2
index 60edab45e..6a5f4ae2e 100644
@@ -20,7 +20,7 @@
  BEGIN
  {
  	printf("\nr_cpu_ids = 0x%x\n", `nr_cpu_ids);
-	printf("ext4`ext4_dir_operations = %p\n", &ext4`ext4_dir_operations);
+	printf("ext4`ext4_dir_operations = %p\n", &`ext4_dir_operations);
  	printf("isofs`isofs_dir_operations = %p\n", &isofs`isofs_dir_operations);
  	printf("vmlinux`major_names = %p\n", &vmlinux`major_names);
  	x = 123;
-- 2.43.5

Signature · 1 line

NULL && (void)

Kris Van Hees

2025-09-15 17:39 UTC #10 lore raw

Re: [DTrace-devel] [PATCH 4/4] test: Do not depend on ext4

Quoted 21 lines · Nick Alcock via DTrace-devel

On 30 Oct 2024, Eugene Loh stated:
Quoted 8 lines
E.g.,
    $ uname -r
    5.15.0-205.149.5.1.el9uek.aarch64
    $ sudo grep ext4_dir_operations /proc/kallmodsyms
     ffffc45a59d9ee88 100 D ext4_dir_operations
     $ sudo awk '/ext4/ { print NF }' /proc/kallmodsyms | uniq -c
       2342 4
    $ sudo grep -w ext4 /proc/kallmodsyms
Something is presumably wrong there -- ext4 should always be considered a built-in module if it's built in, that's what built-in modules *are*, things that might be being built as modules but aren't.
i.e. this test is working as intended and pointing out a real bug :) whether it's one we care about is another matter.
Quoted 3 lines
So, lots of ext4 symbols (including ext4_dir_operations) but none of them are [ext4]. Bug in... kallmodsyms?
On 10/25/24 17:16, Kris Van Hees wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
I am not too sure about this patch... We are not using kallmodsyms anymore for newer kernels, and on older kernels that symbol certainly be listed as [ext4]. So, if ext4_dir_operations *is* in the ext4 module code (compiled in or loadable), then the proper ref to it would be ext4`ext4_dir_operations. So the test should be able to use that - if not, that seems like a bug rather than something to change in the test?
Yes.

OK, I will mark it as NAK so it gets removed from my tracking.

Quoted 61 lines

Quoted 5 lines
Quoted 5 lines · On Fri, Oct 04, 2024 at 12:43:55AM -0400, [email protected] wrote:
Quoted 4 lines
From: Eugene Loh <[email protected]>
It is possible that there is no ext4 module, whether built-in or otherwise, even if its symbols are present. E.g.,
That wasn't true in the kallmodsyms world I wrote this for.
Quoted 45 lines
Quoted 45 lines
Quoted 45 lines
     # grep ext4_dir_operations /proc/kallmodsyms
     ffffc45a59d9ee88 100 D ext4_dir_operations
     # grep -w ext4 /proc/kallmodsyms
     #
Meanwhile, in
ab883bae "tests, io, scalars: use kallsyms instead of kallmodsyms where possible"
we read:
scalars/tst.misc.x needs adjusting to check for the presence of the actual symbols we are looking up, since the modules might well be built-in, and thus not show up in /proc/kallsyms.
With that patch, in test/unittest/scalars/tst.misc.x, we check:
-if ! $(grep -qw ext4 /proc/kallmodsyms); then +if ! grep -qw ext4_dir_operations /proc/kallsyms; then
exit 1
fi
So it is possible for us to find
`ext4_dir_operations
but not
ext4`ext4_dir_operations
Change the .d script to look simply for `ext4_dir_operations.
Signed-off-by Eugene Loh <[email protected]>
---
  test/unittest/scalars/tst.misc.d | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
test/unittest/scalars/tst.misc.d +1 -2
index 60edab45e..6a5f4ae2e 100644
@@ -20,7 +20,7 @@
  BEGIN
  {
  	printf("\nr_cpu_ids = 0x%x\n", `nr_cpu_ids);
-	printf("ext4`ext4_dir_operations = %p\n", &ext4`ext4_dir_operations);
+	printf("ext4`ext4_dir_operations = %p\n", &`ext4_dir_operations);
  	printf("isofs`isofs_dir_operations = %p\n", &isofs`isofs_dir_operations);
  	printf("vmlinux`major_names = %p\n", &vmlinux`major_names);
  	x = 123;
-- 2.43.5
Signature · 6 lines
NULL && (void)

_______________________________________________
DTrace-devel mailing list
[email protected]
https://oss.oracle.com/mailman/listinfo/dtrace-devel

Kris Van Hees

Reviewed-by on v1 1/4

2024-10-25 21:02 UTC #11 lore raw

In reply to [email protected]

Quoted 15 lines · On Fri, Oct 04, 2024 at 12:43:52AM -0400, [email protected] wrote:

From: Eugene Loh <[email protected]>
Commit ea592d60 ("proc: improve armouring against self-grabs") has no tests, but it breaks test/unittest/usdt/tst.multitrace.sh. The patch makes the following change in libdtrace/dt_proc.c dt_proc_control():
if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
dtp->dt_sysslice) > 0) ||
- ((tracer_pid != 0) &&
- (tracer_pid != getpid())) ||
+ (tracer_pid == getpid()) ||
- (dpr->dpr_pid == getpid()))
+ (tgid == getpid()))
noninvasive = 2;

Quoted 8 lines

We change a
        (tracer_pid != getpid())
into a
        (tracer_pid == getpid())
Changing that == back into a != makes tst.multitrace.sh work again.
Signed-off-by Eugene Loh <[email protected]>

Reviewed-by Kris Van Hees <[email protected]>

Quoted 20 lines

---
 libdtrace/dt_proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

libdtrace/dt_proc.c +1 -1

index a052abbac..7c3eb2a24 100644
@@ -954,7 +954,7 @@ dt_proc_control(void *arg)
 
 			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
 				    dtp->dt_sysslice) > 0) ||
-			    (tracer_pid == getpid()) ||
+			    (tracer_pid != getpid()) ||
 			    (tgid == getpid()))
 				noninvasive = 2;
 		}

Signature · 1 line

2.43.5

Nick Alcock

2024-11-29 13:57 UTC #12 lore raw

On 25 Oct 2024, Kris Van Hees via DTrace-devel spake thusly:

Quoted 21 lines · On Fri, Oct 04, 2024 at 12:43:52AM -0400, [email protected] wrote:

Quoted 20 lines
From: Eugene Loh <[email protected]>
Commit ea592d60 ("proc: improve armouring against self-grabs") has no tests, but it breaks test/unittest/usdt/tst.multitrace.sh. The patch makes the following change in libdtrace/dt_proc.c dt_proc_control():
if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
dtp->dt_sysslice) > 0) ||
- ((tracer_pid != 0) &&
- (tracer_pid != getpid())) ||
+ (tracer_pid == getpid()) ||
- (dpr->dpr_pid == getpid()))
+ (tgid == getpid()))
noninvasive = 2;
We change a
        (tracer_pid != getpid())
into a
        (tracer_pid == getpid())

Quoted 5 lines

Quoted 3 lines
Changing that == back into a != makes tst.multitrace.sh work again.
Signed-off-by Eugene Loh <[email protected]>
Reviewed-by Kris Van Hees <[email protected]>

It's better than what I was doing (which was definitely wrong), but it could still be improved.

Quoted 6 lines

libdtrace/dt_proc.c +0 -0

index a052abbac..7c3eb2a24 100644
@@ -954,7 +954,7 @@ dt_proc_control(void *arg)

Quoted 6 lines

libdtrace/dt_proc.c dt_proc_control(void *arg) · line 959

Quoted 6 lines

libdtrace/dt_proc.c +1 -1

 			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
 				    dtp->dt_sysslice) > 0) ||
-			    (tracer_pid == getpid()) ||
+			    (tracer_pid != getpid()) ||
 			    (tgid == getpid()))
 				noninvasive = 2;

What the previous code was saying was "if the traced process is a system daemon, or is being debugged by someone else, or is ourself, do not do an invasive grab"; in the one case because stopping systemd or the system logger, even briefly, seems a like a bad idea, and in the other two cases because you can't be debugged by two people at once and because stopping yourself is an obvious deadlock.

We want to check if it's ourself in two ways: firstly, if the tracer PID is our own PID, we are obviously ourself (this is the process-monitoring thread itself); secondly, if the tracer PID's task group leader is our own thread's task group leader, this is the main thread asking.

Unfortunately that's not what the original code was doing (the tracer_pid thing was indeed wrong, we were saying "if we are debugged by ourself, go noninvasive, but not if we're debugged by someone else"), so your change was good as far as it goes... but if the process-monitoring thread ever decides to monitor itself (perfectly possible with suffiently-wildcardy probes and a ustack() or something), we'll deadlock -- and unfortunately the tracer_pid is 0 when the process is *not* being traced, so what that change is doing is going noninvasive for *everyone, always*. That's what the original's

       ((tracer_pid != 0) &&
        (tracer_pid != getpid())) ||

was all about: if you flip the latter conditional away from ==, you *must* put the != 0 back in.

Something we're also not covering but should is that we want to fail when asked to do long-lived grabs of ourself or a process being debugged by someone else, or for that matter PID 1, *always*: long-lived grabs mean we rely on getting process death notifications, etc, and noninvasive grabs can't do that, so we *should* fail.

Also, we should define "ourself" more widely: we should really refuse ptrace grabs on *any* DTrace thread, because the amount of proxying and callbacks back and forth makes deadlocks horribly likely if, say, the main thread gets invasively grabbed.

Fix coming shortly (before you get back and read this, assuming you're guzzling turkey now like you should be).

Signature · 1 line

NULL && (void)

Nick Alcock

2024-12-03 11:46 UTC #13 lore raw

Quoted 2 lines · Nick Alcock via DTrace-devel

Fix coming shortly (before you get back and read this, assuming you're guzzling turkey now like you should be).

This is now posted. It caused tst.multitrace.sh to break again, so I found and fixed the bugs that were breaking that too :)

Signature · 1 line

NULL && (void)