[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Jan Stancek]

ENOTSUP in userspace is alias for EOPNOTSUPP, so the test still fails.
Add definition of kernel's ENOTSUPP and use that.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 include/lapi/errno.h                                    | 11 +++++++++++
 testcases/kernel/syscalls/timer_create/timer_create01.c |  3 ++-
 2 files changed, 13 insertions(+), 1 deletion(-)
 create mode 100644 include/lapi/errno.h

diff --git a/include/lapi/errno.h b/include/lapi/errno.h
new file mode 100644
index 000000000000..22c2d4d279d8
--- /dev/null
+++ b/include/lapi/errno.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2019 Linux Test Project
+ */
+
+#ifndef _LAPI_ERRNO_H_
+#define _LAPI_ERRNO_H_
+
+#define K_ENOTSUPP	524	/* Operation is not supported */
+
+#endif
diff --git a/testcases/kernel/syscalls/timer_create/timer_create01.c b/testcases/kernel/syscalls/timer_create/timer_create01.c
index 1d0e400d3260..968f917e7876 100644
--- a/testcases/kernel/syscalls/timer_create/timer_create01.c
+++ b/testcases/kernel/syscalls/timer_create/timer_create01.c
@@ -27,6 +27,7 @@
 #include <time.h>
 #include "tst_test.h"
 #include "tst_safe_macros.h"
+#include "lapi/errno.h"
 #include "lapi/common_timers.h"
 
 static struct notif_type {
@@ -81,7 +82,7 @@ static void run(unsigned int n)
 
 		if (TST_RET != 0) {
 			if (possibly_unsupported(clock) &&
-			    (TST_ERR == EINVAL || TST_ERR == ENOTSUP)) {
+			    (TST_ERR == EINVAL || TST_ERR == K_ENOTSUPP)) {
 				tst_res(TPASS | TTERRNO,
 					"%s unsupported, failed as expected",
 					get_clock_str(clock));
-- 
1.8.3.1

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Jan Stancek]

----- Original Message -----
> ENOTSUP in userspace is alias for EOPNOTSUPP, so the test still fails.
> Add definition of kernel's ENOTSUPP and use that.
> 
> Signed-off-by: Jan Stancek <jstancek@redhat.com>

I think I misread the original commit. Seems we want this to fail for ENOTSUPP.

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Cyril Hrubis]

Hi!
> > ENOTSUP in userspace is alias for EOPNOTSUPP, so the test still fails.
> > Add definition of kernel's ENOTSUPP and use that.
> > 
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> 
> I think I misread the original commit. Seems we want this to fail for ENOTSUPP.

Indeed.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Thadeu Lima de Souza Cascardo]

On Wed, Oct 23, 2019 at 09:58:53AM -0400, Jan Stancek wrote:
> 
> 
> ----- Original Message -----
> > ENOTSUP in userspace is alias for EOPNOTSUPP, so the test still fails.
> > Add definition of kernel's ENOTSUPP and use that.
> > 
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> 
> I think I misread the original commit. Seems we want this to fail for ENOTSUPP.
> 

You are right. We want this to succeed with EOPNOTSUPP, as it's a valid error
from kernel space.

However, we don't want to validate kernels that wrongly return ENOTSUPP, as
it's not a valid userspace errno. Any kernel code that exposes ENOTSUPP to
userspace is buggy and should be fixed. So, in fact, we would like to flag such
return values as failures/kernel bugs.

Cascardo.

> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Martin Doucha]

On 10/23/19 4:04 PM, Thadeu Lima de Souza Cascardo wrote:
> You are right. We want this to succeed with EOPNOTSUPP, as it's a valid error
> from kernel space.

Actually, man says that EOPNOTSUPP is only valid for socket operations.
So no, we should not go out of our way to explicitly check timer errors
against EOPNOTSUPP either. (It's also a waste of time because on Linux,
ENOTSUP == EOPNOTSUPP).

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Cyril Hrubis]

Hi!
> > You are right. We want this to succeed with EOPNOTSUPP, as it's a valid error
> > from kernel space.
> 
> Actually, man says that EOPNOTSUPP is only valid for socket operations.
> So no, we should not go out of our way to explicitly check timer errors
> against EOPNOTSUPP either. (It's also a waste of time because on Linux,
> ENOTSUP == EOPNOTSUPP).

Beware that kernel defines ENOTSUP that is not equal to EOPNOTSUPP and
in this case this value leaked to userspace leading to invalid userspace
errno value.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Thadeu Lima de Souza Cascardo]

On Wed, Oct 23, 2019 at 04:35:11PM +0200, Cyril Hrubis wrote:
> Hi!
> > > You are right. We want this to succeed with EOPNOTSUPP, as it's a valid error
> > > from kernel space.
> > 
> > Actually, man says that EOPNOTSUPP is only valid for socket operations.
> > So no, we should not go out of our way to explicitly check timer errors
> > against EOPNOTSUPP either. (It's also a waste of time because on Linux,
> > ENOTSUP == EOPNOTSUPP).
> 
> Beware that kernel defines ENOTSUP that is not equal to EOPNOTSUPP and
> in this case this value leaked to userspace leading to invalid userspace
> errno value.

That was ENOTSUPP (the internal kernel error, defined as 524). ENOTSUP, defined
as EOPNOTSUPP, is the userspace error I guess Martin is saying should not be
used either.

In that case, we need to fix the kernel to return EINVAL instead. Looking at
older changes here, I see commit 98d6f4dd84a134d942827584a3c5f67ffd8ec35f
("alarmtimer: return EINVAL instead of ENOTSUPP if rtcdev doesn't exist")
claiming exactly this. Though it was about clock_getres and clock_gettime,
quoting from that commit:

"
    Second, Posix and Linux man pages agree that clock_gettime and
    clock_getres should return EINVAL if clk_id argument is invalid.
    While the arugment that the clockid is valid, but just not supported
    on this hardware could be made, this is just a technicality that
    doesn't help userspace applicaitons, and only complicates error
    handling.
"

> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Cyril Hrubis]

Hi!
> > Beware that kernel defines ENOTSUP that is not equal to EOPNOTSUPP and
> > in this case this value leaked to userspace leading to invalid userspace
> > errno value.
> 
> That was ENOTSUPP (the internal kernel error, defined as 524). ENOTSUP, defined
> as EOPNOTSUPP, is the userspace error I guess Martin is saying should not be
> used either.

Ah, right, I misunderstand that.

> In that case, we need to fix the kernel to return EINVAL instead. Looking at
> older changes here, I see commit 98d6f4dd84a134d942827584a3c5f67ffd8ec35f
> ("alarmtimer: return EINVAL instead of ENOTSUPP if rtcdev doesn't exist")
> claiming exactly this. Though it was about clock_getres and clock_gettime,
> quoting from that commit:
> 
> "
>     Second, Posix and Linux man pages agree that clock_gettime and
>     clock_getres should return EINVAL if clk_id argument is invalid.
>     While the arugment that the clockid is valid, but just not supported
>     on this hardware could be made, this is just a technicality that
>     doesn't help userspace applicaitons, and only complicates error
>     handling.
> "

I would disagree, if you check latest POSIX it has:

[ENOTSUP]
    The implementation does not support the creation of a timer attached
    to the CPU-time clock that is specified by clock_id and associated
    with a process or thread different from the process or thread
    invoking timer_create().

https://pubs.opengroup.org/onlinepubs/9699919799/

So the implementation is required to return ENOTSUPP in certain cases
anyways so applying it to CLOCK_REALTIME_ALARM and
CLOCK_BOOTTIME_ALARM certainly makes sense.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Cyril Hrubis]

Hi!
> I would disagree, if you check latest POSIX it has:
> 
> [ENOTSUP]
>     The implementation does not support the creation of a timer attached
>     to the CPU-time clock that is specified by clock_id and associated
>     with a process or thread different from the process or thread
>     invoking timer_create().
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/
> 
> So the implementation is required to return ENOTSUPP in certain cases
                                               ^
					       And again, should have
					       been ENOTSUP!
> anyways so applying it to CLOCK_REALTIME_ALARM and
> CLOCK_BOOTTIME_ALARM certainly makes sense.
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] timer_create01: accept kernel ENOTSUPP

[Thadeu Lima de Souza Cascardo]

On Wed, Oct 23, 2019 at 05:36:31PM +0200, Cyril Hrubis wrote:
> Hi!
> > > Beware that kernel defines ENOTSUP that is not equal to EOPNOTSUPP and
> > > in this case this value leaked to userspace leading to invalid userspace
> > > errno value.
> > 
> > That was ENOTSUPP (the internal kernel error, defined as 524). ENOTSUP, defined
> > as EOPNOTSUPP, is the userspace error I guess Martin is saying should not be
> > used either.
> 
> Ah, right, I misunderstand that.
> 
> > In that case, we need to fix the kernel to return EINVAL instead. Looking at
> > older changes here, I see commit 98d6f4dd84a134d942827584a3c5f67ffd8ec35f
> > ("alarmtimer: return EINVAL instead of ENOTSUPP if rtcdev doesn't exist")
> > claiming exactly this. Though it was about clock_getres and clock_gettime,
> > quoting from that commit:
> > 
> > "
> >     Second, Posix and Linux man pages agree that clock_gettime and
> >     clock_getres should return EINVAL if clk_id argument is invalid.
> >     While the arugment that the clockid is valid, but just not supported
> >     on this hardware could be made, this is just a technicality that
> >     doesn't help userspace applicaitons, and only complicates error
> >     handling.
> > "
> 
> I would disagree, if you check latest POSIX it has:
> 
> [ENOTSUP]
>     The implementation does not support the creation of a timer attached
>     to the CPU-time clock that is specified by clock_id and associated
>     with a process or thread different from the process or thread
>     invoking timer_create().
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/
> 
> So the implementation is required to return ENOTSUPP in certain cases
> anyways so applying it to CLOCK_REALTIME_ALARM and
> CLOCK_BOOTTIME_ALARM certainly makes sense.
> 

So, if this is a matter of EOPNOTSUPP versus ENOTSUP (the userspace ones), then
the code that is applied to LTP uses ENOTSUP, which is what POSIX uses, so all
fine from the LTP standpoint.

To be honest, I am relieved about not getting to go through the process of
fixing this in the kernel once again.

Maybe we should even do the opposite and make clock_gettime and clock_getres
return ENOTSUP/EOPNOTSUPP.

Cascardo.

> -- 
> Cyril Hrubis
> chrubis@suse.cz