Re: [RFC PATCH v4 4/9] printk-rb: initialize new descriptors as invalid

[John Ogness]

On 2019-08-20, Petr Mladek  wrote:
>> Initialize never-used descriptors as permanently invalid so there
>
> The word "permanently" is confusing. It suggests that it will
> never ever be valid again. I would just remove the word.

Agreed.

>> is no risk of the descriptor unexpectedly being determined as
>> valid due to dataring head overflowing/wrapping.
>
> Please, provide more details about the solved race.

OK.

> Is it because some reader could have reference to an invalid
> (reused) descriptor?

Yes, but not because it is reused. If a writer succeeded in reserving a
descriptor, but failed to reserve a datablock, that (invalid) descriptor
is put on the committed list (see fA). By setting the lpos values to
something that could _never_ be valid, there is no risk of the
descriptor suddenly becoming valid due to head overflowing.

My RFCv2 did not account for this and instead invalid descriptors just
held on to whatever lpos values they last had. Although they are invalid
at that moment, if not set to something "permanently" invalid, those
values could become valid again. We talked about that here[0].

> Can be these invalid descriptors be member of the list?

Yes (as Sergey shows in his followup post). Readers see them as invalid
and treat them as dropped records.

> Also it might be worth to mention where is the check that might
> detect such invalid descriptors and what will be the consequences.
> Well, this might be clear from the race description.

The check itself is not special. However, readers do have to be aware of
and correctly handle the case of invalid descriptors on the list. I will
find an appropriate place to document this.

John Ogness

[0] https://lkml.kernel.org/r/20190624140948.l7ekcmz5ser3z...@pathway.suse.cz

Re: [RFC PATCH v4 4/9] printk-rb: initialize new descriptors as invalid

[Sergey Senozhatsky]

On (08/20/19 11:23), Petr Mladek wrote:
> > is no risk of the descriptor unexpectedly being determined as
> > valid due to dataring head overflowing/wrapping.
> 
> Please, provide more details about the solved race. Is it because
> some reader could have reference to an invalid (reused) descriptor?
> Can be these invalid descriptors be member of the list?

As far as I understand, such descriptors can be on the list:

prb_reserve()
assign_desc()
// pick a new never used descr
i = atomic_fetch_inc(>desc_next_unused);
d = >descs[i]
dataring_desc_init(>desc);
return d
buf = dataring_push()
// the oldest data is reserved, but not commited
ret = get_new_lpos()
if (ret)
dataring_desc_init()
return NULL
if (!buf)
numlist_push()

_datablock_valid() has a "desc->begin_lpos == desc->next_lpos" check.

-ss

Re: [RFC PATCH v4 4/9] printk-rb: initialize new descriptors as invalid

[Petr Mladek]

On Thu 2019-08-08 00:32:29, John Ogness wrote:
> Initialize never-used descriptors as permanently invalid so there

The word "permanently" is confusing. It suggests that it will
never ever be valid again. I would just remove the word.

> is no risk of the descriptor unexpectedly being determined as
> valid due to dataring head overflowing/wrapping.

Please, provide more details about the solved race. Is it because
some reader could have reference to an invalid (reused) descriptor?
Can be these invalid descriptors be member of the list?

Also it might be worth to mention where is the check that might
detect such invalid descriptors and what will be the consequences.
Well, this might be clear from the race description.

Best Regards,
Petr

[RFC PATCH v4 4/9] printk-rb: initialize new descriptors as invalid

[John Ogness]

Initialize never-used descriptors as permanently invalid so there
is no risk of the descriptor unexpectedly being determined as
valid due to dataring head overflowing/wrapping.

Signed-off-by: John Ogness 
---
 kernel/printk/dataring.c   | 42 +++---
 kernel/printk/dataring.h   | 12 +++
 kernel/printk/ringbuffer.c |  5 +
 kernel/printk/ringbuffer.h |  7 ++-
 4 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/kernel/printk/dataring.c b/kernel/printk/dataring.c
