[PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[Florian Bezdeka]

According to the POSIX spec the value of the timeout parameter needs
not to be validated if the mutex/semaphore could be taken immediately.

While the implementation of the semaphore timedwait (sem_timedwait())
allowed an invalid timeout pthread_mutex_timedlock() was failing with
-EFAULT in case the mutex could be taken immediately.

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---

This was detected while preparing y2038 stuff. Not sure if that should
go into 3.2. Comments welcome...

CCed Philippe because he was already involved some (long) time ago.

Regards,
Florian


 kernel/cobalt/posix/mutex.c                |  5 ++
 testsuite/smokey/posix-mutex/posix-mutex.c | 60 ++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/kernel/cobalt/posix/mutex.c b/kernel/cobalt/posix/mutex.c
index 70fe7960a..01478978e 100644
--- a/kernel/cobalt/posix/mutex.c
+++ b/kernel/cobalt/posix/mutex.c
@@ -167,6 +167,11 @@ redo:
 	xnthread_commit_ceiling(curr);
 
 	if (xnsynch_owner_check(&mutex->synchbase, curr)) {
+		/* Check if we can take the mutex immediately */
+		ret = xnsynch_try_acquire(&mutex->synchbase);
+		if (ret != -EBUSY)
+			goto out;
+
 		if (fetch_timeout) {
 			xnlock_put_irqrestore(&nklock, s);
 			ret = fetch_timeout(&ts, u_ts);
diff --git a/testsuite/smokey/posix-mutex/posix-mutex.c b/testsuite/smokey/posix-mutex/posix-mutex.c
index e5793c42c..4aad24964 100644
--- a/testsuite/smokey/posix-mutex/posix-mutex.c
+++ b/testsuite/smokey/posix-mutex/posix-mutex.c
@@ -1002,6 +1002,65 @@ static int protect_handover(void)
 	return 0;
 }
 
+static void *mutex_timed_locker_inv_timeout(void *arg)
+{
+	struct locker_context *p = arg;
+	int ret;
+
+	if (__F(ret, pthread_mutex_timedlock(p->mutex, (void *) 0xdeadbeef)) &&
+	    __Tassert(ret == -EFAULT))
+		return (void *)1;
+
+	return NULL;
+}
+
+static int check_timedlock_abstime_validation(void)
+{
+	struct locker_context args;
+	pthread_mutex_t mutex;
+	pthread_t tid;
+	void *status;
+	int ret;
+
+	if (!__T(ret, pthread_mutex_init(&mutex, NULL)))
+		return ret;
+
+	/*
+	 * We don't own the mutex yet, so no need to validate the timeout as
+	 * the mutex can be locked immediately.
+	 *
+	 * The second parameter of phtread_mutex_timedlock() is flagged as
+	 * __nonnull so we take an invalid address instead of NULL.
+	 */
+	if (!__T(ret, pthread_mutex_timedlock(&mutex, (void *) 0xdeadbeef)))
+		return ret;
+
+	/*
+	 * Create a second thread which will have to wait and therefore will
+	 * validate the (invalid) timeout
+	 */
+	args.mutex = &mutex;
+	ret = create_thread(&tid, SCHED_FIFO, THREAD_PRIO_LOW,
+			    mutex_timed_locker_inv_timeout, &args);
+
+	if (ret)
+		return ret;
+
+	if (!__T(ret, pthread_join(tid, &status)))
+		return ret;
+
+	if (!__T(ret, pthread_mutex_unlock(&mutex)))
+		return ret;
+
+	if (!__T(ret, pthread_mutex_destroy(&mutex)))
+		return ret;
+
+	if (!__Fassert(status == NULL))
+		return -EINVAL;
+
+	return 0;
+}
+
 /* Detect obviously wrong execution times. */
 static int check_time_limit(const struct timespec *start,
 			    xnticks_t limit_ns)
@@ -1065,6 +1124,7 @@ static int run_posix_mutex(struct smokey_test *t, int argc, char *const argv[])
 	do_test(protect_dynamic, MAX_100_MS);
 	do_test(protect_trylock, MAX_100_MS);
 	do_test(protect_handover, MAX_100_MS);
+	do_test(check_timedlock_abstime_validation, MAX_100_MS);
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> According to the POSIX spec the value of the timeout parameter needs
> not to be validated if the mutex/semaphore could be taken immediately.
>
> While the implementation of the semaphore timedwait (sem_timedwait())
> allowed an invalid timeout pthread_mutex_timedlock() was failing with
> -EFAULT in case the mutex could be taken immediately.
>
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
>
> This was detected while preparing y2038 stuff. Not sure if that should
> go into 3.2. Comments welcome...
>

upsides: save a few cycles in the non-contended case by not copying in
the timeout from userland uselessly + might help passing some picky
POSIX compliance tests.

downside: adds an extra atomic op in the contended case.

> CCed Philippe because he was already involved some (long) time ago.
>

The patch looks good.

> Regards,
> Florian
>
>
>  kernel/cobalt/posix/mutex.c                |  5 ++
>  testsuite/smokey/posix-mutex/posix-mutex.c | 60 ++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/kernel/cobalt/posix/mutex.c b/kernel/cobalt/posix/mutex.c
> index 70fe7960a..01478978e 100644
> --- a/kernel/cobalt/posix/mutex.c
> +++ b/kernel/cobalt/posix/mutex.c
> @@ -167,6 +167,11 @@ redo:
>  	xnthread_commit_ceiling(curr);
>  
>  	if (xnsynch_owner_check(&mutex->synchbase, curr)) {
> +		/* Check if we can take the mutex immediately */
> +		ret = xnsynch_try_acquire(&mutex->synchbase);
> +		if (ret != -EBUSY)
> +			goto out;
> +
>  		if (fetch_timeout) {
>  			xnlock_put_irqrestore(&nklock, s);
>  			ret = fetch_timeout(&ts, u_ts);
> diff --git a/testsuite/smokey/posix-mutex/posix-mutex.c b/testsuite/smokey/posix-mutex/posix-mutex.c
> index e5793c42c..4aad24964 100644
> --- a/testsuite/smokey/posix-mutex/posix-mutex.c
> +++ b/testsuite/smokey/posix-mutex/posix-mutex.c
> @@ -1002,6 +1002,65 @@ static int protect_handover(void)
>  	return 0;
>  }
>  
> +static void *mutex_timed_locker_inv_timeout(void *arg)
> +{
> +	struct locker_context *p = arg;
> +	int ret;
> +
> +	if (__F(ret, pthread_mutex_timedlock(p->mutex, (void *) 0xdeadbeef)) &&
> +	    __Tassert(ret == -EFAULT))
> +		return (void *)1;
> +
> +	return NULL;
> +}
> +
> +static int check_timedlock_abstime_validation(void)
> +{
> +	struct locker_context args;
> +	pthread_mutex_t mutex;
> +	pthread_t tid;
> +	void *status;
> +	int ret;
> +
> +	if (!__T(ret, pthread_mutex_init(&mutex, NULL)))
> +		return ret;
> +
> +	/*
> +	 * We don't own the mutex yet, so no need to validate the timeout as
> +	 * the mutex can be locked immediately.
> +	 *
> +	 * The second parameter of phtread_mutex_timedlock() is flagged as
> +	 * __nonnull so we take an invalid address instead of NULL.
> +	 */
> +	if (!__T(ret, pthread_mutex_timedlock(&mutex, (void *) 0xdeadbeef)))
> +		return ret;
> +
> +	/*
> +	 * Create a second thread which will have to wait and therefore will
> +	 * validate the (invalid) timeout
> +	 */
> +	args.mutex = &mutex;
> +	ret = create_thread(&tid, SCHED_FIFO, THREAD_PRIO_LOW,
> +			    mutex_timed_locker_inv_timeout, &args);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (!__T(ret, pthread_join(tid, &status)))
> +		return ret;
> +
> +	if (!__T(ret, pthread_mutex_unlock(&mutex)))
> +		return ret;
> +
> +	if (!__T(ret, pthread_mutex_destroy(&mutex)))
> +		return ret;
> +
> +	if (!__Fassert(status == NULL))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  /* Detect obviously wrong execution times. */
>  static int check_time_limit(const struct timespec *start,
>  			    xnticks_t limit_ns)
> @@ -1065,6 +1124,7 @@ static int run_posix_mutex(struct smokey_test *t, int argc, char *const argv[])
>  	do_test(protect_dynamic, MAX_100_MS);
>  	do_test(protect_trylock, MAX_100_MS);
>  	do_test(protect_handover, MAX_100_MS);
> +	do_test(check_timedlock_abstime_validation, MAX_100_MS);
>  
>  	return 0;
>  }


-- 
Philippe.

Re: [PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[Jan Kiszka]

On 29.07.21 17:06, Philippe Gerum wrote:
> 
> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> 
>> According to the POSIX spec the value of the timeout parameter needs
>> not to be validated if the mutex/semaphore could be taken immediately.
>>
>> While the implementation of the semaphore timedwait (sem_timedwait())
>> allowed an invalid timeout pthread_mutex_timedlock() was failing with
>> -EFAULT in case the mutex could be taken immediately.
>>
>> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
>> ---
>>
>> This was detected while preparing y2038 stuff. Not sure if that should
>> go into 3.2. Comments welcome...
>>
> 
> upsides: save a few cycles in the non-contended case by not copying in
> the timeout from userland uselessly + might help passing some picky
> POSIX compliance tests.
> 
> downside: adds an extra atomic op in the contended case.
> 

How do we compare to libc implementations here?

Jan

>> CCed Philippe because he was already involved some (long) time ago.
>>
> 
> The patch looks good.
> 
>> Regards,
>> Florian
>>
>>
>>  kernel/cobalt/posix/mutex.c                |  5 ++
>>  testsuite/smokey/posix-mutex/posix-mutex.c | 60 ++++++++++++++++++++++
>>  2 files changed, 65 insertions(+)
>>
>> diff --git a/kernel/cobalt/posix/mutex.c b/kernel/cobalt/posix/mutex.c
>> index 70fe7960a..01478978e 100644
>> --- a/kernel/cobalt/posix/mutex.c
>> +++ b/kernel/cobalt/posix/mutex.c
>> @@ -167,6 +167,11 @@ redo:
>>  	xnthread_commit_ceiling(curr);
>>  
>>  	if (xnsynch_owner_check(&mutex->synchbase, curr)) {
>> +		/* Check if we can take the mutex immediately */
>> +		ret = xnsynch_try_acquire(&mutex->synchbase);
>> +		if (ret != -EBUSY)
>> +			goto out;
>> +
>>  		if (fetch_timeout) {
>>  			xnlock_put_irqrestore(&nklock, s);
>>  			ret = fetch_timeout(&ts, u_ts);
>> diff --git a/testsuite/smokey/posix-mutex/posix-mutex.c b/testsuite/smokey/posix-mutex/posix-mutex.c
>> index e5793c42c..4aad24964 100644
>> --- a/testsuite/smokey/posix-mutex/posix-mutex.c
>> +++ b/testsuite/smokey/posix-mutex/posix-mutex.c
>> @@ -1002,6 +1002,65 @@ static int protect_handover(void)
>>  	return 0;
>>  }
>>  
>> +static void *mutex_timed_locker_inv_timeout(void *arg)
>> +{
>> +	struct locker_context *p = arg;
>> +	int ret;
>> +
>> +	if (__F(ret, pthread_mutex_timedlock(p->mutex, (void *) 0xdeadbeef)) &&
>> +	    __Tassert(ret == -EFAULT))
>> +		return (void *)1;
>> +
>> +	return NULL;
>> +}
>> +
>> +static int check_timedlock_abstime_validation(void)
>> +{
>> +	struct locker_context args;
>> +	pthread_mutex_t mutex;
>> +	pthread_t tid;
>> +	void *status;
>> +	int ret;
>> +
>> +	if (!__T(ret, pthread_mutex_init(&mutex, NULL)))
>> +		return ret;
>> +
>> +	/*
>> +	 * We don't own the mutex yet, so no need to validate the timeout as
>> +	 * the mutex can be locked immediately.
>> +	 *
>> +	 * The second parameter of phtread_mutex_timedlock() is flagged as
>> +	 * __nonnull so we take an invalid address instead of NULL.
>> +	 */
>> +	if (!__T(ret, pthread_mutex_timedlock(&mutex, (void *) 0xdeadbeef)))
>> +		return ret;
>> +
>> +	/*
>> +	 * Create a second thread which will have to wait and therefore will
>> +	 * validate the (invalid) timeout
>> +	 */
>> +	args.mutex = &mutex;
>> +	ret = create_thread(&tid, SCHED_FIFO, THREAD_PRIO_LOW,
>> +			    mutex_timed_locker_inv_timeout, &args);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!__T(ret, pthread_join(tid, &status)))
>> +		return ret;
>> +
>> +	if (!__T(ret, pthread_mutex_unlock(&mutex)))
>> +		return ret;
>> +
>> +	if (!__T(ret, pthread_mutex_destroy(&mutex)))
>> +		return ret;
>> +
>> +	if (!__Fassert(status == NULL))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>  /* Detect obviously wrong execution times. */
>>  static int check_time_limit(const struct timespec *start,
>>  			    xnticks_t limit_ns)
>> @@ -1065,6 +1124,7 @@ static int run_posix_mutex(struct smokey_test *t, int argc, char *const argv[])
>>  	do_test(protect_dynamic, MAX_100_MS);
>>  	do_test(protect_trylock, MAX_100_MS);
>>  	do_test(protect_handover, MAX_100_MS);
>> +	do_test(check_timedlock_abstime_validation, MAX_100_MS);
>>  
>>  	return 0;
>>  }
> 
> 

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[Bezdeka, Florian]

On Thu, 2021-07-29 at 17:45 +0200, Jan Kiszka wrote:
> On 29.07.21 17:06, Philippe Gerum wrote:
> > 
> > Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> > 
> > > According to the POSIX spec the value of the timeout parameter needs
> > > not to be validated if the mutex/semaphore could be taken immediately.
> > > 
> > > While the implementation of the semaphore timedwait (sem_timedwait())
> > > allowed an invalid timeout pthread_mutex_timedlock() was failing with
> > > -EFAULT in case the mutex could be taken immediately.
> > > 
> > > Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> > > ---
> > > 
> > > This was detected while preparing y2038 stuff. Not sure if that should
> > > go into 3.2. Comments welcome...
> > > 
> > 
> > upsides: save a few cycles in the non-contended case by not copying in
> > the timeout from userland uselessly + might help passing some picky
> > POSIX compliance tests.
> > 
> > downside: adds an extra atomic op in the contended case.
> > 
> 
> How do we compare to libc implementations here?

I changed the validation done on cobalt side, not libcobalt. So what
exactly do you mean?

Calling pthread_mutex_timedlock() on a mutex not yet taken with an
invalid / NULL argument for abstime does not report any error. The code
below was tested on my local glibc 2.32:

#include <pthread.h>

int main(int argc, char **argv)
{
	pthread_mutex_t mutex;
	int ret;

	ret = pthread_mutex_init(&mutex, NULL);	
	if (ret)
		return ret;

	ret = pthread_mutex_timedlock(&mutex, (void *) 0xdeadbeef);
	if (ret)
		return ret;

	// A second call with invalid timeout would deliver a segfault

	ret = pthread_mutex_unlock(&mutex);
	if (ret)
		return ret;

	ret = pthread_mutex_destroy(&mutex);
	if (ret)
		return ret;

	return 0;
}

> 
> Jan
> 
> > > CCed Philippe because he was already involved some (long) time ago.
> > > 
> > 
> > The patch looks good.
> > 
> > > Regards,
> > > Florian
> > > 
> > > 
> > >  kernel/cobalt/posix/mutex.c                |  5 ++
> > >  testsuite/smokey/posix-mutex/posix-mutex.c | 60 ++++++++++++++++++++++
> > >  2 files changed, 65 insertions(+)
> > > 
> > > diff --git a/kernel/cobalt/posix/mutex.c b/kernel/cobalt/posix/mutex.c
> > > index 70fe7960a..01478978e 100644
> > > --- a/kernel/cobalt/posix/mutex.c
> > > +++ b/kernel/cobalt/posix/mutex.c
> > > @@ -167,6 +167,11 @@ redo:
> > >  	xnthread_commit_ceiling(curr);
> > >  
> > > 
> > > 
> > > 
> > >  	if (xnsynch_owner_check(&mutex->synchbase, curr)) {
> > > +		/* Check if we can take the mutex immediately */
> > > +		ret = xnsynch_try_acquire(&mutex->synchbase);
> > > +		if (ret != -EBUSY)
> > > +			goto out;
> > > +
> > >  		if (fetch_timeout) {
> > >  			xnlock_put_irqrestore(&nklock, s);
> > >  			ret = fetch_timeout(&ts, u_ts);
> > > diff --git a/testsuite/smokey/posix-mutex/posix-mutex.c b/testsuite/smokey/posix-mutex/posix-mutex.c
> > > index e5793c42c..4aad24964 100644
> > > --- a/testsuite/smokey/posix-mutex/posix-mutex.c
> > > +++ b/testsuite/smokey/posix-mutex/posix-mutex.c
> > > @@ -1002,6 +1002,65 @@ static int protect_handover(void)
> > >  	return 0;
> > >  }
> > >  
> > > 
> > > 
> > > 
> > > +static void *mutex_timed_locker_inv_timeout(void *arg)
> > > +{
> > > +	struct locker_context *p = arg;
> > > +	int ret;
> > > +
> > > +	if (__F(ret, pthread_mutex_timedlock(p->mutex, (void *) 0xdeadbeef)) &&
> > > +	    __Tassert(ret == -EFAULT))
> > > +		return (void *)1;
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +static int check_timedlock_abstime_validation(void)
> > > +{
> > > +	struct locker_context args;
> > > +	pthread_mutex_t mutex;
> > > +	pthread_t tid;
> > > +	void *status;
> > > +	int ret;
> > > +
> > > +	if (!__T(ret, pthread_mutex_init(&mutex, NULL)))
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * We don't own the mutex yet, so no need to validate the timeout as
> > > +	 * the mutex can be locked immediately.
> > > +	 *
> > > +	 * The second parameter of phtread_mutex_timedlock() is flagged as
> > > +	 * __nonnull so we take an invalid address instead of NULL.
> > > +	 */
> > > +	if (!__T(ret, pthread_mutex_timedlock(&mutex, (void *) 0xdeadbeef)))
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Create a second thread which will have to wait and therefore will
> > > +	 * validate the (invalid) timeout
> > > +	 */
> > > +	args.mutex = &mutex;
> > > +	ret = create_thread(&tid, SCHED_FIFO, THREAD_PRIO_LOW,
> > > +			    mutex_timed_locker_inv_timeout, &args);
> > > +
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!__T(ret, pthread_join(tid, &status)))
> > > +		return ret;
> > > +
> > > +	if (!__T(ret, pthread_mutex_unlock(&mutex)))
> > > +		return ret;
> > > +
> > > +	if (!__T(ret, pthread_mutex_destroy(&mutex)))
> > > +		return ret;
> > > +
> > > +	if (!__Fassert(status == NULL))
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /* Detect obviously wrong execution times. */
> > >  static int check_time_limit(const struct timespec *start,
> > >  			    xnticks_t limit_ns)
> > > @@ -1065,6 +1124,7 @@ static int run_posix_mutex(struct smokey_test *t, int argc, char *const argv[])
> > >  	do_test(protect_dynamic, MAX_100_MS);
> > >  	do_test(protect_trylock, MAX_100_MS);
> > >  	do_test(protect_handover, MAX_100_MS);
> > > +	do_test(check_timedlock_abstime_validation, MAX_100_MS);
> > >  
> > > 
> > > 
> > > 
> > >  	return 0;
> > >  }
> > 
> > 
> 

Re: [PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[Jan Kiszka]

On 29.07.21 18:22, Bezdeka, Florian (T RDA IOT SES-DE) wrote:
> On Thu, 2021-07-29 at 17:45 +0200, Jan Kiszka wrote:
>> On 29.07.21 17:06, Philippe Gerum wrote:
>>>
>>> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>>>
>>>> According to the POSIX spec the value of the timeout parameter needs
>>>> not to be validated if the mutex/semaphore could be taken immediately.
>>>>
>>>> While the implementation of the semaphore timedwait (sem_timedwait())
>>>> allowed an invalid timeout pthread_mutex_timedlock() was failing with
>>>> -EFAULT in case the mutex could be taken immediately.
>>>>
>>>> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
>>>> ---
>>>>
>>>> This was detected while preparing y2038 stuff. Not sure if that should
>>>> go into 3.2. Comments welcome...
>>>>
>>>
>>> upsides: save a few cycles in the non-contended case by not copying in
>>> the timeout from userland uselessly + might help passing some picky
>>> POSIX compliance tests.
>>>
>>> downside: adds an extra atomic op in the contended case.
>>>
>>
>> How do we compare to libc implementations here?
> 
> I changed the validation done on cobalt side, not libcobalt. So what
> exactly do you mean?
> 

Indeed, I was looking for the fast or almost fast paths in userspace.

But now I'm confused: The uncontended case is generally not supposed to
trigger a syscall. That is always the slow path anyway, e.g. because ned
to migrated first (likely how you were able to trigger the issue?).

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[Bezdeka, Florian]

On Thu, 2021-07-29 at 19:29 +0200, Jan Kiszka wrote:
> On 29.07.21 18:22, Bezdeka, Florian (T RDA IOT SES-DE) wrote:
> > On Thu, 2021-07-29 at 17:45 +0200, Jan Kiszka wrote:
> > > On 29.07.21 17:06, Philippe Gerum wrote:
> > > > 
> > > > Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> > > > 
> > > > > According to the POSIX spec the value of the timeout parameter needs
> > > > > not to be validated if the mutex/semaphore could be taken immediately.
> > > > > 
> > > > > While the implementation of the semaphore timedwait (sem_timedwait())
> > > > > allowed an invalid timeout pthread_mutex_timedlock() was failing with
> > > > > -EFAULT in case the mutex could be taken immediately.
> > > > > 
> > > > > Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> > > > > ---
> > > > > 
> > > > > This was detected while preparing y2038 stuff. Not sure if that should
> > > > > go into 3.2. Comments welcome...
> > > > > 
> > > > 
> > > > upsides: save a few cycles in the non-contended case by not copying in
> > > > the timeout from userland uselessly + might help passing some picky
> > > > POSIX compliance tests.
> > > > 
> > > > downside: adds an extra atomic op in the contended case.
> > > > 
> > > 
> > > How do we compare to libc implementations here?
> > 
> > I changed the validation done on cobalt side, not libcobalt. So what
> > exactly do you mean?
> > 
> 
> Indeed, I was looking for the fast or almost fast paths in userspace.
> 
> But now I'm confused: The uncontended case is generally not supposed to
> trigger a syscall. That is always the slow path anyway, e.g. because ned
> to migrated first (likely how you were able to trigger the issue?).

The fast-path was always fine, that is right. But please note that
depending on the kernel configuration (especially with
CONFIG_XENO_OPT_DEBUG_MUTEX_SLEEP enabled) we will always be forced
into the slow-path.

So we had another imbalance depending on the kernel configuration.

> 
> Jan
> 

Re: [PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[Jan Kiszka]

On 30.07.21 09:14, Bezdeka, Florian (T RDA IOT SES-DE) wrote:
> On Thu, 2021-07-29 at 19:29 +0200, Jan Kiszka wrote:
>> On 29.07.21 18:22, Bezdeka, Florian (T RDA IOT SES-DE) wrote:
>>> On Thu, 2021-07-29 at 17:45 +0200, Jan Kiszka wrote:
>>>> On 29.07.21 17:06, Philippe Gerum wrote:
>>>>>
>>>>> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>>>>>
>>>>>> According to the POSIX spec the value of the timeout parameter needs
>>>>>> not to be validated if the mutex/semaphore could be taken immediately.
>>>>>>
>>>>>> While the implementation of the semaphore timedwait (sem_timedwait())
>>>>>> allowed an invalid timeout pthread_mutex_timedlock() was failing with
>>>>>> -EFAULT in case the mutex could be taken immediately.
>>>>>>
>>>>>> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
>>>>>> ---
>>>>>>
>>>>>> This was detected while preparing y2038 stuff. Not sure if that should
>>>>>> go into 3.2. Comments welcome...
>>>>>>
>>>>>
>>>>> upsides: save a few cycles in the non-contended case by not copying in
>>>>> the timeout from userland uselessly + might help passing some picky
>>>>> POSIX compliance tests.
>>>>>
>>>>> downside: adds an extra atomic op in the contended case.
>>>>>
>>>>
>>>> How do we compare to libc implementations here?
>>>
>>> I changed the validation done on cobalt side, not libcobalt. So what
>>> exactly do you mean?
>>>
>>
>> Indeed, I was looking for the fast or almost fast paths in userspace.
>>
>> But now I'm confused: The uncontended case is generally not supposed to
>> trigger a syscall. That is always the slow path anyway, e.g. because ned
>> to migrated first (likely how you were able to trigger the issue?).
> 
> The fast-path was always fine, that is right. But please note that
> depending on the kernel configuration (especially with
> CONFIG_XENO_OPT_DEBUG_MUTEX_SLEEP enabled) we will always be forced
> into the slow-path.
> 
> So we had another imbalance depending on the kernel configuration.
> 

OK, I just wanted to understand whether that single atomic more is an
issue - it's not at all, we are slow already at that point.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] cobalt/posix/mutex: Harmonize pthread_mutex_timedlock() and sem_timedwait()

[Jan Kiszka]

On 29.07.21 16:47, Florian Bezdeka wrote:
> According to the POSIX spec the value of the timeout parameter needs
> not to be validated if the mutex/semaphore could be taken immediately.
> 
> While the implementation of the semaphore timedwait (sem_timedwait())
> allowed an invalid timeout pthread_mutex_timedlock() was failing with
> -EFAULT in case the mutex could be taken immediately.
> 
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
> 
> This was detected while preparing y2038 stuff. Not sure if that should
> go into 3.2. Comments welcome...
> 
> CCed Philippe because he was already involved some (long) time ago.
> 
> Regards,
> Florian
> 
> 
>  kernel/cobalt/posix/mutex.c                |  5 ++
>  testsuite/smokey/posix-mutex/posix-mutex.c | 60 ++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/kernel/cobalt/posix/mutex.c b/kernel/cobalt/posix/mutex.c
> index 70fe7960a..01478978e 100644
> --- a/kernel/cobalt/posix/mutex.c
> +++ b/kernel/cobalt/posix/mutex.c
> @@ -167,6 +167,11 @@ redo:
>  	xnthread_commit_ceiling(curr);
>  
>  	if (xnsynch_owner_check(&mutex->synchbase, curr)) {
> +		/* Check if we can take the mutex immediately */
> +		ret = xnsynch_try_acquire(&mutex->synchbase);
> +		if (ret != -EBUSY)
> +			goto out;
> +
>  		if (fetch_timeout) {
>  			xnlock_put_irqrestore(&nklock, s);
>  			ret = fetch_timeout(&ts, u_ts);
> diff --git a/testsuite/smokey/posix-mutex/posix-mutex.c b/testsuite/smokey/posix-mutex/posix-mutex.c
> index e5793c42c..4aad24964 100644
> --- a/testsuite/smokey/posix-mutex/posix-mutex.c
> +++ b/testsuite/smokey/posix-mutex/posix-mutex.c
> @@ -1002,6 +1002,65 @@ static int protect_handover(void)
>  	return 0;
>  }
>  
> +static void *mutex_timed_locker_inv_timeout(void *arg)
> +{
> +	struct locker_context *p = arg;
> +	int ret;
> +
> +	if (__F(ret, pthread_mutex_timedlock(p->mutex, (void *) 0xdeadbeef)) &&
> +	    __Tassert(ret == -EFAULT))
> +		return (void *)1;
> +
> +	return NULL;
> +}
> +
> +static int check_timedlock_abstime_validation(void)
> +{
> +	struct locker_context args;
> +	pthread_mutex_t mutex;
> +	pthread_t tid;
> +	void *status;
> +	int ret;
> +
> +	if (!__T(ret, pthread_mutex_init(&mutex, NULL)))
> +		return ret;
> +
> +	/*
> +	 * We don't own the mutex yet, so no need to validate the timeout as
> +	 * the mutex can be locked immediately.
> +	 *
> +	 * The second parameter of phtread_mutex_timedlock() is flagged as
> +	 * __nonnull so we take an invalid address instead of NULL.
> +	 */
> +	if (!__T(ret, pthread_mutex_timedlock(&mutex, (void *) 0xdeadbeef)))
> +		return ret;
> +
> +	/*
> +	 * Create a second thread which will have to wait and therefore will
> +	 * validate the (invalid) timeout
> +	 */
> +	args.mutex = &mutex;
> +	ret = create_thread(&tid, SCHED_FIFO, THREAD_PRIO_LOW,
> +			    mutex_timed_locker_inv_timeout, &args);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (!__T(ret, pthread_join(tid, &status)))
> +		return ret;
> +
> +	if (!__T(ret, pthread_mutex_unlock(&mutex)))
> +		return ret;
> +
> +	if (!__T(ret, pthread_mutex_destroy(&mutex)))
> +		return ret;
> +
> +	if (!__Fassert(status == NULL))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  /* Detect obviously wrong execution times. */
>  static int check_time_limit(const struct timespec *start,
>  			    xnticks_t limit_ns)
> @@ -1065,6 +1124,7 @@ static int run_posix_mutex(struct smokey_test *t, int argc, char *const argv[])
>  	do_test(protect_dynamic, MAX_100_MS);
>  	do_test(protect_trylock, MAX_100_MS);
>  	do_test(protect_handover, MAX_100_MS);
> +	do_test(check_timedlock_abstime_validation, MAX_100_MS);
>  
>  	return 0;
>  }
> 

Thanks, applied.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux