[PATCH 1/5] cobalt/intr: vfile: do not assume all irqs are cobalt-managed

[PATCH 1/5] cobalt/intr: vfile: do not assume all irqs are cobalt-managed

[Philippe Gerum]

Non-cobalt kernel code may hook interrupts using ipipe_request_irq()
directly, which means that xnintr_vec_first() cannot assume that
__ipipe_irq_cookie() always returns a valid xnintr struct for all
irqs.

We need to detect those irqs while iterating over the interrupt
namespace when pulling data from /proc files not to dereference
invalid memory.

Fixes:
/proc/xenomai/irq
/proc/xenomai/sched/stat
/proc/xenomai/sched/acct

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/cobalt/intr.c | 48 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/kernel/cobalt/intr.c b/kernel/cobalt/intr.c
index 184ffad13..ba6409ebb 100644
--- a/kernel/cobalt/intr.c
+++ b/kernel/cobalt/intr.c
@@ -449,6 +449,17 @@ out:
 	}
 }
 
+static inline bool cobalt_owns_irq(int irq)
+{
+	ipipe_irq_handler_t h;
+
+	h = __ipipe_irq_handler(&xnsched_realtime_domain, irq);
+
+	return h == xnintr_vec_handler ||
+		h == xnintr_edge_vec_handler ||
+		h == xnintr_irq_handler;
+}
+
 static inline int xnintr_irq_attach(struct xnintr *intr)
 {
 	struct xnintr_vector *vec = vectors + intr->irq;
@@ -538,9 +549,19 @@ struct xnintr_vector {
 
 static struct xnintr_vector vectors[IPIPE_NR_IRQS];
 
+static inline bool cobalt_owns_irq(int irq)
+{
+	ipipe_irq_handler_t h;
+
+	h = __ipipe_irq_handler(&xnsched_realtime_domain, irq);
+
+	return h == xnintr_irq_handler;
+}
+
 static inline struct xnintr *xnintr_vec_first(unsigned int irq)
 {
-	return __ipipe_irq_cookie(&xnsched_realtime_domain, irq);
+	return cobalt_owns_irq(irq) ?
+		__ipipe_irq_cookie(&xnsched_realtime_domain, irq) : NULL;
 }
 
 static inline struct xnintr *xnintr_vec_next(struct xnintr *prev)
@@ -1067,6 +1088,7 @@ static inline int format_irq_proc(unsigned int irq,
 				  struct xnvfile_regular_iterator *it)
 {
 	struct xnintr *intr;
+	struct irq_desc *d;
 	int cpu;
 
 	for_each_realtime_cpu(cpu)
@@ -1100,15 +1122,21 @@ static inline int format_irq_proc(unsigned int irq,
 
 	mutex_lock(&intrlock);
 
-	intr = xnintr_vec_first(irq);
-	if (intr) {
-		xnvfile_puts(it, "        ");
-
-		do {
-			xnvfile_putc(it, ' ');
-			xnvfile_puts(it, intr->name);
-			intr = xnintr_vec_next(intr);
-		} while (intr);
+	if (!cobalt_owns_irq(irq)) {
+		xnvfile_puts(it, "         ");
+		d = irq_to_desc(irq);
+		xnvfile_puts(it, d && d->name ? d->name : "-");
+	} else {
+		intr = xnintr_vec_first(irq);
+		if (intr) {
+			xnvfile_puts(it, "        ");
+
+			do {
+				xnvfile_putc(it, ' ');
+				xnvfile_puts(it, intr->name);
+				intr = xnintr_vec_next(intr);
+			} while (intr);
+		}
 	}
 
 	mutex_unlock(&intrlock);
-- 
2.17.2

[PATCH 2/5] drivers/autotune: do not read user data we should not expect

[Philippe Gerum]

We receive the sampling period from the setup struct attached to the
AUTOTUNE_RTIOC_{IRQ,KERN,USER} requests, drop the last copy from user
from autotune_ioctl_nrt() which is spurious.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/autotune/autotune.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/drivers/autotune/autotune.c b/kernel/drivers/autotune/autotune.c
index 46a34bf36..2629bf9d4 100644
--- a/kernel/drivers/autotune/autotune.c
+++ b/kernel/drivers/autotune/autotune.c
@@ -644,7 +644,7 @@ static int autotune_ioctl_nrt(struct rtdm_fd *fd, unsigned int request, void *ar
 	struct autotune_context *context;
 	struct autotune_setup setup;
 	struct gravity_tuner *tuner;
-	int period, ret;
+	int ret;
 
 	if (request == AUTOTUNE_RTIOC_RESET) {
 		xnclock_reset_gravity(&nkclock);
@@ -678,10 +678,6 @@ static int autotune_ioctl_nrt(struct rtdm_fd *fd, unsigned int request, void *ar
 		return -EINVAL;
 	}
 
-	ret = rtdm_safe_copy_from_user(fd, &period, arg, sizeof(period));
-	if (ret)
-		return ret;
-
 	ret = tuner->init_tuner(tuner);
 	if (ret)
 		return ret;
-- 
2.17.2

[PATCH 3/5] cobalt/thread: skip boundary check for infinite round-robin time slice

[Philippe Gerum]

Disabling round-robin is obtained by passing XN_INFINITE for quantum
to xnthread_set_slice(), which is zero: we don't want to check such
value against the clock gravity.

As a matter of fact, this bug may have prevented the RR mode to be
switched off for years, causing the routine to always fail early with
-EINVAL.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/cobalt/thread.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
index 04c0b62d3..a43d2f1ad 100644
--- a/kernel/cobalt/thread.c
+++ b/kernel/cobalt/thread.c
@@ -1471,16 +1471,14 @@ int xnthread_set_slice(struct xnthread *thread, xnticks_t quantum)
 	struct xnsched *sched;
 	spl_t s;
 
-	if (quantum <= xnclock_get_gravity(&nkclock, user))
-		return -EINVAL;
-
 	xnlock_get_irqsave(&nklock, s);
 
 	sched = thread->sched;
 	thread->rrperiod = quantum;
 
 	if (quantum != XN_INFINITE) {
-		if (thread->base_class->sched_tick == NULL) {
+		if (quantum <= xnclock_get_gravity(&nkclock, user) ||
+		    thread->base_class->sched_tick == NULL) {
 			xnlock_put_irqrestore(&nklock, s);
 			return -EINVAL;
 		}
-- 
2.17.2

[PATCH 4/5] drivers/ipc: bufp: fix read-write, write-write preemption cases

[Philippe Gerum]

The token-based approach for detecting preemption while data is being
moved into or out of the ring only protects from read vs read races,
not from races involving a write side. For instance, a reader might
read dirty data being changed by a writer concurrently, or two writers
might compete writing two distinct messages at the same place in the
ring space.

To address this issue, use a slot-based implementation which
atomically reserves exclusive portions of the ring space readers and
writers will be accessing locklessly. Those slots are guaranteed to
never overlap among read and write requests, until the lockless
operation finishes.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/ipc/bufp.c | 118 ++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 68 deletions(-)

diff --git a/kernel/drivers/ipc/bufp.c b/kernel/drivers/ipc/bufp.c
index e1c867288..4fa6593c3 100644
--- a/kernel/drivers/ipc/bufp.c
+++ b/kernel/drivers/ipc/bufp.c
@@ -42,10 +42,10 @@ struct bufp_socket {
 	char label[XNOBJECT_NAME_LEN];
 
 	off_t rdoff;
+	off_t rdrsvd;
 	off_t wroff;
+	off_t wrrsvd;
 	size_t fillsz;
-	u_long wrtoken;
-	u_long rdtoken;
 	rtdm_event_t i_event;
 	rtdm_event_t o_event;
 
@@ -115,8 +115,8 @@ static int bufp_socket(struct rtdm_fd *fd)
 	sk->rdoff = 0;
 	sk->wroff = 0;
 	sk->fillsz = 0;
-	sk->rdtoken = 0;
-	sk->wrtoken = 0;
+	sk->rdrsvd = 0;
+	sk->wrrsvd = 0;
 	sk->status = 0;
 	sk->handle = 0;
 	sk->rx_timeout = RTDM_TIMEOUT_INFINITE;
@@ -162,11 +162,10 @@ static ssize_t __bufp_readbuf(struct bufp_socket *sk,
 	struct bufp_wait_context wait, *bufwc;
 	struct rtipc_wait_context *wc;
 	struct xnthread *waiter;
+	size_t rbytes, n, avail;
+	ssize_t len, ret, xret;
 	rtdm_toseq_t toseq;
-	ssize_t len, ret;
-	size_t rbytes, n;
 	rtdm_lockctx_t s;
-	u_long rdtoken;
 	off_t rdoff;
 	int resched;
 
@@ -181,18 +180,15 @@ redo:
 		 * We should be able to read a complete message of the
 		 * requested length, or block.
 		 */
-		if (sk->fillsz < len)
+		avail = sk->fillsz - sk->rdrsvd;
+		if (avail < len)
 			goto wait;
 
-		/*
-		 * Draw the next read token so that we can later
-		 * detect preemption.
-		 */
-		rdtoken = ++sk->rdtoken;
-
-		/* Read from the buffer in a circular way. */
+		/* Reserve a read slot into the circular buffer. */
 		rdoff = sk->rdoff;
-		rbytes = len;
+		sk->rdoff = (rdoff + len) % sk->bufsz;
+		sk->rdrsvd += len;
+		rbytes = ret = len;
 
 		do {
 			if (rdoff + rbytes > sk->bufsz)
@@ -200,37 +196,30 @@ redo:
 			else
 				n = rbytes;
 			/*
-			 * Release the lock while retrieving the data
-			 * to keep latency low.
+			 * Drop the lock before copying data to
+			 * user. The read slot is consumed in any
+			 * case: the non-copied portion of the message
+			 * is lost on bad write.
 			 */
 			cobalt_atomic_leave(s);
-			ret = xnbufd_copy_from_kmem(bufd, sk->bufmem + rdoff, n);
-			if (ret < 0)
-				return ret;
-
+			xret = xnbufd_copy_from_kmem(bufd, sk->bufmem + rdoff, n);
 			cobalt_atomic_enter(s);
-			/*
-			 * In case we were preempted while retrieving
-			 * the message, we have to re-read the whole
-			 * thing.
-			 */
-			if (sk->rdtoken != rdtoken) {
-				xnbufd_reset(bufd);
-				goto redo;
+			if (xret < 0) {
+				ret = -EFAULT;
+				break;
 			}
 
-			rdoff = (rdoff + n) % sk->bufsz;
 			rbytes -= n;
+			rdoff = (rdoff + n) % sk->bufsz;
 		} while (rbytes > 0);
 
-		sk->fillsz -= len;
-		sk->rdoff = rdoff;
-		ret = len;
-
 		resched = 0;
-		if (sk->fillsz + len == sk->bufsz) /* -> writable */
+		if (sk->fillsz == sk->bufsz) /* -> writable */
 			resched |= xnselect_signal(&sk->priv->send_block, POLLOUT);
 
+		sk->rdrsvd -= len;
+		sk->fillsz -= len;
+
 		if (sk->fillsz == 0) /* -> non-readable */
 			resched |= xnselect_signal(&sk->priv->recv_block, 0);
 
@@ -416,11 +405,10 @@ static ssize_t __bufp_writebuf(struct bufp_socket *rsk,
 	struct bufp_wait_context wait, *bufwc;
 	struct rtipc_wait_context *wc;
 	struct xnthread *waiter;
+	size_t wbytes, n, avail;
+	ssize_t len, ret, xret;
 	rtdm_toseq_t toseq;
 	rtdm_lockctx_t s;
-	ssize_t len, ret;
-	size_t wbytes, n;
-	u_long wrtoken;
 	off_t wroff;
 	int resched;
 
@@ -429,24 +417,21 @@ static ssize_t __bufp_writebuf(struct bufp_socket *rsk,
 	rtdm_toseq_init(&toseq, sk->tx_timeout);
 
 	cobalt_atomic_enter(s);
-redo:
+
 	for (;;) {
 		/*
-		 * We should be able to write the entire message at
-		 * once or block.
+		 * No short or scattered writes: we should write the
+		 * entire message atomically or block.
 		 */
-		if (rsk->fillsz + len > rsk->bufsz)
+		avail = rsk->fillsz + rsk->wrrsvd;
+		if (avail + len > rsk->bufsz)
 			goto wait;
 
-		/*
-		 * Draw the next write token so that we can later
-		 * detect preemption.
-		 */
-		wrtoken = ++rsk->wrtoken;
-
-		/* Write to the buffer in a circular way. */
+		/* Reserve a write slot into the circular buffer. */
 		wroff = rsk->wroff;
-		wbytes = len;
+		rsk->wroff = (wroff + len) % rsk->bufsz;
+		rsk->wrrsvd += len;
+		wbytes = ret = len;
 
 		do {
 			if (wroff + wbytes > rsk->bufsz)
@@ -454,33 +439,30 @@ redo:
 			else
 				n = wbytes;
 			/*
-			 * Release the lock while copying the data to
-			 * keep latency low.
+			 * We have to drop the lock while reading in
+			 * data, but we can't rollback on bad read
+			 * from user because some other thread might
+			 * have populated the memory ahead of our
+			 * write slot already: bluntly clear the
+			 * unavailable bytes on copy error.
 			 */
 			cobalt_atomic_leave(s);
-			ret = xnbufd_copy_to_kmem(rsk->bufmem + wroff, bufd, n);
-			if (ret < 0)
-				return ret;
+			xret = xnbufd_copy_to_kmem(rsk->bufmem + wroff, bufd, n);
 			cobalt_atomic_enter(s);
-			/*
-			 * In case we were preempted while copying the
-			 * message, we have to write the whole thing
-			 * again.
-			 */
-			if (rsk->wrtoken != wrtoken) {
-				xnbufd_reset(bufd);
-				goto redo;
+			if (xret < 0) {
+				memset(rsk->bufmem + wroff + n - xret, 0, xret);
+				ret = -EFAULT;
+				break;
 			}
 
-			wroff = (wroff + n) % rsk->bufsz;
 			wbytes -= n;
+			wroff = (wroff + n) % rsk->bufsz;
 		} while (wbytes > 0);
 
 		rsk->fillsz += len;
-		rsk->wroff = wroff;
-		ret = len;
-		resched = 0;
+		rsk->wrrsvd -= len;
 
+		resched = 0;
 		if (rsk->fillsz == len) /* -> readable */
 			resched |= xnselect_signal(&rsk->priv->recv_block, POLLIN);
 
-- 
2.17.2

[PATCH 5/5] testsuite/smokey: posix_clock: prevent false positive in timing-depending test

[Philippe Gerum]

clock_decrease_after_periodic_timer_first_tick checks that periodic
interval timers based on CLOCK_REALTIME are not (pathologically)
affected by the epoch going backwards.

To this end, we measure the actual time observed between two ticks of
a periodic timer based on CLOCK_REALTIME with a call to
clock_settime() injecting a negative offset in between, equivalent to
five ticks.

Due to processing delays induced by clock_settime() and other latency,
we could observe a duration which exceeds a tick by a few tenths of
microseconds. Since we can't anticipate the amount of latency
involved, let's accept a longer delay of at most two ticks.

This is still correct from the standpoint of the test, which verifies
that no correlation exists between the clock offset injected by
clock_settime() and the delay until the next tick generated by the
affected clock.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 testsuite/smokey/posix-clock/posix-clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testsuite/smokey/posix-clock/posix-clock.c b/testsuite/smokey/posix-clock/posix-clock.c
index f672a9d52..3a638d41f 100644
--- a/testsuite/smokey/posix-clock/posix-clock.c
+++ b/testsuite/smokey/posix-clock/posix-clock.c
@@ -417,7 +417,7 @@ static int clock_decrease_after_periodic_timer_first_tick(void)
 
 	diff = now.tv_sec * 1000000000ULL + now.tv_nsec -
 		(timer.it_value.tv_sec * 1000000000ULL + timer.it_value.tv_nsec);
-	if (!smokey_assert(diff < 1000000000))
+	if (!smokey_assert(diff < 2000000000))
 		return -EINVAL;
 	
 	ret = smokey_check_errno(read(t, &ticks, sizeof(ticks)));
-- 
2.17.2

Re: [PATCH 1/5] cobalt/intr: vfile: do not assume all irqs are cobalt-managed

[Jan Kiszka]

On 14.01.19 18:35, Philippe Gerum wrote:
> Non-cobalt kernel code may hook interrupts using ipipe_request_irq()
> directly, which means that xnintr_vec_first() cannot assume that
> __ipipe_irq_cookie() always returns a valid xnintr struct for all
> irqs.

Is there an out-of-tree use case for that already, or is this rather defensive?

> 
> We need to detect those irqs while iterating over the interrupt
> namespace when pulling data from /proc files not to dereference
> invalid memory.
> 
> Fixes:
> /proc/xenomai/irq
> /proc/xenomai/sched/stat
> /proc/xenomai/sched/acct
> 
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
>   kernel/cobalt/intr.c | 48 +++++++++++++++++++++++++++++++++++---------
>   1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/cobalt/intr.c b/kernel/cobalt/intr.c
> index 184ffad13..ba6409ebb 100644
> --- a/kernel/cobalt/intr.c
> +++ b/kernel/cobalt/intr.c
> @@ -449,6 +449,17 @@ out:
>   	}
>   }
>   
> +static inline bool cobalt_owns_irq(int irq)
> +{
> +	ipipe_irq_handler_t h;
> +
> +	h = __ipipe_irq_handler(&xnsched_realtime_domain, irq);
> +
> +	return h == xnintr_vec_handler ||
> +		h == xnintr_edge_vec_handler ||
> +		h == xnintr_irq_handler;
> +}
> +
>   static inline int xnintr_irq_attach(struct xnintr *intr)
>   {
>   	struct xnintr_vector *vec = vectors + intr->irq;
> @@ -538,9 +549,19 @@ struct xnintr_vector {
>   
>   static struct xnintr_vector vectors[IPIPE_NR_IRQS];
>   
> +static inline bool cobalt_owns_irq(int irq)
> +{
> +	ipipe_irq_handler_t h;
> +
> +	h = __ipipe_irq_handler(&xnsched_realtime_domain, irq);
> +
> +	return h == xnintr_irq_handler;
> +}
> +
>   static inline struct xnintr *xnintr_vec_first(unsigned int irq)
>   {
> -	return __ipipe_irq_cookie(&xnsched_realtime_domain, irq);
> +	return cobalt_owns_irq(irq) ?
> +		__ipipe_irq_cookie(&xnsched_realtime_domain, irq) : NULL;
>   }
>   
>   static inline struct xnintr *xnintr_vec_next(struct xnintr *prev)
> @@ -1067,6 +1088,7 @@ static inline int format_irq_proc(unsigned int irq,
>   				  struct xnvfile_regular_iterator *it)
>   {
>   	struct xnintr *intr;
> +	struct irq_desc *d;
>   	int cpu;
>   
>   	for_each_realtime_cpu(cpu)
> @@ -1100,15 +1122,21 @@ static inline int format_irq_proc(unsigned int irq,
>   
>   	mutex_lock(&intrlock);
>   
> -	intr = xnintr_vec_first(irq);
> -	if (intr) {
> -		xnvfile_puts(it, "        ");
> -
> -		do {
> -			xnvfile_putc(it, ' ');
> -			xnvfile_puts(it, intr->name);
> -			intr = xnintr_vec_next(intr);
> -		} while (intr);
> +	if (!cobalt_owns_irq(irq)) {
> +		xnvfile_puts(it, "         ");
> +		d = irq_to_desc(irq);
> +		xnvfile_puts(it, d && d->name ? d->name : "-");
> +	} else {
> +		intr = xnintr_vec_first(irq);
> +		if (intr) {
> +			xnvfile_puts(it, "        ");
> +
> +			do {
> +				xnvfile_putc(it, ' ');
> +				xnvfile_puts(it, intr->name);
> +				intr = xnintr_vec_next(intr);
> +			} while (intr);
> +		}
>   	}
>   
>   	mutex_unlock(&intrlock);
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 5/5] testsuite/smokey: posix_clock: prevent false positive in timing-depending test

[Jan Kiszka]

On 14.01.19 18:35, Philippe Gerum wrote:
> clock_decrease_after_periodic_timer_first_tick checks that periodic
> interval timers based on CLOCK_REALTIME are not (pathologically)
> affected by the epoch going backwards.
> 
> To this end, we measure the actual time observed between two ticks of
> a periodic timer based on CLOCK_REALTIME with a call to
> clock_settime() injecting a negative offset in between, equivalent to
> five ticks.
> 
> Due to processing delays induced by clock_settime() and other latency,
> we could observe a duration which exceeds a tick by a few tenths of
> microseconds. Since we can't anticipate the amount of latency
> involved, let's accept a longer delay of at most two ticks.
> 
> This is still correct from the standpoint of the test, which verifies
> that no correlation exists between the clock offset injected by
> clock_settime() and the delay until the next tick generated by the
> affected clock.
> 
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
>   testsuite/smokey/posix-clock/posix-clock.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/testsuite/smokey/posix-clock/posix-clock.c b/testsuite/smokey/posix-clock/posix-clock.c
> index f672a9d52..3a638d41f 100644
> --- a/testsuite/smokey/posix-clock/posix-clock.c
> +++ b/testsuite/smokey/posix-clock/posix-clock.c
> @@ -417,7 +417,7 @@ static int clock_decrease_after_periodic_timer_first_tick(void)
>   
>   	diff = now.tv_sec * 1000000000ULL + now.tv_nsec -
>   		(timer.it_value.tv_sec * 1000000000ULL + timer.it_value.tv_nsec);
> -	if (!smokey_assert(diff < 1000000000))
> +	if (!smokey_assert(diff < 2000000000))
>   		return -EINVAL;
>   	
>   	ret = smokey_check_errno(read(t, &ticks, sizeof(ticks)));
> 

OK, this apparently addresses the issue Henning once brought up.

You sent the patch again outside of the queue but both look identical. Will take 
this one.

Finally: All 5 a also stable material as it looks like, right?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 1/5] cobalt/intr: vfile: do not assume all irqs are cobalt-managed

[Philippe Gerum]

On 1/15/19 6:12 PM, Jan Kiszka wrote:
> On 14.01.19 18:35, Philippe Gerum wrote:
>> Non-cobalt kernel code may hook interrupts using ipipe_request_irq()
>> directly, which means that xnintr_vec_first() cannot assume that
>> __ipipe_irq_cookie() always returns a valid xnintr struct for all
>> irqs.
> 
> Is there an out-of-tree use case for that already, or is this rather
> defensive?
> 

I stumbled upon this issue when hooking a device IRQ inside a regular
driver using the pipeline interface, as using xnintr* and friends at
such interface level would be a bad idea. Sometimes we may want to have
pipeline-aware drivers which are not and should not be RTDM-based. We
can't assume what 3rd party drivers are doing or not in this respect.

Once such driver is enabled, reading /proc/xenomai/irq for instance
would start the fireworks.

-- 
Philippe.

Re: [PATCH 5/5] testsuite/smokey: posix_clock: prevent false positive in timing-depending test

[Philippe Gerum]

On 1/15/19 6:14 PM, Jan Kiszka wrote:
> On 14.01.19 18:35, Philippe Gerum wrote:
>> clock_decrease_after_periodic_timer_first_tick checks that periodic
>> interval timers based on CLOCK_REALTIME are not (pathologically)
>> affected by the epoch going backwards.
>>
>> To this end, we measure the actual time observed between two ticks of
>> a periodic timer based on CLOCK_REALTIME with a call to
>> clock_settime() injecting a negative offset in between, equivalent to
>> five ticks.
>>
>> Due to processing delays induced by clock_settime() and other latency,
>> we could observe a duration which exceeds a tick by a few tenths of
>> microseconds. Since we can't anticipate the amount of latency
>> involved, let's accept a longer delay of at most two ticks.
>>
>> This is still correct from the standpoint of the test, which verifies
>> that no correlation exists between the clock offset injected by
>> clock_settime() and the delay until the next tick generated by the
>> affected clock.
>>
>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>> ---
>>   testsuite/smokey/posix-clock/posix-clock.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/testsuite/smokey/posix-clock/posix-clock.c
>> b/testsuite/smokey/posix-clock/posix-clock.c
>> index f672a9d52..3a638d41f 100644
>> --- a/testsuite/smokey/posix-clock/posix-clock.c
>> +++ b/testsuite/smokey/posix-clock/posix-clock.c
>> @@ -417,7 +417,7 @@ static int
>> clock_decrease_after_periodic_timer_first_tick(void)
>>         diff = now.tv_sec * 1000000000ULL + now.tv_nsec -
>>           (timer.it_value.tv_sec * 1000000000ULL +
>> timer.it_value.tv_nsec);
>> -    if (!smokey_assert(diff < 1000000000))
>> +    if (!smokey_assert(diff < 2000000000))
>>           return -EINVAL;
>>      
>>       ret = smokey_check_errno(read(t, &ticks, sizeof(ticks)));
>>
> 
> OK, this apparently addresses the issue Henning once brought up.
> 
> You sent the patch again outside of the queue but both look identical.
> Will take this one.
>

The second one fixes the shortlog.

> Finally: All 5 a also stable material as it looks like, right?
> 

I think so.

-- 
Philippe.

Re: [PATCH 5/5] testsuite/smokey: posix_clock: prevent false positive in timing-depending test

[Jan Kiszka]

On 15.01.19 19:06, Philippe Gerum wrote:
> On 1/15/19 6:14 PM, Jan Kiszka wrote:
>> On 14.01.19 18:35, Philippe Gerum wrote:
>>> clock_decrease_after_periodic_timer_first_tick checks that periodic
>>> interval timers based on CLOCK_REALTIME are not (pathologically)
>>> affected by the epoch going backwards.
>>>
>>> To this end, we measure the actual time observed between two ticks of
>>> a periodic timer based on CLOCK_REALTIME with a call to
>>> clock_settime() injecting a negative offset in between, equivalent to
>>> five ticks.
>>>
>>> Due to processing delays induced by clock_settime() and other latency,
>>> we could observe a duration which exceeds a tick by a few tenths of
>>> microseconds. Since we can't anticipate the amount of latency
>>> involved, let's accept a longer delay of at most two ticks.
>>>
>>> This is still correct from the standpoint of the test, which verifies
>>> that no correlation exists between the clock offset injected by
>>> clock_settime() and the delay until the next tick generated by the
>>> affected clock.
>>>
>>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>>> ---
>>>    testsuite/smokey/posix-clock/posix-clock.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/testsuite/smokey/posix-clock/posix-clock.c
>>> b/testsuite/smokey/posix-clock/posix-clock.c
>>> index f672a9d52..3a638d41f 100644
>>> --- a/testsuite/smokey/posix-clock/posix-clock.c
>>> +++ b/testsuite/smokey/posix-clock/posix-clock.c
>>> @@ -417,7 +417,7 @@ static int
>>> clock_decrease_after_periodic_timer_first_tick(void)
>>>          diff = now.tv_sec * 1000000000ULL + now.tv_nsec -
>>>            (timer.it_value.tv_sec * 1000000000ULL +
>>> timer.it_value.tv_nsec);
>>> -    if (!smokey_assert(diff < 1000000000))
>>> +    if (!smokey_assert(diff < 2000000000))
>>>            return -EINVAL;
>>>       
>>>        ret = smokey_check_errno(read(t, &ticks, sizeof(ticks)));
>>>
>>
>> OK, this apparently addresses the issue Henning once brought up.
>>
>> You sent the patch again outside of the queue but both look identical.
>> Will take this one.
>>
> 
> The second one fixes the shortlog.
> 

Again, both patches are 100% identical. If they shouldn't, please check and resend.

>> Finally: All 5 a also stable material as it looks like, right?
>>
> 
> I think so.
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 4/5] drivers/ipc: bufp: fix read-write, write-write preemption cases

[Jan Kiszka]

On 14.01.19 18:35, Philippe Gerum wrote:
> The token-based approach for detecting preemption while data is being
> moved into or out of the ring only protects from read vs read races,
> not from races involving a write side. For instance, a reader might
> read dirty data being changed by a writer concurrently, or two writers
> might compete writing two distinct messages at the same place in the
> ring space.
> 
> To address this issue, use a slot-based implementation which
> atomically reserves exclusive portions of the ring space readers and
> writers will be accessing locklessly. Those slots are guaranteed to
> never overlap among read and write requests, until the lockless
> operation finishes.
> 
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
>   kernel/drivers/ipc/bufp.c | 118 ++++++++++++++++----------------------
>   1 file changed, 50 insertions(+), 68 deletions(-)
> 
> diff --git a/kernel/drivers/ipc/bufp.c b/kernel/drivers/ipc/bufp.c
> index e1c867288..4fa6593c3 100644
> --- a/kernel/drivers/ipc/bufp.c
> +++ b/kernel/drivers/ipc/bufp.c
> @@ -42,10 +42,10 @@ struct bufp_socket {
>   	char label[XNOBJECT_NAME_LEN];
>   
>   	off_t rdoff;
> +	off_t rdrsvd;
>   	off_t wroff;
> +	off_t wrrsvd;
>   	size_t fillsz;
> -	u_long wrtoken;
> -	u_long rdtoken;
>   	rtdm_event_t i_event;
>   	rtdm_event_t o_event;
>   
> @@ -115,8 +115,8 @@ static int bufp_socket(struct rtdm_fd *fd)
>   	sk->rdoff = 0;
>   	sk->wroff = 0;
>   	sk->fillsz = 0;
> -	sk->rdtoken = 0;
> -	sk->wrtoken = 0;
> +	sk->rdrsvd = 0;
> +	sk->wrrsvd = 0;
>   	sk->status = 0;
>   	sk->handle = 0;
>   	sk->rx_timeout = RTDM_TIMEOUT_INFINITE;
> @@ -162,11 +162,10 @@ static ssize_t __bufp_readbuf(struct bufp_socket *sk,
>   	struct bufp_wait_context wait, *bufwc;
>   	struct rtipc_wait_context *wc;
>   	struct xnthread *waiter;
> +	size_t rbytes, n, avail;
> +	ssize_t len, ret, xret;
>   	rtdm_toseq_t toseq;
> -	ssize_t len, ret;
> -	size_t rbytes, n;
>   	rtdm_lockctx_t s;
> -	u_long rdtoken;
>   	off_t rdoff;
>   	int resched;
>   
> @@ -181,18 +180,15 @@ redo:
>   		 * We should be able to read a complete message of the
>   		 * requested length, or block.
>   		 */
> -		if (sk->fillsz < len)
> +		avail = sk->fillsz - sk->rdrsvd;
> +		if (avail < len)
>   			goto wait;
>   
> -		/*
> -		 * Draw the next read token so that we can later
> -		 * detect preemption.
> -		 */
> -		rdtoken = ++sk->rdtoken;
> -
> -		/* Read from the buffer in a circular way. */
> +		/* Reserve a read slot into the circular buffer. */
>   		rdoff = sk->rdoff;
> -		rbytes = len;
> +		sk->rdoff = (rdoff + len) % sk->bufsz;
> +		sk->rdrsvd += len;
> +		rbytes = ret = len;
>   
>   		do {
>   			if (rdoff + rbytes > sk->bufsz)
> @@ -200,37 +196,30 @@ redo:
>   			else
>   				n = rbytes;
>   			/*
> -			 * Release the lock while retrieving the data
> -			 * to keep latency low.
> +			 * Drop the lock before copying data to
> +			 * user. The read slot is consumed in any
> +			 * case: the non-copied portion of the message
> +			 * is lost on bad write.
>   			 */
>   			cobalt_atomic_leave(s);
> -			ret = xnbufd_copy_from_kmem(bufd, sk->bufmem + rdoff, n);
> -			if (ret < 0)
> -				return ret;
> -
> +			xret = xnbufd_copy_from_kmem(bufd, sk->bufmem + rdoff, n);
>   			cobalt_atomic_enter(s);
> -			/*
> -			 * In case we were preempted while retrieving
> -			 * the message, we have to re-read the whole
> -			 * thing.
> -			 */
> -			if (sk->rdtoken != rdtoken) {
> -				xnbufd_reset(bufd);
> -				goto redo;
> +			if (xret < 0) {
> +				ret = -EFAULT;
> +				break;
>   			}
>   
> -			rdoff = (rdoff + n) % sk->bufsz;
>   			rbytes -= n;
> +			rdoff = (rdoff + n) % sk->bufsz;
>   		} while (rbytes > 0);
>   
> -		sk->fillsz -= len;
> -		sk->rdoff = rdoff;
> -		ret = len;
> -
>   		resched = 0;
> -		if (sk->fillsz + len == sk->bufsz) /* -> writable */
> +		if (sk->fillsz == sk->bufsz) /* -> writable */
>   			resched |= xnselect_signal(&sk->priv->send_block, POLLOUT);
>   
> +		sk->rdrsvd -= len;
> +		sk->fillsz -= len;
> +
>   		if (sk->fillsz == 0) /* -> non-readable */
>   			resched |= xnselect_signal(&sk->priv->recv_block, 0);
>   
> @@ -416,11 +405,10 @@ static ssize_t __bufp_writebuf(struct bufp_socket *rsk,
>   	struct bufp_wait_context wait, *bufwc;
>   	struct rtipc_wait_context *wc;
>   	struct xnthread *waiter;
> +	size_t wbytes, n, avail;
> +	ssize_t len, ret, xret;
>   	rtdm_toseq_t toseq;
>   	rtdm_lockctx_t s;
> -	ssize_t len, ret;
> -	size_t wbytes, n;
> -	u_long wrtoken;
>   	off_t wroff;
>   	int resched;
>   
> @@ -429,24 +417,21 @@ static ssize_t __bufp_writebuf(struct bufp_socket *rsk,
>   	rtdm_toseq_init(&toseq, sk->tx_timeout);
>   
>   	cobalt_atomic_enter(s);
> -redo:
> +
>   	for (;;) {
>   		/*
> -		 * We should be able to write the entire message at
> -		 * once or block.
> +		 * No short or scattered writes: we should write the
> +		 * entire message atomically or block.
>   		 */
> -		if (rsk->fillsz + len > rsk->bufsz)
> +		avail = rsk->fillsz + rsk->wrrsvd;
> +		if (avail + len > rsk->bufsz)
>   			goto wait;
>   
> -		/*
> -		 * Draw the next write token so that we can later
> -		 * detect preemption.
> -		 */
> -		wrtoken = ++rsk->wrtoken;
> -
> -		/* Write to the buffer in a circular way. */
> +		/* Reserve a write slot into the circular buffer. */
>   		wroff = rsk->wroff;
> -		wbytes = len;
> +		rsk->wroff = (wroff + len) % rsk->bufsz;
> +		rsk->wrrsvd += len;
> +		wbytes = ret = len;
>   
>   		do {
>   			if (wroff + wbytes > rsk->bufsz)
> @@ -454,33 +439,30 @@ redo:
>   			else
>   				n = wbytes;
>   			/*
> -			 * Release the lock while copying the data to
> -			 * keep latency low.
> +			 * We have to drop the lock while reading in
> +			 * data, but we can't rollback on bad read
> +			 * from user because some other thread might
> +			 * have populated the memory ahead of our
> +			 * write slot already: bluntly clear the
> +			 * unavailable bytes on copy error.
>   			 */
>   			cobalt_atomic_leave(s);
> -			ret = xnbufd_copy_to_kmem(rsk->bufmem + wroff, bufd, n);
> -			if (ret < 0)
> -				return ret;
> +			xret = xnbufd_copy_to_kmem(rsk->bufmem + wroff, bufd, n);
>   			cobalt_atomic_enter(s);
> -			/*
> -			 * In case we were preempted while copying the
> -			 * message, we have to write the whole thing
> -			 * again.
> -			 */
> -			if (rsk->wrtoken != wrtoken) {
> -				xnbufd_reset(bufd);
> -				goto redo;
> +			if (xret < 0) {
> +				memset(rsk->bufmem + wroff + n - xret, 0, xret);
> +				ret = -EFAULT;
> +				break;
>   			}
>   
> -			wroff = (wroff + n) % rsk->bufsz;
>   			wbytes -= n;
> +			wroff = (wroff + n) % rsk->bufsz;
>   		} while (wbytes > 0);
>   
>   		rsk->fillsz += len;
> -		rsk->wroff = wroff;
> -		ret = len;
> -		resched = 0;
> +		rsk->wrrsvd -= len;
>   
> +		resched = 0;
>   		if (rsk->fillsz == len) /* -> readable */
>   			resched |= xnselect_signal(&rsk->priv->recv_block, POLLIN);
>   
> 

Applied up to this patch to next, still waiting for The Final Version of patch 5.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 4/5] drivers/ipc: bufp: fix read-write, write-write preemption cases

[Philippe Gerum]

On 1/24/19 7:28 PM, Jan Kiszka wrote:

> 
> Applied up to this patch to next, still waiting for The Final Version of
> patch 5.
> 

AFAIU, patch 5 would be [1], which should be [2] in its final state.

[1] https://www.xenomai.org/pipermail/xenomai/2019-January/040215.html
[2] https://www.xenomai.org/pipermail/xenomai/2019-January/040238.html

or are you referring to another patch?

-- 
Philippe.