Corrupt data stream about emlog HOT 4 CLOSED

Huh, that's weird.
Do you have a simple reproducer code by any chance?

Well, I'd suggest adding more pr_debug()'s into the module itself.
For example, you can dump the whole struct emlog_info contents on every read and write to see if there is anything suspicious.

from emlog.

I figured it out.

Race condition between writer and reader. On an embedded system with no preemption this wouldn't be a problem. However on any SMP system it is.

if (*offset < einfo->offset)
      *offset = einfo->offset;

  /* is the user trying to read past EOF? */
  if (*offset >= EMLOG_FIRST_EMPTY_BYTE(einfo))
      return NULL;

  /* find the smaller of the total bytes we have available and what
   * the user is asking for */
  *length = min_t(size_t, *length, EMLOG_FIRST_EMPTY_BYTE(einfo) - *offset);
  remaining = *length;

  /* figure out where to start based on user's offset */
  start_point = einfo->read_point + (*offset - einfo->offset);

If a writer comes in and writes something inbetween the reader reading einfo->offset for the first time and the second time - you end up with the start point being smaller than it should be by the last write.

I guess this isn't a problem on a simple logger - I'm using it for something else though and it is a problem.

I fixed the issue by using a read_lock and a write_lock around the critical sections. Not sure if this is the best approach though, I will most likely be starving readers under high load.

from emlog.

Race condition between writer and reader. On an embedded system with no preemption this wouldn't be a problem. However on any SMP system it is.

Great finding!
Yeah, I think, you're right.
Also, einfo->read_point may be adjusted by write after the read side has calculated start_point.

I guess this isn't a problem on a simple logger - I'm using it for something else though and it is a problem.

Well, it's still a correctness issue, and it may produce quite confusing logs.

As far as I can see, there are a few other races (which don't need to be addressed here):

• with emlog_info_list manipulation (create_einfo() vs free_einfo())
• with new einfo allocation (get_einfo() vs create_einfo())

I fixed the issue by using a read_lock and a write_lock around the critical sections. Not sure if this is the best approach though, I will most likely be starving readers under high load.

I'll be glad to merge your fix as is -- it's better than silently producing corrupt data.
On non-SMP non-preempt kernel they wouldn't add much overhead, and solve the correctness issue on SMP/preemt.
Are the locks per-einfo?

from emlog.

I'll be glad to merge your fix as is -- it's better than silently producing corrupt data.

And we can explore (later) if we can use generic circular buffer implementation (<linux/circ_buf.h> or even <linux/ring_buffer.h>) with independent reader-only and writer-only locks.

from emlog.