From: Yulei Zhang <[email protected]>
Early this year we spot there may be two issues in kernel
kfifo.
One is reported by Xiao Guangrong to linux kernel.
https://lkml.org/lkml/2018/5/11/58
In current kfifo implementation there are missing memory
barrier in the read side, so that without proper barrier
between reading the kfifo->in and fetching the data there
is potential ordering issue.
Beside that, there is another potential issue in kfifo,
please consider the following case:
at the beginning
ring->size = 4
ring->out = 0
ring->in = 4
Consumer Producer
--------------- --------------
index = ring->out; /* index == 0 */
ring->out++; /* ring->out == 1 */
< Re-Order >
out = ring->out;
if (ring->in - out >= ring->mask)
return -EFULL;
/* see the ring is not full */
index = ring->in & ring->mask;
/* index == 0 */
ring->data[index] = new_data;
ring->in++;
data = ring->data[index];
/* you will find the old data is overwritten by the new_data */
In order to avoid the issue:
1) for the consumer, we should read the ring->data[] out before
updating ring->out
2) for the producer, we should read ring->out before updating
ring->data[]
So in this patch we introduce the following four functions which
are wrapped with proper memory barrier and keep in pairs to make
sure the in and out index are fetched and updated in order to avoid
data loss.
kfifo_read_index_in()
kfifo_write_index_in()
kfifo_read_index_out()
kfifo_write_index_out()
Signed-off-by: Yulei Zhang <[email protected]>
Signed-off-by: Guangrong Xiao <[email protected]>
---
include/linux/kfifo.h | 70 ++++++++++++++++++++++++++-
lib/kfifo.c | 107 +++++++++++++++++++++++++++---------------
2 files changed, 136 insertions(+), 41 deletions(-)
diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 89fc8dc7bf38..3bd2a869ca7e 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -286,6 +286,71 @@ __kfifo_uint_must_check_helper( \
}) \
)
+/**
+ * kfifo_read_index_in - returns the in index of the fifo
+ * @fifo: address of the kfifo to be used
+ *
+ * add memory read barrier to make sure the fifo->in index
+ * is fetched first before write data to the fifo, which
+ * is paired with the write barrier in kfifo_write_index_in
+ */
+#define kfifo_read_index_in(kfifo) \
+({ \
+ typeof((kfifo) + 1) __tmp = (kfifo); \
+ struct __kfifo *__kfifo = __tmp; \
+ unsigned int __val = READ_ONCE(__kfifo->in); \
+ smp_rmb(); \
+ __val; \
+})
+
+/**
+ * kfifo_write_index_in - updates the in index of the fifo
+ * @fifo: address of the kfifo to be used
+ *
+ * add memory write barrier to make sure the data entry is
+ * updated before increase the fifo->in
+ */
+#define kfifo_write_index_in(kfifo, val) \
+({ \
+ typeof((kfifo) + 1) __tmp = (kfifo); \
+ struct __kfifo *__kfifo = __tmp; \
+ unsigned int __val = (val); \
+ smp_wmb(); \
+ WRITE_ONCE(__kfifo->in, __val); \
+})
+
+/**
+ * kfifo_read_index_out - returns the out index of the fifo
+ * @fifo: address of the kfifo to be used
+ *
+ * add memory barrier to make sure the fifo->out index is
+ * fetched before read data from the fifo, which is paired
+ * with the memory barrier in kfifo_write_index_out
+ */
+#define kfifo_read_index_out(kfifo) \
+({ \
+ typeof((kfifo) + 1) __tmp = (kfifo); \
+ struct __kfifo *__kfifo = __tmp; \
+ unsigned int __val = smp_load_acquire(&__kfifo->out); \
+ __val; \
+})
+
+/**
+ * kfifo_write_index_out - updates the out index of the fifo
+ * @fifo: address of the kfifo to be used
+ *
+ * add memory barrier to make sure reading out the entry before
+ * update the fifo->out index to avoid overwitten the entry by
+ * the producer
+ */
+#define kfifo_write_index_out(kfifo, val) \
+({ \
+ typeof((kfifo) + 1) __tmp = (kfifo); \
+ struct __kfifo *__kfifo = __tmp; \
+ unsigned int __val = (val); \
+ smp_store_release(&__kfifo->out, __val); \
+})
+
/**
* kfifo_skip - skip output data
* @fifo: address of the fifo to be used
@@ -298,7 +363,7 @@ __kfifo_uint_must_check_helper( \
if (__recsize) \
__kfifo_skip_r(__kfifo, __recsize); \
else \
- __kfifo->out++; \
+ kfifo_write_index_out(__kfifo, __kfifo->out++); \
})
/**
@@ -740,7 +805,8 @@ __kfifo_uint_must_check_helper( \
if (__recsize) \
__kfifo_dma_out_finish_r(__kfifo, __recsize); \
else \
- __kfifo->out += __len / sizeof(*__tmp->type); \
+ kfifo_write_index_out(__kfifo, __kfifo->out \
+ + (__len / sizeof(*__tmp->type))); \
})
/**
diff --git a/lib/kfifo.c b/lib/kfifo.c
index 015656aa8182..189590d9d614 100644
--- a/lib/kfifo.c
+++ b/lib/kfifo.c
@@ -32,7 +32,12 @@
*/
static inline unsigned int kfifo_unused(struct __kfifo *fifo)
{
- return (fifo->mask + 1) - (fifo->in - fifo->out);
+ /*
+ * add memory barrier to make sure the index is fetched
+ * before write to the buffer
+ */
+ return (fifo->mask + 1) -
+ (fifo->in - kfifo_read_index_out(fifo));
}
int __kfifo_alloc(struct __kfifo *fifo, unsigned int size,
@@ -116,11 +121,6 @@ static void kfifo_copy_in(struct __kfifo *fifo, const void *src,
memcpy(fifo->data + off, src, l);
memcpy(fifo->data, src + l, len - l);
- /*
- * make sure that the data in the fifo is up to date before
- * incrementing the fifo->in index counter
- */
- smp_wmb();
}
unsigned int __kfifo_in(struct __kfifo *fifo,
@@ -133,7 +133,11 @@ unsigned int __kfifo_in(struct __kfifo *fifo,
len = l;
kfifo_copy_in(fifo, buf, len, fifo->in);
- fifo->in += len;
+ /*
+ * make sure that the data in the fifo is up to date before
+ * incrementing the fifo->in index counter
+ */
+ kfifo_write_index_in(fifo, fifo->in + len);
return len;
}
EXPORT_SYMBOL(__kfifo_in);
@@ -155,11 +159,6 @@ static void kfifo_copy_out(struct __kfifo *fifo, void *dst,
memcpy(dst, fifo->data + off, l);
memcpy(dst + l, fifo->data, len - l);
- /*
- * make sure that the data is copied before
- * incrementing the fifo->out index counter
- */
- smp_wmb();
}
unsigned int __kfifo_out_peek(struct __kfifo *fifo,
@@ -167,7 +166,7 @@ unsigned int __kfifo_out_peek(struct __kfifo *fifo,
{
unsigned int l;
- l = fifo->in - fifo->out;
+ l = kfifo_read_index_in(fifo) - fifo->out;
if (len > l)
len = l;
@@ -180,7 +179,11 @@ unsigned int __kfifo_out(struct __kfifo *fifo,
void *buf, unsigned int len)
{
len = __kfifo_out_peek(fifo, buf, len);
- fifo->out += len;
+ /*
+ * make sure that the data in the fifo is fetched before
+ * incrementing the fifo->out index counter
+ */
+ kfifo_write_index_out(fifo, fifo->out + len);
return len;
}
EXPORT_SYMBOL(__kfifo_out);
@@ -210,11 +213,6 @@ static unsigned long kfifo_copy_from_user(struct __kfifo *fifo,
if (unlikely(ret))
ret = DIV_ROUND_UP(ret, esize);
}
- /*
- * make sure that the data in the fifo is up to date before
- * incrementing the fifo->in index counter
- */
- smp_wmb();
*copied = len - ret * esize;
/* return the number of elements which are not copied */
return ret;
@@ -241,7 +239,11 @@ int __kfifo_from_user(struct __kfifo *fifo, const void __user *from,
err = -EFAULT;
} else
err = 0;
- fifo->in += len;
+ /*
+ * make sure that the data in the fifo is up to date before
+ * incrementing the fifo->in index counter
+ */
+ kfifo_write_index_in(fifo, fifo->in + len);
return err;
}
EXPORT_SYMBOL(__kfifo_from_user);
@@ -270,11 +272,6 @@ static unsigned long kfifo_copy_to_user(struct __kfifo *fifo, void __user *to,
if (unlikely(ret))
ret = DIV_ROUND_UP(ret, esize);
}
- /*
- * make sure that the data is copied before
- * incrementing the fifo->out index counter
- */
- smp_wmb();
*copied = len - ret * esize;
/* return the number of elements which are not copied */
return ret;
@@ -291,7 +288,7 @@ int __kfifo_to_user(struct __kfifo *fifo, void __user *to,
if (esize != 1)
len /= esize;
- l = fifo->in - fifo->out;
+ l = kfifo_read_index_in(fifo) - fifo->out;
if (len > l)
len = l;
ret = kfifo_copy_to_user(fifo, to, len, fifo->out, copied);
@@ -300,7 +297,11 @@ int __kfifo_to_user(struct __kfifo *fifo, void __user *to,
err = -EFAULT;
} else
err = 0;
- fifo->out += len;
+ /*
+ * make sure that the data is copied before
+ * incrementing the fifo->out index counter
+ */
+ kfifo_write_index_out(fifo, fifo->out + len);
return err;
}
EXPORT_SYMBOL(__kfifo_to_user);
@@ -384,7 +385,7 @@ unsigned int __kfifo_dma_out_prepare(struct __kfifo *fifo,
{
unsigned int l;
- l = fifo->in - fifo->out;
+ l = kfifo_read_index_in(fifo) - fifo->out;
if (len > l)
len = l;
@@ -457,7 +458,11 @@ unsigned int __kfifo_in_r(struct __kfifo *fifo, const void *buf,
__kfifo_poke_n(fifo, len, recsize);
kfifo_copy_in(fifo, buf, len, fifo->in + recsize);
- fifo->in += len + recsize;
+ /*
+ * make sure that the data in the fifo is up to date before
+ * incrementing the fifo->in index counter
+ */
+ kfifo_write_index_in(fifo, fifo->in + len + recsize);
return len;
}
EXPORT_SYMBOL(__kfifo_in_r);
@@ -479,7 +484,7 @@ unsigned int __kfifo_out_peek_r(struct __kfifo *fifo, void *buf,
{
unsigned int n;
- if (fifo->in == fifo->out)
+ if (kfifo_read_index_in(fifo) == fifo->out)
return 0;
return kfifo_out_copy_r(fifo, buf, len, recsize, &n);
@@ -491,11 +496,15 @@ unsigned int __kfifo_out_r(struct __kfifo *fifo, void *buf,
{
unsigned int n;
- if (fifo->in == fifo->out)
+ if (kfifo_read_index_in(fifo) == fifo->out)
return 0;
len = kfifo_out_copy_r(fifo, buf, len, recsize, &n);
- fifo->out += n + recsize;
+ /*
+ * make sure that the fifo data is fetched before
+ * incrementing the fifo->out index counter
+ */
+ kfifo_write_index_out(fifo, fifo->out + n + recsize);
return len;
}
EXPORT_SYMBOL(__kfifo_out_r);
@@ -505,7 +514,11 @@ void __kfifo_skip_r(struct __kfifo *fifo, size_t recsize)
unsigned int n;
n = __kfifo_peek_n(fifo, recsize);
- fifo->out += n + recsize;
+ /*
+ * make sure that the fifo data is fetched before
+ * incrementing the fifo->out index counter
+ */
+ kfifo_write_index_out(fifo, fifo->out + n + recsize);
}
EXPORT_SYMBOL(__kfifo_skip_r);
@@ -528,7 +541,11 @@ int __kfifo_from_user_r(struct __kfifo *fifo, const void __user *from,
*copied = 0;
return -EFAULT;
}
- fifo->in += len + recsize;
+ /*
+ * make sure that the data in the fifo is up to date before
+ * incrementing the fifo->in index counter
+ */
+ kfifo_write_index_in(fifo, fifo->in + len + recsize);
return 0;
}
EXPORT_SYMBOL(__kfifo_from_user_r);
@@ -539,7 +556,7 @@ int __kfifo_to_user_r(struct __kfifo *fifo, void __user *to,
unsigned long ret;
unsigned int n;
- if (fifo->in == fifo->out) {
+ if (kfifo_read_index_in(fifo) == fifo->out) {
*copied = 0;
return 0;
}
@@ -553,7 +570,11 @@ int __kfifo_to_user_r(struct __kfifo *fifo, void __user *to,
*copied = 0;
return -EFAULT;
}
- fifo->out += n + recsize;
+ /*
+ * make sure that the data is copied before
+ * incrementing the fifo->out index counter
+ */
+ kfifo_write_index_out(fifo, fifo->out + n + recsize);
return 0;
}
EXPORT_SYMBOL(__kfifo_to_user_r);
@@ -577,7 +598,11 @@ void __kfifo_dma_in_finish_r(struct __kfifo *fifo,
{
len = __kfifo_max_r(len, recsize);
__kfifo_poke_n(fifo, len, recsize);
- fifo->in += len + recsize;
+ /*
+ * make sure that the data in the fifo is updated before
+ * incrementing the fifo->in index counter
+ */
+ kfifo_write_index_in(fifo, fifo->in + len + recsize);
}
EXPORT_SYMBOL(__kfifo_dma_in_finish_r);
@@ -588,7 +613,7 @@ unsigned int __kfifo_dma_out_prepare_r(struct __kfifo *fifo,
len = __kfifo_max_r(len, recsize);
- if (len + recsize > fifo->in - fifo->out)
+ if (len + recsize > kfifo_read_index_in(fifo) - fifo->out)
return 0;
return setup_sgl(fifo, sgl, nents, len, fifo->out + recsize);
@@ -600,6 +625,10 @@ void __kfifo_dma_out_finish_r(struct __kfifo *fifo, size_t recsize)
unsigned int len;
len = __kfifo_peek_n(fifo, recsize);
- fifo->out += len + recsize;
+ /*
+ * make sure that the data is copied before
+ * incrementing the fifo->out index counter
+ */
+ kfifo_write_index_out(fifo, fifo->out + len + recsize);
}
EXPORT_SYMBOL(__kfifo_dma_out_finish_r);
--
2.17.1
On Mon, Dec 10, 2018 at 7:41 PM <[email protected]> wrote:
>
> From: Yulei Zhang <[email protected]>
>
> Early this year we spot there may be two issues in kernel
> kfifo.
>
> One is reported by Xiao Guangrong to linux kernel.
> https://lkml.org/lkml/2018/5/11/58
> In current kfifo implementation there are missing memory
> barrier in the read side, so that without proper barrier
> between reading the kfifo->in and fetching the data there
> is potential ordering issue.
>
> Beside that, there is another potential issue in kfifo,
> please consider the following case:
> at the beginning
> ring->size = 4
> ring->out = 0
> ring->in = 4
>
> Consumer Producer
> --------------- --------------
> index = ring->out; /* index == 0 */
> ring->out++; /* ring->out == 1 */
> < Re-Order >
> out = ring->out;
> if (ring->in - out >= ring->mask)
> return -EFULL;
> /* see the ring is not full */
> index = ring->in & ring->mask;
> /* index == 0 */
> ring->data[index] = new_data;
> ring->in++;
>
> data = ring->data[index];
> /* you will find the old data is overwritten by the new_data */
>
> In order to avoid the issue:
> 1) for the consumer, we should read the ring->data[] out before
> updating ring->out
> 2) for the producer, we should read ring->out before updating
> ring->data[]
>
> So in this patch we introduce the following four functions which
> are wrapped with proper memory barrier and keep in pairs to make
> sure the in and out index are fetched and updated in order to avoid
> data loss.
>
> kfifo_read_index_in()
> kfifo_write_index_in()
> kfifo_read_index_out()
> kfifo_write_index_out()
>
> Signed-off-by: Yulei Zhang <[email protected]>
> Signed-off-by: Guangrong Xiao <[email protected]>
I've added some more people to CC that might want to see this. Thanks
for sending this!
-Kees
> ---
> include/linux/kfifo.h | 70 ++++++++++++++++++++++++++-
> lib/kfifo.c | 107 +++++++++++++++++++++++++++---------------
> 2 files changed, 136 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
> index 89fc8dc7bf38..3bd2a869ca7e 100644
> --- a/include/linux/kfifo.h
> +++ b/include/linux/kfifo.h
> @@ -286,6 +286,71 @@ __kfifo_uint_must_check_helper( \
> }) \
> )
>
> +/**
> + * kfifo_read_index_in - returns the in index of the fifo
> + * @fifo: address of the kfifo to be used
> + *
> + * add memory read barrier to make sure the fifo->in index
> + * is fetched first before write data to the fifo, which
> + * is paired with the write barrier in kfifo_write_index_in
> + */
> +#define kfifo_read_index_in(kfifo) \
> +({ \
> + typeof((kfifo) + 1) __tmp = (kfifo); \
> + struct __kfifo *__kfifo = __tmp; \
> + unsigned int __val = READ_ONCE(__kfifo->in); \
> + smp_rmb(); \
> + __val; \
> +})
> +
> +/**
> + * kfifo_write_index_in - updates the in index of the fifo
> + * @fifo: address of the kfifo to be used
> + *
> + * add memory write barrier to make sure the data entry is
> + * updated before increase the fifo->in
> + */
> +#define kfifo_write_index_in(kfifo, val) \
> +({ \
> + typeof((kfifo) + 1) __tmp = (kfifo); \
> + struct __kfifo *__kfifo = __tmp; \
> + unsigned int __val = (val); \
> + smp_wmb(); \
> + WRITE_ONCE(__kfifo->in, __val); \
> +})
> +
> +/**
> + * kfifo_read_index_out - returns the out index of the fifo
> + * @fifo: address of the kfifo to be used
> + *
> + * add memory barrier to make sure the fifo->out index is
> + * fetched before read data from the fifo, which is paired
> + * with the memory barrier in kfifo_write_index_out
> + */
> +#define kfifo_read_index_out(kfifo) \
> +({ \
> + typeof((kfifo) + 1) __tmp = (kfifo); \
> + struct __kfifo *__kfifo = __tmp; \
> + unsigned int __val = smp_load_acquire(&__kfifo->out); \
> + __val; \
> +})
> +
> +/**
> + * kfifo_write_index_out - updates the out index of the fifo
> + * @fifo: address of the kfifo to be used
> + *
> + * add memory barrier to make sure reading out the entry before
> + * update the fifo->out index to avoid overwitten the entry by
> + * the producer
> + */
> +#define kfifo_write_index_out(kfifo, val) \
> +({ \
> + typeof((kfifo) + 1) __tmp = (kfifo); \
> + struct __kfifo *__kfifo = __tmp; \
> + unsigned int __val = (val); \
> + smp_store_release(&__kfifo->out, __val); \
> +})
> +
> /**
> * kfifo_skip - skip output data
> * @fifo: address of the fifo to be used
> @@ -298,7 +363,7 @@ __kfifo_uint_must_check_helper( \
> if (__recsize) \
> __kfifo_skip_r(__kfifo, __recsize); \
> else \
> - __kfifo->out++; \
> + kfifo_write_index_out(__kfifo, __kfifo->out++); \
> })
>
> /**
> @@ -740,7 +805,8 @@ __kfifo_uint_must_check_helper( \
> if (__recsize) \
> __kfifo_dma_out_finish_r(__kfifo, __recsize); \
> else \
> - __kfifo->out += __len / sizeof(*__tmp->type); \
> + kfifo_write_index_out(__kfifo, __kfifo->out \
> + + (__len / sizeof(*__tmp->type))); \
> })
>
> /**
> diff --git a/lib/kfifo.c b/lib/kfifo.c
> index 015656aa8182..189590d9d614 100644
> --- a/lib/kfifo.c
> +++ b/lib/kfifo.c
> @@ -32,7 +32,12 @@
> */
> static inline unsigned int kfifo_unused(struct __kfifo *fifo)
> {
> - return (fifo->mask + 1) - (fifo->in - fifo->out);
> + /*
> + * add memory barrier to make sure the index is fetched
> + * before write to the buffer
> + */
> + return (fifo->mask + 1) -
> + (fifo->in - kfifo_read_index_out(fifo));
> }
>
> int __kfifo_alloc(struct __kfifo *fifo, unsigned int size,
> @@ -116,11 +121,6 @@ static void kfifo_copy_in(struct __kfifo *fifo, const void *src,
>
> memcpy(fifo->data + off, src, l);
> memcpy(fifo->data, src + l, len - l);
> - /*
> - * make sure that the data in the fifo is up to date before
> - * incrementing the fifo->in index counter
> - */
> - smp_wmb();
> }
>
> unsigned int __kfifo_in(struct __kfifo *fifo,
> @@ -133,7 +133,11 @@ unsigned int __kfifo_in(struct __kfifo *fifo,
> len = l;
>
> kfifo_copy_in(fifo, buf, len, fifo->in);
> - fifo->in += len;
> + /*
> + * make sure that the data in the fifo is up to date before
> + * incrementing the fifo->in index counter
> + */
> + kfifo_write_index_in(fifo, fifo->in + len);
> return len;
> }
> EXPORT_SYMBOL(__kfifo_in);
> @@ -155,11 +159,6 @@ static void kfifo_copy_out(struct __kfifo *fifo, void *dst,
>
> memcpy(dst, fifo->data + off, l);
> memcpy(dst + l, fifo->data, len - l);
> - /*
> - * make sure that the data is copied before
> - * incrementing the fifo->out index counter
> - */
> - smp_wmb();
> }
>
> unsigned int __kfifo_out_peek(struct __kfifo *fifo,
> @@ -167,7 +166,7 @@ unsigned int __kfifo_out_peek(struct __kfifo *fifo,
> {
> unsigned int l;
>
> - l = fifo->in - fifo->out;
> + l = kfifo_read_index_in(fifo) - fifo->out;
> if (len > l)
> len = l;
>
> @@ -180,7 +179,11 @@ unsigned int __kfifo_out(struct __kfifo *fifo,
> void *buf, unsigned int len)
> {
> len = __kfifo_out_peek(fifo, buf, len);
> - fifo->out += len;
> + /*
> + * make sure that the data in the fifo is fetched before
> + * incrementing the fifo->out index counter
> + */
> + kfifo_write_index_out(fifo, fifo->out + len);
> return len;
> }
> EXPORT_SYMBOL(__kfifo_out);
> @@ -210,11 +213,6 @@ static unsigned long kfifo_copy_from_user(struct __kfifo *fifo,
> if (unlikely(ret))
> ret = DIV_ROUND_UP(ret, esize);
> }
> - /*
> - * make sure that the data in the fifo is up to date before
> - * incrementing the fifo->in index counter
> - */
> - smp_wmb();
> *copied = len - ret * esize;
> /* return the number of elements which are not copied */
> return ret;
> @@ -241,7 +239,11 @@ int __kfifo_from_user(struct __kfifo *fifo, const void __user *from,
> err = -EFAULT;
> } else
> err = 0;
> - fifo->in += len;
> + /*
> + * make sure that the data in the fifo is up to date before
> + * incrementing the fifo->in index counter
> + */
> + kfifo_write_index_in(fifo, fifo->in + len);
> return err;
> }
> EXPORT_SYMBOL(__kfifo_from_user);
> @@ -270,11 +272,6 @@ static unsigned long kfifo_copy_to_user(struct __kfifo *fifo, void __user *to,
> if (unlikely(ret))
> ret = DIV_ROUND_UP(ret, esize);
> }
> - /*
> - * make sure that the data is copied before
> - * incrementing the fifo->out index counter
> - */
> - smp_wmb();
> *copied = len - ret * esize;
> /* return the number of elements which are not copied */
> return ret;
> @@ -291,7 +288,7 @@ int __kfifo_to_user(struct __kfifo *fifo, void __user *to,
> if (esize != 1)
> len /= esize;
>
> - l = fifo->in - fifo->out;
> + l = kfifo_read_index_in(fifo) - fifo->out;
> if (len > l)
> len = l;
> ret = kfifo_copy_to_user(fifo, to, len, fifo->out, copied);
> @@ -300,7 +297,11 @@ int __kfifo_to_user(struct __kfifo *fifo, void __user *to,
> err = -EFAULT;
> } else
> err = 0;
> - fifo->out += len;
> + /*
> + * make sure that the data is copied before
> + * incrementing the fifo->out index counter
> + */
> + kfifo_write_index_out(fifo, fifo->out + len);
> return err;
> }
> EXPORT_SYMBOL(__kfifo_to_user);
> @@ -384,7 +385,7 @@ unsigned int __kfifo_dma_out_prepare(struct __kfifo *fifo,
> {
> unsigned int l;
>
> - l = fifo->in - fifo->out;
> + l = kfifo_read_index_in(fifo) - fifo->out;
> if (len > l)
> len = l;
>
> @@ -457,7 +458,11 @@ unsigned int __kfifo_in_r(struct __kfifo *fifo, const void *buf,
> __kfifo_poke_n(fifo, len, recsize);
>
> kfifo_copy_in(fifo, buf, len, fifo->in + recsize);
> - fifo->in += len + recsize;
> + /*
> + * make sure that the data in the fifo is up to date before
> + * incrementing the fifo->in index counter
> + */
> + kfifo_write_index_in(fifo, fifo->in + len + recsize);
> return len;
> }
> EXPORT_SYMBOL(__kfifo_in_r);
> @@ -479,7 +484,7 @@ unsigned int __kfifo_out_peek_r(struct __kfifo *fifo, void *buf,
> {
> unsigned int n;
>
> - if (fifo->in == fifo->out)
> + if (kfifo_read_index_in(fifo) == fifo->out)
> return 0;
>
> return kfifo_out_copy_r(fifo, buf, len, recsize, &n);
> @@ -491,11 +496,15 @@ unsigned int __kfifo_out_r(struct __kfifo *fifo, void *buf,
> {
> unsigned int n;
>
> - if (fifo->in == fifo->out)
> + if (kfifo_read_index_in(fifo) == fifo->out)
> return 0;
>
> len = kfifo_out_copy_r(fifo, buf, len, recsize, &n);
> - fifo->out += n + recsize;
> + /*
> + * make sure that the fifo data is fetched before
> + * incrementing the fifo->out index counter
> + */
> + kfifo_write_index_out(fifo, fifo->out + n + recsize);
> return len;
> }
> EXPORT_SYMBOL(__kfifo_out_r);
> @@ -505,7 +514,11 @@ void __kfifo_skip_r(struct __kfifo *fifo, size_t recsize)
> unsigned int n;
>
> n = __kfifo_peek_n(fifo, recsize);
> - fifo->out += n + recsize;
> + /*
> + * make sure that the fifo data is fetched before
> + * incrementing the fifo->out index counter
> + */
> + kfifo_write_index_out(fifo, fifo->out + n + recsize);
> }
> EXPORT_SYMBOL(__kfifo_skip_r);
>
> @@ -528,7 +541,11 @@ int __kfifo_from_user_r(struct __kfifo *fifo, const void __user *from,
> *copied = 0;
> return -EFAULT;
> }
> - fifo->in += len + recsize;
> + /*
> + * make sure that the data in the fifo is up to date before
> + * incrementing the fifo->in index counter
> + */
> + kfifo_write_index_in(fifo, fifo->in + len + recsize);
> return 0;
> }
> EXPORT_SYMBOL(__kfifo_from_user_r);
> @@ -539,7 +556,7 @@ int __kfifo_to_user_r(struct __kfifo *fifo, void __user *to,
> unsigned long ret;
> unsigned int n;
>
> - if (fifo->in == fifo->out) {
> + if (kfifo_read_index_in(fifo) == fifo->out) {
> *copied = 0;
> return 0;
> }
> @@ -553,7 +570,11 @@ int __kfifo_to_user_r(struct __kfifo *fifo, void __user *to,
> *copied = 0;
> return -EFAULT;
> }
> - fifo->out += n + recsize;
> + /*
> + * make sure that the data is copied before
> + * incrementing the fifo->out index counter
> + */
> + kfifo_write_index_out(fifo, fifo->out + n + recsize);
> return 0;
> }
> EXPORT_SYMBOL(__kfifo_to_user_r);
> @@ -577,7 +598,11 @@ void __kfifo_dma_in_finish_r(struct __kfifo *fifo,
> {
> len = __kfifo_max_r(len, recsize);
> __kfifo_poke_n(fifo, len, recsize);
> - fifo->in += len + recsize;
> + /*
> + * make sure that the data in the fifo is updated before
> + * incrementing the fifo->in index counter
> + */
> + kfifo_write_index_in(fifo, fifo->in + len + recsize);
> }
> EXPORT_SYMBOL(__kfifo_dma_in_finish_r);
>
> @@ -588,7 +613,7 @@ unsigned int __kfifo_dma_out_prepare_r(struct __kfifo *fifo,
>
> len = __kfifo_max_r(len, recsize);
>
> - if (len + recsize > fifo->in - fifo->out)
> + if (len + recsize > kfifo_read_index_in(fifo) - fifo->out)
> return 0;
>
> return setup_sgl(fifo, sgl, nents, len, fifo->out + recsize);
> @@ -600,6 +625,10 @@ void __kfifo_dma_out_finish_r(struct __kfifo *fifo, size_t recsize)
> unsigned int len;
>
> len = __kfifo_peek_n(fifo, recsize);
> - fifo->out += len + recsize;
> + /*
> + * make sure that the data is copied before
> + * incrementing the fifo->out index counter
> + */
> + kfifo_write_index_out(fifo, fifo->out + len + recsize);
> }
> EXPORT_SYMBOL(__kfifo_dma_out_finish_r);
> --
> 2.17.1
>
--
Kees Cook
On Tue, Dec 11, 2018 at 04:50:34PM -0800, Kees Cook wrote:
> On Mon, Dec 10, 2018 at 7:41 PM <[email protected]> wrote:
> >
> > From: Yulei Zhang <[email protected]>
> >
> > Early this year we spot there may be two issues in kernel
> > kfifo.
> >
> > One is reported by Xiao Guangrong to linux kernel.
> > https://lkml.org/lkml/2018/5/11/58
> > In current kfifo implementation there are missing memory
> > barrier in the read side, so that without proper barrier
> > between reading the kfifo->in and fetching the data there
> > is potential ordering issue.
> >
> > Beside that, there is another potential issue in kfifo,
> > please consider the following case:
> > at the beginning
> > ring->size = 4
> > ring->out = 0
> > ring->in = 4
> >
> > Consumer Producer
> > --------------- --------------
> > index = ring->out; /* index == 0 */
> > ring->out++; /* ring->out == 1 */
> > < Re-Order >
> > out = ring->out;
> > if (ring->in - out >= ring->mask)
> > return -EFULL;
> > /* see the ring is not full */
> > index = ring->in & ring->mask;
> > /* index == 0 */
> > ring->data[index] = new_data;
> > ring->in++;
> >
> > data = ring->data[index];
> > /* you will find the old data is overwritten by the new_data */
> >
> > In order to avoid the issue:
> > 1) for the consumer, we should read the ring->data[] out before
> > updating ring->out
> > 2) for the producer, we should read ring->out before updating
> > ring->data[]
> >
> > So in this patch we introduce the following four functions which
> > are wrapped with proper memory barrier and keep in pairs to make
> > sure the in and out index are fetched and updated in order to avoid
> > data loss.
> >
> > kfifo_read_index_in()
> > kfifo_write_index_in()
> > kfifo_read_index_out()
> > kfifo_write_index_out()
> >
> > Signed-off-by: Yulei Zhang <[email protected]>
> > Signed-off-by: Guangrong Xiao <[email protected]>
>
> I've added some more people to CC that might want to see this. Thanks
> for sending this!
I haven't looked at the guts of kfifo before and I'm fully prepared to
believe that there are ordering problems in there. However, I'm having a
hard time matching the implementation to the snippets above.
Please could you provide the description of the consumer/producer
interaction as above, but annotated with the function/macro names?
There are things like kfifo_get() using smp_wmb(), which looks suspicious,
but doesn't appear to be what you're reporting here.
Thanks,
Will
Hi Yulei,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.20-rc6 next-20181214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/yulei-kernel-gmail-com/kfifo-add-memory-barrier-in-kfifo-to-prevent-data-loss/20181211-204949
reproduce: make htmldocs
All warnings (new ones prefixed by >>):
WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
>> include/linux/kfifo.h:305: warning: Function parameter or member 'kfifo' not described in 'kfifo_read_index_in'
include/linux/kfifo.h:305: warning: Excess function parameter 'fifo' description in 'kfifo_read_index_in'
>> include/linux/kfifo.h:321: warning: Function parameter or member 'kfifo' not described in 'kfifo_write_index_in'
>> include/linux/kfifo.h:321: warning: Function parameter or member 'val' not described in 'kfifo_write_index_in'
include/linux/kfifo.h:321: warning: Excess function parameter 'fifo' description in 'kfifo_write_index_in'
>> include/linux/kfifo.h:337: warning: Function parameter or member 'kfifo' not described in 'kfifo_read_index_out'
include/linux/kfifo.h:337: warning: Excess function parameter 'fifo' description in 'kfifo_read_index_out'
>> include/linux/kfifo.h:353: warning: Function parameter or member 'kfifo' not described in 'kfifo_write_index_out'
>> include/linux/kfifo.h:353: warning: Function parameter or member 'val' not described in 'kfifo_write_index_out'
include/linux/kfifo.h:353: warning: Excess function parameter 'fifo' description in 'kfifo_write_index_out'
include/linux/rcutree.h:1: warning: no structured comments found
kernel/rcu/tree.c:684: warning: Excess function parameter 'irq' description in 'rcu_nmi_exit'
include/linux/srcu.h:175: warning: Function parameter or member 'p' not described in 'srcu_dereference_notrace'
include/linux/srcu.h:175: warning: Function parameter or member 'sp' not described in 'srcu_dereference_notrace'
include/linux/gfp.h:1: warning: no structured comments found
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.ibss' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.connect' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.keys' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.ie' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.ie_len' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.ssid' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.default_key' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.default_mgmt_key' not described in 'wireless_dev'
include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.prev_bssid_valid' not described in 'wireless_dev'
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
vim +305 include/linux/kfifo.h
> 305
306 /**
307 * kfifo_write_index_in - updates the in index of the fifo
308 * @fifo: address of the kfifo to be used
309 *
310 * add memory write barrier to make sure the data entry is
311 * updated before increase the fifo->in
312 */
313 #define kfifo_write_index_in(kfifo, val) \
314 ({ \
315 typeof((kfifo) + 1) __tmp = (kfifo); \
316 struct __kfifo *__kfifo = __tmp; \
317 unsigned int __val = (val); \
318 smp_wmb(); \
319 WRITE_ONCE(__kfifo->in, __val); \
320 })
> 321
322 /**
323 * kfifo_read_index_out - returns the out index of the fifo
324 * @fifo: address of the kfifo to be used
325 *
326 * add memory barrier to make sure the fifo->out index is
327 * fetched before read data from the fifo, which is paired
328 * with the memory barrier in kfifo_write_index_out
329 */
330 #define kfifo_read_index_out(kfifo) \
331 ({ \
332 typeof((kfifo) + 1) __tmp = (kfifo); \
333 struct __kfifo *__kfifo = __tmp; \
334 unsigned int __val = smp_load_acquire(&__kfifo->out); \
335 __val; \
336 })
> 337
338 /**
339 * kfifo_write_index_out - updates the out index of the fifo
340 * @fifo: address of the kfifo to be used
341 *
342 * add memory barrier to make sure reading out the entry before
343 * update the fifo->out index to avoid overwitten the entry by
344 * the producer
345 */
346 #define kfifo_write_index_out(kfifo, val) \
347 ({ \
348 typeof((kfifo) + 1) __tmp = (kfifo); \
349 struct __kfifo *__kfifo = __tmp; \
350 unsigned int __val = (val); \
351 smp_store_release(&__kfifo->out, __val); \
352 })
> 353
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 12/12/18 8:50 AM, Kees Cook wrote:
> On Mon, Dec 10, 2018 at 7:41 PM <[email protected]> wrote:
>>
>> From: Yulei Zhang <[email protected]>
>>
>> Early this year we spot there may be two issues in kernel
>> kfifo.
>>
>> One is reported by Xiao Guangrong to linux kernel.
>> https://lkml.org/lkml/2018/5/11/58
>> In current kfifo implementation there are missing memory
>> barrier in the read side, so that without proper barrier
>> between reading the kfifo->in and fetching the data there
>> is potential ordering issue.
>>
>> Beside that, there is another potential issue in kfifo,
>> please consider the following case:
>> at the beginning
>> ring->size = 4
>> ring->out = 0
>> ring->in = 4
>>
>> Consumer Producer
>> --------------- --------------
>> index = ring->out; /* index == 0 */
>> ring->out++; /* ring->out == 1 */
>> < Re-Order >
>> out = ring->out;
>> if (ring->in - out >= ring->mask)
>> return -EFULL;
>> /* see the ring is not full */
>> index = ring->in & ring->mask;
>> /* index == 0 */
>> ring->data[index] = new_data;
>> ???????????????????????????????? ring->in++;
>>
>> data = ring->data[index];
>> /* you will find the old data is overwritten by the new_data */
>>
>> In order to avoid the issue:
>> 1) for the consumer, we should read the ring->data[] out before
>> updating ring->out
>> 2) for the producer, we should read ring->out before updating
>> ring->data[]
>>
>> So in this patch we introduce the following four functions which
>> are wrapped with proper memory barrier and keep in pairs to make
>> sure the in and out index are fetched and updated in order to avoid
>> data loss.
>>
>> kfifo_read_index_in()
>> kfifo_write_index_in()
>> kfifo_read_index_out()
>> kfifo_write_index_out()
>>
>> Signed-off-by: Yulei Zhang <[email protected]>
>> Signed-off-by: Guangrong Xiao <[email protected]>
>
> I've added some more people to CC that might want to see this. Thanks
> for sending this!
Hi,
Ping... could anyone have a look? ;)
Thanks!
On Thu, Jan 03, 2019 at 07:43:10AM +0000, xiaoguangrong(Xiao Guangrong) wrote:
> On 12/12/18 8:50 AM, Kees Cook wrote:
> > On Mon, Dec 10, 2018 at 7:41 PM <[email protected]> wrote:
> >>
> >> From: Yulei Zhang <[email protected]>
> >>
> >> Early this year we spot there may be two issues in kernel
> >> kfifo.
> >>
> >> One is reported by Xiao Guangrong to linux kernel.
> >> https://lkml.org/lkml/2018/5/11/58
> >> In current kfifo implementation there are missing memory
> >> barrier in the read side, so that without proper barrier
> >> between reading the kfifo->in and fetching the data there
> >> is potential ordering issue.
> >>
> >> Beside that, there is another potential issue in kfifo,
> >> please consider the following case:
> >> at the beginning
> >> ring->size = 4
> >> ring->out = 0
> >> ring->in = 4
> >>
> >> Consumer Producer
> >> --------------- --------------
> >> index = ring->out; /* index == 0 */
> >> ring->out++; /* ring->out == 1 */
> >> < Re-Order >
> >> out = ring->out;
> >> if (ring->in - out >= ring->mask)
> >> return -EFULL;
> >> /* see the ring is not full */
> >> index = ring->in & ring->mask;
> >> /* index == 0 */
> >> ring->data[index] = new_data;
> >> ring->in++;
> >>
> >> data = ring->data[index];
> >> /* you will find the old data is overwritten by the new_data */
> >>
> >> In order to avoid the issue:
> >> 1) for the consumer, we should read the ring->data[] out before
> >> updating ring->out
> >> 2) for the producer, we should read ring->out before updating
> >> ring->data[]
> >>
> >> So in this patch we introduce the following four functions which
> >> are wrapped with proper memory barrier and keep in pairs to make
> >> sure the in and out index are fetched and updated in order to avoid
> >> data loss.
> >>
> >> kfifo_read_index_in()
> >> kfifo_write_index_in()
> >> kfifo_read_index_out()
> >> kfifo_write_index_out()
> >>
> >> Signed-off-by: Yulei Zhang <[email protected]>
> >> Signed-off-by: Guangrong Xiao <[email protected]>
> >
> > I've added some more people to CC that might want to see this. Thanks
> > for sending this!
>
> Hi,
>
> Ping... could anyone have a look? ;)
I've started looking at kfifo, but I suspect it needs a fair amount more
work than your patch. Please stay tuned.
Will