* [LTP] [PATCH 0/2] Optimization reference to needs_root
@ 2022-10-12 9:15 Zhao Gongyi via ltp
2022-10-12 9:15 ` [LTP] [PATCH 1/2] lib: Add checking of needs_root Zhao Gongyi via ltp
2022-10-12 9:15 ` [LTP] [PATCH 2/2] needs_root: Add setting " Zhao Gongyi via ltp
0 siblings, 2 replies; 11+ messages in thread
From: Zhao Gongyi via ltp @ 2022-10-12 9:15 UTC (permalink / raw)
To: ltp
1. Add checking of needs_root on the phase of do_setup.
2. Add setting of needs_root for tests that reference to /dev
or /proc/.
Zhao Gongyi (2):
lib: Add checking of needs_root
needs_root: Add setting of needs_root
lib/tst_test.c | 5 +++++
testcases/kernel/syscalls/bind/bind06.c | 1 +
testcases/kernel/syscalls/cma/process_vm_readv02.c | 1 +
testcases/kernel/syscalls/cma/process_vm_readv03.c | 1 +
testcases/kernel/syscalls/cma/process_vm_writev02.c | 1 +
.../kernel/syscalls/copy_file_range/copy_file_range01.c | 1 +
testcases/kernel/syscalls/ipc/msgget/msgget03.c | 1 +
testcases/kernel/syscalls/preadv/preadv03.c | 1 +
testcases/kernel/syscalls/pwritev/pwritev03.c | 1 +
testcases/kernel/syscalls/sendto/sendto03.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt05.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt06.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt07.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt08.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt09.c | 1 +
testcases/kernel/syscalls/swapon/swapon01.c | 1 +
16 files changed, 20 insertions(+)
--
2.17.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
* [LTP] [PATCH 1/2] lib: Add checking of needs_root
2022-10-12 9:15 [LTP] [PATCH 0/2] Optimization reference to needs_root Zhao Gongyi via ltp
@ 2022-10-12 9:15 ` Zhao Gongyi via ltp
2022-10-12 11:33 ` Petr Vorel
2022-10-12 9:15 ` [LTP] [PATCH 2/2] needs_root: Add setting " Zhao Gongyi via ltp
1 sibling, 1 reply; 11+ messages in thread
From: Zhao Gongyi via ltp @ 2022-10-12 9:15 UTC (permalink / raw)
To: ltp
We need to check needs_root is set when tst_test->needs_device or
tst_test->mount_device is set since access the /dev/* need a
privilege.
Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
lib/tst_test.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 8ccde1629..728a1d921 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -1196,6 +1196,11 @@ static void do_setup(int argc, char *argv[])
tst_brk(TBROK, "tst_test->mntpoint must be set!");
}
+ if ((tst_test->needs_device || tst_test->mount_device) &&
+ !tst_test->needs_root) {
+ tst_brk(TBROK, "tst_test->needs_root must be set!");
+ }
+
if (!!tst_test->needs_rofs + !!tst_test->needs_devfs +
!!tst_test->needs_device > 1) {
tst_brk(TBROK,
--
2.17.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [LTP] [PATCH 2/2] needs_root: Add setting of needs_root
2022-10-12 9:15 [LTP] [PATCH 0/2] Optimization reference to needs_root Zhao Gongyi via ltp
2022-10-12 9:15 ` [LTP] [PATCH 1/2] lib: Add checking of needs_root Zhao Gongyi via ltp
@ 2022-10-12 9:15 ` Zhao Gongyi via ltp
2022-10-13 7:41 ` Petr Vorel
1 sibling, 1 reply; 11+ messages in thread
From: Zhao Gongyi via ltp @ 2022-10-12 9:15 UTC (permalink / raw)
To: ltp
Add setting of needs_root, otherwise run the test without root
will fail and report EACCESS or EPERM.
Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
testcases/kernel/syscalls/bind/bind06.c | 1 +
testcases/kernel/syscalls/cma/process_vm_readv02.c | 1 +
testcases/kernel/syscalls/cma/process_vm_readv03.c | 1 +
testcases/kernel/syscalls/cma/process_vm_writev02.c | 1 +
testcases/kernel/syscalls/copy_file_range/copy_file_range01.c | 1 +
testcases/kernel/syscalls/ipc/msgget/msgget03.c | 1 +
testcases/kernel/syscalls/preadv/preadv03.c | 1 +
testcases/kernel/syscalls/pwritev/pwritev03.c | 1 +
testcases/kernel/syscalls/sendto/sendto03.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt05.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt06.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt07.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt08.c | 1 +
testcases/kernel/syscalls/setsockopt/setsockopt09.c | 1 +
testcases/kernel/syscalls/swapon/swapon01.c | 1 +
15 files changed, 15 insertions(+)
diff --git a/testcases/kernel/syscalls/bind/bind06.c b/testcases/kernel/syscalls/bind/bind06.c
index 618cfce46..3b6e1c99e 100644
--- a/testcases/kernel/syscalls/bind/bind06.c
+++ b/testcases/kernel/syscalls/bind/bind06.c
@@ -100,6 +100,7 @@ static void run(void)
static struct tst_test test = {
.test_all = run,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.max_runtime = 300,
diff --git a/testcases/kernel/syscalls/cma/process_vm_readv02.c b/testcases/kernel/syscalls/cma/process_vm_readv02.c
index 2bd66a49f..97a1595e3 100644
--- a/testcases/kernel/syscalls/cma/process_vm_readv02.c
+++ b/testcases/kernel/syscalls/cma/process_vm_readv02.c
@@ -116,6 +116,7 @@ static void run(void)
static struct tst_test test = {
.test_all = run,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.forks_child = 1,
diff --git a/testcases/kernel/syscalls/cma/process_vm_readv03.c b/testcases/kernel/syscalls/cma/process_vm_readv03.c
index 4caafe867..e2fc84e06 100644
--- a/testcases/kernel/syscalls/cma/process_vm_readv03.c
+++ b/testcases/kernel/syscalls/cma/process_vm_readv03.c
@@ -189,6 +189,7 @@ static void run(unsigned int i)
static struct tst_test test = {
.test = run,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.forks_child = 1,
diff --git a/testcases/kernel/syscalls/cma/process_vm_writev02.c b/testcases/kernel/syscalls/cma/process_vm_writev02.c
index 991110d24..794b9d04f 100644
--- a/testcases/kernel/syscalls/cma/process_vm_writev02.c
+++ b/testcases/kernel/syscalls/cma/process_vm_writev02.c
@@ -116,6 +116,7 @@ static void run(void)
static struct tst_test test = {
.test_all = run,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.forks_child = 1,
diff --git a/testcases/kernel/syscalls/copy_file_range/copy_file_range01.c b/testcases/kernel/syscalls/copy_file_range/copy_file_range01.c
index bbcb0ca3b..edcd5c368 100644
--- a/testcases/kernel/syscalls/copy_file_range/copy_file_range01.c
+++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range01.c
@@ -225,6 +225,7 @@ static void cleanup(void)
static struct tst_test test = {
.setup = setup,
+ .needs_root = 1,
.cleanup = cleanup,
.tcnt = ARRAY_SIZE(tcases),
.mount_device = 1,
diff --git a/testcases/kernel/syscalls/ipc/msgget/msgget03.c b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
index 2257ae0f9..d3e4bbc59 100644
--- a/testcases/kernel/syscalls/ipc/msgget/msgget03.c
+++ b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
@@ -72,6 +72,7 @@ static void cleanup(void)
static struct tst_test test = {
.needs_tmpdir = 1,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.test_all = verify_msgget,
diff --git a/testcases/kernel/syscalls/preadv/preadv03.c b/testcases/kernel/syscalls/preadv/preadv03.c
index 00b25c549..7b69c77f9 100644
--- a/testcases/kernel/syscalls/preadv/preadv03.c
+++ b/testcases/kernel/syscalls/preadv/preadv03.c
@@ -128,6 +128,7 @@ static void cleanup(void)
static struct tst_test test = {
.tcnt = ARRAY_SIZE(tcases),
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.test = verify_direct_preadv,
diff --git a/testcases/kernel/syscalls/pwritev/pwritev03.c b/testcases/kernel/syscalls/pwritev/pwritev03.c
index 91a5e3c54..ded3c9b7a 100644
--- a/testcases/kernel/syscalls/pwritev/pwritev03.c
+++ b/testcases/kernel/syscalls/pwritev/pwritev03.c
@@ -127,6 +127,7 @@ static void cleanup(void)
static struct tst_test test = {
.tcnt = ARRAY_SIZE(tcases),
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.test = verify_direct_pwritev,
diff --git a/testcases/kernel/syscalls/sendto/sendto03.c b/testcases/kernel/syscalls/sendto/sendto03.c
index 5d2c1e112..49dded5bb 100644
--- a/testcases/kernel/syscalls/sendto/sendto03.c
+++ b/testcases/kernel/syscalls/sendto/sendto03.c
@@ -208,6 +208,7 @@ static void cleanup(void)
static struct tst_test test = {
.test = run,
+ .needs_root = 1,
.tcnt = ARRAY_SIZE(testcase_list),
.setup = setup,
.cleanup = cleanup,
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt05.c b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
index 651583fb6..cfff4b970 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt05.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt05.c
@@ -93,6 +93,7 @@ static void run(void)
static struct tst_test test = {
.test_all = run,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.taint_check = TST_TAINT_W | TST_TAINT_D,
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt06.c b/testcases/kernel/syscalls/setsockopt/setsockopt06.c
index 9c818646b..7daf293b1 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt06.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt06.c
@@ -120,6 +120,7 @@ static void cleanup(void)
static struct tst_test test = {
.test_all = run,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.max_runtime = 270,
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt07.c b/testcases/kernel/syscalls/setsockopt/setsockopt07.c
index 616159a90..417fcd077 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt07.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt07.c
@@ -134,6 +134,7 @@ static void cleanup(void)
static struct tst_test test = {
.test_all = run,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.max_runtime = 150,
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
index 563444635..ce9409521 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt08.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
@@ -144,6 +144,7 @@ void run(void)
static struct tst_test test = {
.setup = setup,
+ .needs_root = 1,
.test_all = run,
.taint_check = TST_TAINT_W | TST_TAINT_D,
.forks_child = 1,
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt09.c b/testcases/kernel/syscalls/setsockopt/setsockopt09.c
index 98f7fd00e..08d07dd44 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt09.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt09.c
@@ -115,6 +115,7 @@ static void cleanup(void)
static struct tst_test test = {
.test_all = run,
+ .needs_root = 1,
.setup = setup,
.cleanup = cleanup,
.taint_check = TST_TAINT_W | TST_TAINT_D,
diff --git a/testcases/kernel/syscalls/swapon/swapon01.c b/testcases/kernel/syscalls/swapon/swapon01.c
index c334ae246..98dc92079 100644
--- a/testcases/kernel/syscalls/swapon/swapon01.c
+++ b/testcases/kernel/syscalls/swapon/swapon01.c
@@ -41,6 +41,7 @@ static void setup(void)
static struct tst_test test = {
.needs_tmpdir = 1,
+ .needs_root = 1,
.test_all = verify_swapon,
.setup = setup
};
--
2.17.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root
2022-10-12 9:15 ` [LTP] [PATCH 1/2] lib: Add checking of needs_root Zhao Gongyi via ltp
@ 2022-10-12 11:33 ` Petr Vorel
2022-10-12 18:47 ` Bird, Tim
0 siblings, 1 reply; 11+ messages in thread
From: Petr Vorel @ 2022-10-12 11:33 UTC (permalink / raw)
To: Zhao Gongyi; +Cc: ltp
Hi all,
The subject "lib: Add checking of needs_root" is a bit misleading
as it does not mention at all that it's for the loop device.
> We need to check needs_root is set when tst_test->needs_device or
> tst_test->mount_device is set since access the /dev/* need a
> privilege.
FYI we had some discussion about it, quoting Cyril [1]:
Well technically you can be added into whatever group is set to
/dev/loop-control e.g. disk group and then you can create devices
without a need to be a root.
So the most correct solution would be checking if we can access
/dev/loop-control if tst_test.needs_device is set and if not we would
imply needs_root. However this would need to be rethinked properly so
that we do not end up creating something complex and not really
required.
There is also possibility to add custom device via $LTP_DEV. That might allow to
add permissions which allow to test without root.
I'll write to automated-testing ML (and maybe to LKML ML) to see if people
prefers to test without non-root.
Also, if changed, we'd need the same change for C API.
Kind regards,
Petr
[1] https://lore.kernel.org/ltp/Yx8I0ponDUIFC8le@yuki/
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
> lib/tst_test.c | 5 +++++
> 1 file changed, 5 insertions(+)
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 8ccde1629..728a1d921 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -1196,6 +1196,11 @@ static void do_setup(int argc, char *argv[])
> tst_brk(TBROK, "tst_test->mntpoint must be set!");
> }
> + if ((tst_test->needs_device || tst_test->mount_device) &&
> + !tst_test->needs_root) {
> + tst_brk(TBROK, "tst_test->needs_root must be set!");
> + }
> +
> if (!!tst_test->needs_rofs + !!tst_test->needs_devfs +
> !!tst_test->needs_device > 1) {
> tst_brk(TBROK,
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root
2022-10-12 11:33 ` Petr Vorel
@ 2022-10-12 18:47 ` Bird, Tim
2022-10-12 19:13 ` Petr Vorel
0 siblings, 1 reply; 11+ messages in thread
From: Bird, Tim @ 2022-10-12 18:47 UTC (permalink / raw)
To: Petr Vorel, Zhao Gongyi; +Cc: ltp@lists.linux.it
> -----Original Message-----
> From: ltp <ltp-bounces+tim.bird=sony.com@lists.linux.it> On Behalf Of Petr Vorel
>
> Hi all,
>
> The subject "lib: Add checking of needs_root" is a bit misleading
> as it does not mention at all that it's for the loop device.
>
> > We need to check needs_root is set when tst_test->needs_device or
> > tst_test->mount_device is set since access the /dev/* need a
> > privilege.
>
> FYI we had some discussion about it, quoting Cyril [1]:
>
> Well technically you can be added into whatever group is set to
> /dev/loop-control e.g. disk group and then you can create devices
> without a need to be a root.
>
> So the most correct solution would be checking if we can access
> /dev/loop-control if tst_test.needs_device is set and if not we would
> imply needs_root. However this would need to be rethinked properly so
> that we do not end up creating something complex and not really
> required.
>
> There is also possibility to add custom device via $LTP_DEV. That might allow to
> add permissions which allow to test without root.
>
> I'll write to automated-testing ML (and maybe to LKML ML) to see if people
> prefers to test without non-root.
I took a quick look at this, and don't like the change.
I didn't investigate all the affected tests, and what device exactly is being protected.
But the overall sense of the change takes makes the authorization checking for tests
less granular.
Fuego often runs tests as 'root', but it is also fairly common in Fuego to have a
dedicated testing user account on a device under test, that has permissions
for things like mounting, access to device nodes, etc. This change
would cause tests to break for that account.
That's my 2 cents.
-- Tim
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root
2022-10-12 18:47 ` Bird, Tim
@ 2022-10-12 19:13 ` Petr Vorel
0 siblings, 0 replies; 11+ messages in thread
From: Petr Vorel @ 2022-10-12 19:13 UTC (permalink / raw)
To: Bird, Tim; +Cc: ltp@lists.linux.it
> > -----Original Message-----
> > From: ltp <ltp-bounces+tim.bird=sony.com@lists.linux.it> On Behalf Of Petr Vorel
> > Hi all,
> > The subject "lib: Add checking of needs_root" is a bit misleading
> > as it does not mention at all that it's for the loop device.
> > > We need to check needs_root is set when tst_test->needs_device or
> > > tst_test->mount_device is set since access the /dev/* need a
> > > privilege.
> > FYI we had some discussion about it, quoting Cyril [1]:
> > Well technically you can be added into whatever group is set to
> > /dev/loop-control e.g. disk group and then you can create devices
> > without a need to be a root.
> > So the most correct solution would be checking if we can access
> > /dev/loop-control if tst_test.needs_device is set and if not we would
> > imply needs_root. However this would need to be rethinked properly so
> > that we do not end up creating something complex and not really
> > required.
> > There is also possibility to add custom device via $LTP_DEV. That might allow to
> > add permissions which allow to test without root.
> > I'll write to automated-testing ML (and maybe to LKML ML) to see if people
> > prefers to test without non-root.
> I took a quick look at this, and don't like the change.
> I didn't investigate all the affected tests, and what device exactly is being protected.
> But the overall sense of the change takes makes the authorization checking for tests
> less granular.
> Fuego often runs tests as 'root', but it is also fairly common in Fuego to have a
> dedicated testing user account on a device under test, that has permissions
> for things like mounting, access to device nodes, etc. This change
> would cause tests to break for that account.
Hi Tim,
thanks a lot for confirming that people are using non-root users for testing.
I'm not sure if we ever implement complex checks, but at least we should not
merge this patchset.
Kind regards,
Petr
> That's my 2 cents.
> -- Tim
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root
@ 2022-10-13 2:32 zhaogongyi via ltp
2022-10-13 7:38 ` Petr Vorel
0 siblings, 1 reply; 11+ messages in thread
From: zhaogongyi via ltp @ 2022-10-13 2:32 UTC (permalink / raw)
To: Petr Vorel, Bird, Tim; +Cc: ltp@lists.linux.it
Hi,
If we neeed to run the test as a non-root user, the non-root user would belong to the root group.
Shall we add a checking of needs_root and needs_rootgroup?
Regards,
Gongyi
>
>
> > > -----Original Message-----
> > > From: ltp <ltp-bounces+tim.bird=sony.com@lists.linux.it> On Behalf
> > > Of Petr Vorel
>
> > > Hi all,
>
> > > The subject "lib: Add checking of needs_root" is a bit misleading as
> > > it does not mention at all that it's for the loop device.
>
> > > > We need to check needs_root is set when tst_test->needs_device
> or
> > > > tst_test->mount_device is set since access the /dev/* need a
> > > > privilege.
>
> > > FYI we had some discussion about it, quoting Cyril [1]:
>
> > > Well technically you can be added into whatever group is set to
> > > /dev/loop-control e.g. disk group and then you can create devices
> > > without a need to be a root.
>
> > > So the most correct solution would be checking if we can access
> > > /dev/loop-control if tst_test.needs_device is set and if not we would
> > > imply needs_root. However this would need to be rethinked properly
> so
> > > that we do not end up creating something complex and not really
> > > required.
>
> > > There is also possibility to add custom device via $LTP_DEV. That
> > > might allow to add permissions which allow to test without root.
>
> > > I'll write to automated-testing ML (and maybe to LKML ML) to see if
> > > people prefers to test without non-root.
>
> > I took a quick look at this, and don't like the change.
>
> > I didn't investigate all the affected tests, and what device exactly is being
> protected.
> > But the overall sense of the change takes makes the authorization
> > checking for tests less granular.
>
> > Fuego often runs tests as 'root', but it is also fairly common in
> > Fuego to have a dedicated testing user account on a device under test,
> > that has permissions for things like mounting, access to device nodes,
> > etc. This change would cause tests to break for that account.
>
> Hi Tim,
>
> thanks a lot for confirming that people are using non-root users for testing.
> I'm not sure if we ever implement complex checks, but at least we should
> not merge this patchset.
>
> Kind regards,
> Petr
>
> > That's my 2 cents.
> > -- Tim
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root
2022-10-13 2:32 [LTP] [PATCH 1/2] lib: Add checking " zhaogongyi via ltp
@ 2022-10-13 7:38 ` Petr Vorel
0 siblings, 0 replies; 11+ messages in thread
From: Petr Vorel @ 2022-10-13 7:38 UTC (permalink / raw)
To: zhaogongyi; +Cc: ltp@lists.linux.it
> Hi,
> If we neeed to run the test as a non-root user, the non-root user would belong to the root group.
> Shall we add a checking of needs_root and needs_rootgroup?
How many of these tests we have? I wonder if it's worth to add this.
Kind regards,
Petr
> Regards,
> Gongyi
> > > > -----Original Message-----
> > > > From: ltp <ltp-bounces+tim.bird=sony.com@lists.linux.it> On Behalf
> > > > Of Petr Vorel
> > > > Hi all,
> > > > The subject "lib: Add checking of needs_root" is a bit misleading as
> > > > it does not mention at all that it's for the loop device.
> > > > > We need to check needs_root is set when tst_test->needs_device
> > or
> > > > > tst_test->mount_device is set since access the /dev/* need a
> > > > > privilege.
> > > > FYI we had some discussion about it, quoting Cyril [1]:
> > > > Well technically you can be added into whatever group is set to
> > > > /dev/loop-control e.g. disk group and then you can create devices
> > > > without a need to be a root.
> > > > So the most correct solution would be checking if we can access
> > > > /dev/loop-control if tst_test.needs_device is set and if not we would
> > > > imply needs_root. However this would need to be rethinked properly
> > so
> > > > that we do not end up creating something complex and not really
> > > > required.
> > > > There is also possibility to add custom device via $LTP_DEV. That
> > > > might allow to add permissions which allow to test without root.
> > > > I'll write to automated-testing ML (and maybe to LKML ML) to see if
> > > > people prefers to test without non-root.
> > > I took a quick look at this, and don't like the change.
> > > I didn't investigate all the affected tests, and what device exactly is being
> > protected.
> > > But the overall sense of the change takes makes the authorization
> > > checking for tests less granular.
> > > Fuego often runs tests as 'root', but it is also fairly common in
> > > Fuego to have a dedicated testing user account on a device under test,
> > > that has permissions for things like mounting, access to device nodes,
> > > etc. This change would cause tests to break for that account.
> > Hi Tim,
> > thanks a lot for confirming that people are using non-root users for testing.
> > I'm not sure if we ever implement complex checks, but at least we should
> > not merge this patchset.
> > Kind regards,
> > Petr
> > > That's my 2 cents.
> > > -- Tim
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [LTP] [PATCH 2/2] needs_root: Add setting of needs_root
2022-10-12 9:15 ` [LTP] [PATCH 2/2] needs_root: Add setting " Zhao Gongyi via ltp
@ 2022-10-13 7:41 ` Petr Vorel
0 siblings, 0 replies; 11+ messages in thread
From: Petr Vorel @ 2022-10-13 7:41 UTC (permalink / raw)
To: Zhao Gongyi; +Cc: ltp
Hi Zhao,
> Add setting of needs_root, otherwise run the test without root
> will fail and report EACCESS or EPERM.
...
> testcases/kernel/syscalls/bind/bind06.c | 1 +
> testcases/kernel/syscalls/cma/process_vm_readv02.c | 1 +
> testcases/kernel/syscalls/cma/process_vm_readv03.c | 1 +
> testcases/kernel/syscalls/cma/process_vm_writev02.c | 1 +
> testcases/kernel/syscalls/copy_file_range/copy_file_range01.c | 1 +
> testcases/kernel/syscalls/ipc/msgget/msgget03.c | 1 +
> testcases/kernel/syscalls/preadv/preadv03.c | 1 +
> testcases/kernel/syscalls/pwritev/pwritev03.c | 1 +
> testcases/kernel/syscalls/sendto/sendto03.c | 1 +
> testcases/kernel/syscalls/setsockopt/setsockopt05.c | 1 +
> testcases/kernel/syscalls/setsockopt/setsockopt06.c | 1 +
> testcases/kernel/syscalls/setsockopt/setsockopt07.c | 1 +
> testcases/kernel/syscalls/setsockopt/setsockopt08.c | 1 +
> testcases/kernel/syscalls/setsockopt/setsockopt09.c | 1 +
> testcases/kernel/syscalls/swapon/swapon01.c | 1 +
> 15 files changed, 15 insertions(+)
...
> +++ b/testcases/kernel/syscalls/ipc/msgget/msgget03.c
> @@ -72,6 +72,7 @@ static void cleanup(void)
> static struct tst_test test = {
> .needs_tmpdir = 1,
> + .needs_root = 1,
> .setup = setup,
> .cleanup = cleanup,
> .test_all = verify_msgget,
At least this one is working without root:
$ ./testcases/kernel/syscalls/ipc/msgget/msgget03
tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s
msgget03.c:41: TINFO: Current environment 0 message queues are already in use
msgget03.c:55: TINFO: The maximum number of message queues (32000) reached
msgget03.c:30: TPASS: msgget(1629588886, 1536) : ENOSPC (28)
Summary:
passed 1
failed 0
broken 0
skipped 0
warnings 0
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root
@ 2022-10-13 7:52 zhaogongyi via ltp
2022-10-13 9:11 ` Petr Vorel
0 siblings, 1 reply; 11+ messages in thread
From: zhaogongyi via ltp @ 2022-10-13 7:52 UTC (permalink / raw)
To: Petr Vorel, Bird, Tim; +Cc: ltp@lists.linux.it
Hi petr,
The testcases referenced mount_device and needs_device are more than 88 in kernel/syscalls:
./fsopen/fsopen02.c:57: .needs_device = 1,
./quotactl/quotactl04.c:164: .needs_device = 1,
./quotactl/quotactl08.c:219: .needs_device = 1,
./fsconfig/fsconfig02.c:96: .needs_device = 1,
./ioctl/ioctl06.c:56: .needs_device = 1,
./ioctl/ioctl05.c:69: .needs_device = 1,
./statx/statx05.c:121: .needs_device = 1,
./preadv/preadv03.c:137: .mount_device = 1,
./mkdir/mkdir09.c:142: .mount_device = 1,
./fanotify/fanotify22.c:300: .mount_device = 1,
./fanotify/fanotify05.c:216: .mount_device = 1,
./fanotify/fanotify06.c:244: .mount_device = 1,
./fanotify/fanotify15.c:288: .mount_device = 1,
./fanotify/fanotify14.c:227: .mount_device = 1,
./fanotify/fanotify18.c:197: .mount_device = 1,
./fanotify/fanotify09.c:513: .mount_device = 1,
./fanotify/fanotify13.c:298: .mount_device = 1,
./fanotify/fanotify19.c:287: .mount_device = 1,
./fanotify/fanotify01.c:359: .mount_device = 1,
./fanotify/fanotify10.c:756: .mount_device = 1,
./fanotify/fanotify03.c:352: .mount_device = 1,
./fanotify/fanotify17.c:261: .mount_device = 1,
./fanotify/fanotify23.c:250: .mount_device = 1,
./fanotify/fanotify16.c:790: .mount_device = 1,
./msync/msync04.c:99: .mount_device = 1,
./pwritev/pwritev03.c:136: .mount_device = 1,
./creat/creat09.c:146: .mount_device = 1,
./sync_file_range/sync_file_range02.c:122: .mount_device = 1,
./fsetxattr/fsetxattr01.c:222: .mount_device = 1,
./inotify/inotify08.c:176: .mount_device = 1,
./inotify/inotify07.c:182: .mount_device = 1,
./fsync/fsync01.c:51: .mount_device = 1,
./fsync/fsync04.c:57: .mount_device = 1,
./setxattr/setxattr01.c:211: .mount_device = 1,
./open_tree/open_tree01.c:70: .mount_device = 1,
./open_tree/open_tree02.c:51: .mount_device = 1,
./prctl/prctl06.c:125: .mount_device = 1,
./sync/sync01.c:54: .mount_device = 1,
./writev/writev03.c:142: .mount_device = 1,
./madvise/madvise12.c:82: .mount_device = 1,
./madvise/madvise13.c:83: .mount_device = 1,
./quotactl/quotactl06.c:224: .mount_device = 1,
./quotactl/quotactl05.c:115: .mount_device = 1,
./quotactl/quotactl02.c:152: .mount_device = 1,
./quotactl/quotactl03.c:85: .mount_device = 1,
./quotactl/quotactl09.c:182: .mount_device = 1,
./quotactl/quotactl01.c:218: .mount_device = 1,
./execveat/execveat03.c:72: .mount_device = 1,
./fremovexattr/fremovexattr02.c:113: .mount_device = 1,
./fremovexattr/fremovexattr01.c:90: .mount_device = 1,
./copy_file_range/copy_file_range02.c:252: .mount_device = 1,
./copy_file_range/copy_file_range01.c:231: .mount_device = 1,
./ioctl/ioctl08.c:123: .mount_device = 1,
./utime/utime05.c:70: .mount_device = 1,
./utime/utime03.c:94: .mount_device = 1,
./utime/utime02.c:83: .mount_device = 1,
./utime/utime04.c:56: .mount_device = 1,
./utime/utime01.c:68: .mount_device = 1,
./ftruncate/ftruncate04.c:179: .mount_device = 1,
./readahead/readahead02.c:403: .mount_device = 1,
./preadv2/preadv203.c:279: .mount_device = 1,
./statx/statx04.c:130: .mount_device = 1,
./statx/statx06.c:157: .mount_device = 1,
./statx/statx08.c:177: .mount_device = 1,
./getxattr/getxattr04.c:110: .mount_device = 1,
./fgetxattr/fgetxattr01.c:142: .mount_device = 1,
./fallocate/fallocate06.c:263: .mount_device = 1,
./fallocate/fallocate05.c:149: .mount_device = 1,
./fallocate/fallocate04.c:308: .mount_device = 1,
./fspick/fspick02.c:50: .mount_device = 1,
./fspick/fspick01.c:63: .mount_device = 1,
./mount_setattr/mount_setattr01.c:130: .mount_device = 1,
./close_range/close_range01.c:194: .mount_device = 1,
./fdatasync/fdatasync03.c:57: .mount_device = 1,
./lremovexattr/lremovexattr01.c:122: .mount_device = 1,
./utimensat/utimensat01.c:318: .mount_device = 1,
./rename/rename12.c:60: .mount_device = 1,
./rename/rename04.c:48: .mount_device = 1,
./rename/rename13.c:47: .mount_device = 1,
./rename/rename06.c:39: .mount_device = 1,
./rename/rename10.c:39: .mount_device = 1,
./rename/rename08.c:40: .mount_device = 1,
./rename/rename05.c:39: .mount_device = 1,
./rename/rename01.c:80: .mount_device = 1,
./rename/rename07.c:40: .mount_device = 1,
./rename/rename03.c:70: .mount_device = 1,
./syncfs/syncfs01.c:63: .mount_device = 1,
./mmap/mmap16.c:177: .mount_device = 1,
>
> If we neeed to run the test as a non-root user, the non-root user would
> belong to the root group.
>
> Shall we add a checking of needs_root and needs_rootgroup?
>
> Regards,
> Gongyi
>
> >
> >
> > > > -----Original Message-----
> > > > From: ltp <ltp-bounces+tim.bird=sony.com@lists.linux.it> On Behalf
> > > > Of Petr Vorel
> >
> > > > Hi all,
> >
> > > > The subject "lib: Add checking of needs_root" is a bit misleading
> > > > as it does not mention at all that it's for the loop device.
> >
> > > > > We need to check needs_root is set when tst_test->needs_device
> > or
> > > > > tst_test->mount_device is set since access the /dev/* need a
> > > > > privilege.
> >
> > > > FYI we had some discussion about it, quoting Cyril [1]:
> >
> > > > Well technically you can be added into whatever group is set to
> > > > /dev/loop-control e.g. disk group and then you can create
> devices
> > > > without a need to be a root.
> >
> > > > So the most correct solution would be checking if we can access
> > > > /dev/loop-control if tst_test.needs_device is set and if not we
> would
> > > > imply needs_root. However this would need to be rethinked
> > > > properly
> > so
> > > > that we do not end up creating something complex and not really
> > > > required.
> >
> > > > There is also possibility to add custom device via $LTP_DEV. That
> > > > might allow to add permissions which allow to test without root.
> >
> > > > I'll write to automated-testing ML (and maybe to LKML ML) to see
> > > > if people prefers to test without non-root.
> >
> > > I took a quick look at this, and don't like the change.
> >
> > > I didn't investigate all the affected tests, and what device exactly
> > > is being
> > protected.
> > > But the overall sense of the change takes makes the authorization
> > > checking for tests less granular.
> >
> > > Fuego often runs tests as 'root', but it is also fairly common in
> > > Fuego to have a dedicated testing user account on a device under
> > > test, that has permissions for things like mounting, access to
> > > device nodes, etc. This change would cause tests to break for that
> account.
> >
> > Hi Tim,
> >
> > thanks a lot for confirming that people are using non-root users for
> testing.
> > I'm not sure if we ever implement complex checks, but at least we
> > should not merge this patchset.
> >
> > Kind regards,
> > Petr
> >
> > > That's my 2 cents.
> > > -- Tim
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root
2022-10-13 7:52 zhaogongyi via ltp
@ 2022-10-13 9:11 ` Petr Vorel
0 siblings, 0 replies; 11+ messages in thread
From: Petr Vorel @ 2022-10-13 9:11 UTC (permalink / raw)
To: zhaogongyi; +Cc: ltp@lists.linux.it
> Hi petr,
> The testcases referenced mount_device and needs_device are more than 88 in kernel/syscalls:
FYI I asked about the number of tests which need needs_rootgroup.
https://lore.kernel.org/ltp/Y0fAgGH6R8uEbcqh@pevik/
I thought these tests are somehow special (use root GID).
But if needs_rootgroup was meant for mount_device and needs_device I don't get
the point why (needs_root would be enough, but as Tim wrote that might be strict
for some people, thus we'd need the cyril's solution mentioned at
https://lore.kernel.org/ltp/Yx8I0ponDUIFC8le@yuki/
Or am I missing something obvious?
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-10-13 9:11 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-12 9:15 [LTP] [PATCH 0/2] Optimization reference to needs_root Zhao Gongyi via ltp
2022-10-12 9:15 ` [LTP] [PATCH 1/2] lib: Add checking of needs_root Zhao Gongyi via ltp
2022-10-12 11:33 ` Petr Vorel
2022-10-12 18:47 ` Bird, Tim
2022-10-12 19:13 ` Petr Vorel
2022-10-12 9:15 ` [LTP] [PATCH 2/2] needs_root: Add setting " Zhao Gongyi via ltp
2022-10-13 7:41 ` Petr Vorel
-- strict thread matches above, loose matches on Subject: below --
2022-10-13 2:32 [LTP] [PATCH 1/2] lib: Add checking " zhaogongyi via ltp
2022-10-13 7:38 ` Petr Vorel
2022-10-13 7:52 zhaogongyi via ltp
2022-10-13 9:11 ` Petr Vorel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox