[PATCH 1/4] Tweak self-armouring

[PATCH 1/4] Tweak self-armouring

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

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 <eugene.loh@oracle.com>
---
 libdtrace/dt_proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
index a052abbac..7c3eb2a24 100644
--- a/libdtrace/dt_proc.c
+++ b/libdtrace/dt_proc.c
@@ -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;
 		}
-- 
2.43.5

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

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/unittest/pid/tst.entry_off0.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/test/unittest/pid/tst.entry_off0.sh b/test/unittest/pid/tst.entry_off0.sh
index f1a75b6ae..1a4c3d207 100755
--- a/test/unittest/pid/tst.entry_off0.sh
+++ b/test/unittest/pid/tst.entry_off0.sh
@@ -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
-- 
2.43.5

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

[Kris Van Hees]

On Fri, Oct 04, 2024 at 12:43:53AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  test/unittest/pid/tst.entry_off0.sh | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/test/unittest/pid/tst.entry_off0.sh b/test/unittest/pid/tst.entry_off0.sh
> index f1a75b6ae..1a4c3d207 100755
> --- a/test/unittest/pid/tst.entry_off0.sh
> +++ b/test/unittest/pid/tst.entry_off0.sh
> @@ -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
> -- 
> 2.43.5
> 

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

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

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 <eugene.loh@oracle.com>
---
 test/unittest/types/err.unloaded_var.x | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/unittest/types/err.unloaded_var.x b/test/unittest/types/err.unloaded_var.x
index 54c61d57d..cc63265a4 100755
--- a/test/unittest/types/err.unloaded_var.x
+++ b/test/unittest/types/err.unloaded_var.x
@@ -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
-- 
2.43.5

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

[Kris Van Hees]

On Fri, Oct 04, 2024 at 12:43:54AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> 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 <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  test/unittest/types/err.unloaded_var.x | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/test/unittest/types/err.unloaded_var.x b/test/unittest/types/err.unloaded_var.x
> index 54c61d57d..cc63265a4 100755
> --- a/test/unittest/types/err.unloaded_var.x
> +++ b/test/unittest/types/err.unloaded_var.x
> @@ -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
> -- 
> 2.43.5
> 

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

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

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 <eugene.loh@oracle.com>
---
 test/unittest/scalars/tst.misc.d | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/unittest/scalars/tst.misc.d b/test/unittest/scalars/tst.misc.d
index 60edab45e..6a5f4ae2e 100644
--- a/test/unittest/scalars/tst.misc.d
+++ b/test/unittest/scalars/tst.misc.d
@@ -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

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

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

On Fri, Oct 04, 2024 at 12:43:55AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> 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 <eugene.loh@oracle.com>
> ---
>  test/unittest/scalars/tst.misc.d | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/unittest/scalars/tst.misc.d b/test/unittest/scalars/tst.misc.d
> index 60edab45e..6a5f4ae2e 100644
> --- a/test/unittest/scalars/tst.misc.d
> +++ b/test/unittest/scalars/tst.misc.d
> @@ -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
> 

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

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

On 10/25/24 17:16, Kris Van Hees wrote:
> 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?
>
> On Fri, Oct 04, 2024 at 12:43:55AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> 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 <eugene.loh@oracle.com>
>> ---
>>   test/unittest/scalars/tst.misc.d | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/test/unittest/scalars/tst.misc.d b/test/unittest/scalars/tst.misc.d
>> index 60edab45e..6a5f4ae2e 100644
>> --- a/test/unittest/scalars/tst.misc.d
>> +++ b/test/unittest/scalars/tst.misc.d
>> @@ -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
>>

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

[Kris Van Hees]

Resend to the list for Nick

On Wed, Oct 30, 2024 at 04:14:54PM -0400, Eugene Loh wrote:
> 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?
> 
> On 10/25/24 17:16, Kris Van Hees wrote:
> > 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?
> > 
> > On Fri, Oct 04, 2024 at 12:43:55AM -0400, eugene.loh@oracle.com wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > 
> > > 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 <eugene.loh@oracle.com>
> > > ---
> > >   test/unittest/scalars/tst.misc.d | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/test/unittest/scalars/tst.misc.d b/test/unittest/scalars/tst.misc.d
> > > index 60edab45e..6a5f4ae2e 100644
> > > --- a/test/unittest/scalars/tst.misc.d
> > > +++ b/test/unittest/scalars/tst.misc.d
> > > @@ -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
> > > 

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

[Nick Alcock]

On 30 Oct 2024, Eugene Loh stated:

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

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

>> On Fri, Oct 04, 2024 at 12:43:55AM -0400, eugene.loh@oracle.com wrote:
>>> From: Eugene Loh <eugene.loh@oracle.com>
>>>
>>> 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.

>>>      # 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 <eugene.loh@oracle.com>
>>> ---
>>>   test/unittest/scalars/tst.misc.d | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/test/unittest/scalars/tst.misc.d b/test/unittest/scalars/tst.misc.d
>>> index 60edab45e..6a5f4ae2e 100644
>>> --- a/test/unittest/scalars/tst.misc.d
>>> +++ b/test/unittest/scalars/tst.misc.d
>>> @@ -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
>>>

-- 
NULL && (void)

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

[Kris Van Hees]

On Sun, Sep 14, 2025 at 12:18:20PM +0100, Nick Alcock via DTrace-devel wrote:
> On 30 Oct 2024, Eugene Loh stated:
> 
> > 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.
> 
> > 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:
> >> 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.

> >> On Fri, Oct 04, 2024 at 12:43:55AM -0400, eugene.loh@oracle.com wrote:
> >>> From: Eugene Loh <eugene.loh@oracle.com>
> >>>
> >>> 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.
> 
> >>>      # 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 <eugene.loh@oracle.com>
> >>> ---
> >>>   test/unittest/scalars/tst.misc.d | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/test/unittest/scalars/tst.misc.d b/test/unittest/scalars/tst.misc.d
> >>> index 60edab45e..6a5f4ae2e 100644
> >>> --- a/test/unittest/scalars/tst.misc.d
> >>> +++ b/test/unittest/scalars/tst.misc.d
> >>> @@ -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
> >>>
> 
> -- 
> NULL && (void)
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel

Re: [PATCH 1/4] Tweak self-armouring

[Kris Van Hees]

On Fri, Oct 04, 2024 at 12:43:52AM -0400, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> 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 <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  libdtrace/dt_proc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
> index a052abbac..7c3eb2a24 100644
> --- a/libdtrace/dt_proc.c
> +++ b/libdtrace/dt_proc.c
> @@ -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;
>  		}
> -- 
> 2.43.5
> 

Re: [DTrace-devel] [PATCH 1/4] Tweak self-armouring

[Nick Alcock]

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

> On Fri, Oct 04, 2024 at 12:43:52AM -0400, eugene.loh@oracle.com wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>> 
>> 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 <eugene.loh@oracle.com>
>
> Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

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

>> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
>> index a052abbac..7c3eb2a24 100644
>> --- a/libdtrace/dt_proc.c
>> +++ b/libdtrace/dt_proc.c
>> @@ -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;

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

-- 
NULL && (void)

Re: [DTrace-devel] [PATCH 1/4] Tweak self-armouring

[Nick Alcock]

On 29 Nov 2024, Nick Alcock via DTrace-devel said:
> 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 :)

-- 
NULL && (void)