index 6642e085a05d..345c46dba5bb 100644
--- a/kernel/printk/dataring.c
+++ b/kernel/printk/dataring.c
@@ -316,8 +316,8 @@ static unsigned long _dataring_pop(struct dataring *dr,
 * If dB reads from gA, then dD reads from fH.
 * If dB reads from gA, then dE reads from fE.
 *
-* Note that if dB reads from gA, then dC cannot read from fC.
-* Note that if dB reads from gA, then dD cannot read from fD.
+* Note that if dB reads from gA, then dC cannot read from fC->nA.
+* Note that if dB reads from gA, then dD cannot read from fC->nB.
 *
 * Relies on:
 *
@@ -489,6 +489,30 @@ static bool get_new_lpos(struct dataring *dr, unsigned int 
size,
}
 }
 
+/**
+ * dataring_desc_init() - Initialize a descriptor to be permanently invalid.
+ *
+ * @desc: The descriptor to initialize.
+ *
+ * Setting a descriptor to be permanently invalid means that there is no risk
+ * of the descriptor later unexpectedly being determined as valid due to
+ * overflowing/wrapping of @head_lpos.
+ */
+void dataring_desc_init(struct dr_desc *desc)
+{
+   /*
+* An unaligned @begin_lpos can never point to a data block and
+* having the same value for @begin_lpos and @next_lpos is also
+* invalid.
+*/
+
+   /* nA: */
+   WRITE_ONCE(desc->begin_lpos, 1);
+
+   /* nB: */
+   WRITE_ONCE(desc->next_lpos, 1);
+}
+
 /**
  * dataring_push() - Reserve a data block in the data array.
  *
@@ -568,20 +592,14 @@ char *dataring_push(struct dataring *dr, unsigned int 
size,
 
if (!ret) {
/*
+* fC:
+*
 * Force @desc permanently invalid to minimize risk
 * of the descriptor later unexpectedly being
 * determined as valid due to overflowing/wrapping of
-* @head_lpos. An unaligned @begin_lpos can never
-* point to a data block and having the same value
-* for @begin_lpos and @next_lpos is also invalid.
+* @head_lpos.
 */
-
-   /* fC: */
-   WRITE_ONCE(desc->begin_lpos, 1);
-
-   /* fD: */
-   WRITE_ONCE(desc->next_lpos, 1);
-
+   dataring_desc_init(desc);
return NULL;
}
/* fE: */
diff --git a/kernel/printk/dataring.h b/kernel/printk/dataring.h
index 346a455a335a..413ee95f4dd6 100644
--- a/kernel/printk/dataring.h
+++ b/kernel/printk/dataring.h
@@ -43,6 +43,17 @@ struct dr_desc {
unsigned long   next_lpos;
 };
 
+/*
+ * Initialize a descriptor to be permanently invalid so there is no risk
+ * of the descriptor later unexpectedly being determined as valid due to
+ * overflowing/wrapping of @head_lpos.
+ */
+#define __DR_DESC_INITIALIZER  \
+   {   \
+   .begin_lpos = 1,\
+   .next_lpos  = 1,\
+   }
+
 /**
  * struct dataring - A data ringbuffer with support for entry IDs.
  *
@@ -90,6 +101,7 @@ void dataring_datablock_setid(struct dataring *dr, struct 
dr_desc *desc,
 struct dr_datablock *dataring_getdatablock(struct dataring *dr,
   struct dr_desc *desc, int *size);
 bool dataring_datablock_isvalid(struct dataring *dr, struct dr_desc *desc);
+void dataring_desc_init(struct dr_desc *desc);
 void dataring_desc_copy(struct dr_desc *dst, struct dr_desc *src);
 
 #endif /* _KERNEL_PRINTK_DATARING_H */
diff --git a/kernel/printk/ringbuffer.c b/kernel/printk/ringbuffer.c
index b9fc13597b10..9be841761ea2 100644
--- a/kernel/printk/ringbuffer.c
+++ b/kernel/printk/ringbuffer.c
@@ -345,6 +345,11 @@ static bool assign_desc(struct prb_reserved_entry *e)
if (i < DESCS_COUNT(rb)) {
d = >descs[i];
atomic_long_set(>id, i);
+   /*
+* Initialize the new descriptor such that
+* it is permanently invalid.
+*/
+   dataring_desc_init(>desc);
break;
}
}
diff --git