[LTP] [PATCH 1/2] lib: Add checking of needs_root

[LTP] [PATCH 1/2] lib: Add checking of needs_root

[Zhao Gongyi via 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

Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root

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

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

Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root

[Bird, Tim]

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

Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root

[Petr Vorel]

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

Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root

[zhaogongyi via ltp]

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

Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root

[Petr Vorel]

> 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

Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root

[zhaogongyi via ltp]

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

Re: [LTP] [PATCH 1/2] lib: Add checking of needs_root

[Petr Vorel]

> 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