Re: [PATCH 1/3] relay: Fix 4 off-by-one errors occuring when writing to a CPU buffer.

[Tom Zanussi <tzanussi@gmail.com>]

Hi,

On Fri, 2008-06-13 at 04:09 +0300, Eduard - Gabriel Munteanu wrote:
> Semantically, start + offset is always lower than the total size in bytes,
> so the code should check against equality to size, not size + 1. When
> userspace did read() (but not necessarily restricted to this), this
> resulted in all-zeros data at the end of the buffer.
> 

I'm wondering if the all-zeroes at the end of the buffer might be
another case of the all-zeroes you were seeing due to cross-cpu reading
you decribed in the other patch.  In any case, I'm pretty sure this
patch isn't doing what you think it is, and don't see how it could have
fixed the problem (see below).  There may still be a bug somewhere, but
it would be good to be able to reproduce it.  Does it happen even when
running on a single cpu?

> Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
> ---
>  include/linux/relay.h |    4 ++--
>  kernel/relay.c        |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/relay.h b/include/linux/relay.h
> index 6cd8c44..8593ca1 100644
> --- a/include/linux/relay.h
> +++ b/include/linux/relay.h
> @@ -202,7 +202,7 @@ static inline void relay_write(struct rchan *chan,
>  
>  	local_irq_save(flags);
>  	buf = chan->buf[smp_processor_id()];
> -	if (unlikely(buf->offset + length > chan->subbuf_size))
> +	if (unlikely(buf->offset + length >= chan->subbuf_size))

The original code basically says, "if the current write won't fit in the
remaining space in current subbuffer, move on to the next sub-buffer".
Your change makes it "if the current write won't fit in the current
subbuffer or if it exactly fits, move on to the next sub-buffer", which
isn't what we want.

>  		length = relay_switch_subbuf(buf, length);
>  	memcpy(buf->data + buf->offset, data, length);
>  	buf->offset += length;
> @@ -228,7 +228,7 @@ static inline void __relay_write(struct rchan *chan,
>  	struct rchan_buf *buf;
>  
>  	buf = chan->buf[get_cpu()];
> -	if (unlikely(buf->offset + length > buf->chan->subbuf_size))

Same thing here.

> +	if (unlikely(buf->offset + length >= buf->chan->subbuf_size))
>  		length = relay_switch_subbuf(buf, length);
>  	memcpy(buf->data + buf->offset, data, length);
>  	buf->offset += length;
> diff --git a/kernel/relay.c b/kernel/relay.c
> index 7de644c..07f25e7 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -622,7 +622,7 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
>  	if (unlikely(length > buf->chan->subbuf_size))
>  		goto toobig;
>  
> -	if (buf->offset != buf->chan->subbuf_size + 1) {
> +	if (buf->offset != buf->chan->subbuf_size) {

This case, offset being 1 larger than the subbuf size, is how we note a
full sub-buffer, so changing this will break full-subbuffer cases. 

>  		buf->prev_padding = buf->chan->subbuf_size - buf->offset;
>  		old_subbuf = buf->subbufs_produced % buf->chan->n_subbufs;
>  		buf->padding[old_subbuf] = buf->prev_padding;
> @@ -645,7 +645,7 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
>  	new = buf->start + new_subbuf * buf->chan->subbuf_size;
>  	buf->offset = 0;
>  	if (!buf->chan->cb->subbuf_start(buf, new, old, buf->prev_padding)) {
> -		buf->offset = buf->chan->subbuf_size + 1;
> +		buf->offset = buf->chan->subbuf_size;

Same here.

Tom


>  		return 0;
>  	}
>  	buf->data = new;