[PATCH 1/2] printk: ringbuffer: fix setting state in desc_read()

[PATCH 1/2] printk: ringbuffer: fix setting state in desc_read()

[John Ogness]

It is expected that desc_read() will always set at least the
@state_var field. However, if the descriptor is in an inconsistent
state, no fields are set.

Also, the second load of @state_var is not stored in @desc_out and
so might not match the state value that is returned.

Always set the last loaded @state_var into @desc_out, regardless of
the descriptor consistency.

Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk_ringbuffer.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 0659b50872b5..88f7dd4cb0c1 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -368,9 +368,9 @@ static enum desc_state get_desc_state(unsigned long id,
 }
 
 /*
- * Get a copy of a specified descriptor and its queried state. A descriptor
- * that is not in the committed or reusable state must be considered garbage
- * by the reader.
+ * Get a copy of a specified descriptor and return its queried state. If the
+ * descriptor is in an inconsistent state (miss or reserved), the caller can
+ * only expect the descriptor's @state_var field to be valid.
  */
 static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
 				 unsigned long id, struct prb_desc *desc_out)
@@ -383,8 +383,14 @@ static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
 	/* Check the descriptor state. */
 	state_val = atomic_long_read(state_var); /* LMM(desc_read:A) */
 	d_state = get_desc_state(id, state_val);
-	if (d_state != desc_committed && d_state != desc_reusable)
-		return d_state;
+	if (d_state == desc_miss || d_state == desc_reserved) {
+		/*
+		 * The descriptor is in an inconsistent state. Set at least
+		 * @state_var so that the caller can see the details of
+		 * the inconsistent state.
+		 */
+		goto out;
+	}
 
 	/*
 	 * Guarantee the state is loaded before copying the descriptor
@@ -449,9 +455,15 @@ static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
 	 */
 	smp_rmb(); /* LMM(desc_read:D) */
 
-	/* Re-check the descriptor state. */
+	/*
+	 * The data has been copied. Return the current descriptor state,
+	 * which may have changed since the load above.
+	 */
 	state_val = atomic_long_read(state_var); /* LMM(desc_read:E) */
-	return get_desc_state(id, state_val);
+	d_state = get_desc_state(id, state_val);
+out:
+	atomic_long_set(&desc_out->state_var, state_val);
+	return d_state;
 }
 
 /*
-- 
2.19.0

[PATCH 2/2] printk: ringbuffer: avoid memcpy() on state_var

[John Ogness]

@state_var is copied as part of the descriptor copying via
memcpy(). This is not allowed because @state_var is an atomic type,
which in some implementations may contain a spinlock.

Avoid using memcpy() with @state_var by explicitly copying the other
fields of the descriptor. @state_var is set using atomic set
operator before returning.

Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk_ringbuffer.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 88f7dd4cb0c1..11b860ad5264 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -412,9 +412,14 @@ static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
 
 	/*
 	 * Copy the descriptor data. The data is not valid until the
-	 * state has been re-checked.
+	 * state has been re-checked. A memcpy() for all of @desc
+	 * cannot be used because of the atomic_t @state_var field.
 	 */
-	memcpy(desc_out, desc, sizeof(*desc_out)); /* LMM(desc_read:C) */
+	memcpy(&desc_out->info, &desc->info, sizeof(desc_out->info)); /* LMM(desc_read:C) */
+	memcpy(&desc_out->text_blk_lpos, &desc->text_blk_lpos,
+	       sizeof(desc_out->text_blk_lpos)); /* also part of desc_read:C */
+	memcpy(&desc_out->dict_blk_lpos, &desc->dict_blk_lpos,
+	       sizeof(desc_out->dict_blk_lpos)); /* also part of desc_read:C */
 
 	/*
 	 * 1. Guarantee the descriptor content is loaded before re-checking
-- 
2.19.0

Re: [PATCH 1/2] printk: ringbuffer: fix setting state in desc_read()

[Petr Mladek]

On Mon 2020-09-14 11:54:02, John Ogness wrote:
> It is expected that desc_read() will always set at least the
> @state_var field. However, if the descriptor is in an inconsistent
> state, no fields are set.
> 
> Also, the second load of @state_var is not stored in @desc_out and
> so might not match the state value that is returned.
> 
> Always set the last loaded @state_var into @desc_out, regardless of
> the descriptor consistency.
> 
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 2/2] printk: ringbuffer: avoid memcpy() on state_var

[Petr Mladek]

On Mon 2020-09-14 11:54:03, John Ogness wrote:
> @state_var is copied as part of the descriptor copying via
> memcpy(). This is not allowed because @state_var is an atomic type,
> which in some implementations may contain a spinlock.

Great catch!

> Avoid using memcpy() with @state_var by explicitly copying the other
> fields of the descriptor. @state_var is set using atomic set
> operator before returning.

Just thinking loudly.

Strictly speaking, memcpy() might be used when the atomic variable
is later corrected by an atomic operation. And it is done by the first
patch.

I think that it is a matter of taste what solution is more error
prone. Either this function must be updated when a new field
is added into struct prb_desc or the atomic_long_set() must not
get removed.

Hmm, missing memcpy() should be rather easy to debug. While missing
atomic_long_set() is really subtle problem problem. So, the new
code is better from my POV.


> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 1/2] printk: ringbuffer: fix setting state in desc_read()

[Petr Mladek]

On Mon 2020-09-14 11:54:02, John Ogness wrote:
> It is expected that desc_read() will always set at least the
> @state_var field. However, if the descriptor is in an inconsistent
> state, no fields are set.
> 
> Also, the second load of @state_var is not stored in @desc_out and
> so might not match the state value that is returned.
> 
> Always set the last loaded @state_var into @desc_out, regardless of
> the descriptor consistency.
> 
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Both patches are committed into printk/linux.git, branch printk-rework.

Best Regards,
Petr