On Tue, Dec 11, 2018 at 04:50:34PM -0800, Kees Cook wrote: > On Mon, Dec 10, 2018 at 7:41 PM <yulei.ker...@gmail.com> wrote: > > > > From: Yulei Zhang <yuleixzh...@tencent.com> > > > > 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 <yuleixzh...@tencent.com> > > Signed-off-by: Guangrong Xiao <xiaoguangr...@tencent.com> > > 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