On Fri, May 30, 2008 at 13:44, Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:
> On Fri, May 30 2008, Carl Henrik Lunde wrote:
>> Hi,
>>
>> Can you review this patch? I'm new to locking in the Linux kernel
>> so I may be misssing something.
>>
>> I think we must disable IRQs between relay_reserve and initializing
>> the data; consider the following scenario where task 1 and task 2
>> runs on the same CPU:
>>
>> task 1: trace_note_message task 2: interrupt userspace (blktrace)
>> -------------------------- ----------------- --------------------
>> __trace_note_message read(relay)
>> relay_reserve <blocks ...>
>> <interrupted: I/O completion>
>>
>> __blk_add_trace
>> relay_reserve
>> <buffers switched,
>> wake user>
>> <reads uninitialized
>> trace_note_message>
>> <done>
>> <runs again>
>> memcpy() - too late
>>
>> --
>> Carl Henrik
>
>> From 30fce97a2d7c02ba265eceed59592dbdc9c34f26 Mon Sep 17 00:00:00 2001
>> From: Carl Henrik Lunde <chlunde@xxxxxxxxxxx>
>> Date: Fri, 30 May 2008 12:57:47 +0200
>> Subject: [PATCH] block: disable IRQs until data is written to relay channel
>>
>> As we may run relay_reserve from interrupt context we must always disable
>> IRQs. This is because a call to relay_reserve may expose previously written
>> data to use space.
>>
>> Updated new message code and an old but related comment.
>>
>> Signed-off-by: Carl Henrik Lunde <chlunde@xxxxxxxxxxx>
>> ---
>> block/blktrace.c | 10 ++++------
>> 1 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/blktrace.c b/block/blktrace.c
>> index 7ae87cc..8d3a277 100644
>> --- a/block/blktrace.c
>> +++ b/block/blktrace.c
>> @@ -79,16 +79,17 @@ void __trace_note_message(struct blk_trace *bt, const char *fmt, ...)
>> {
>> int n;
>> va_list args;
>> + unsigned long flags;
>> char *buf;
>>
>> - preempt_disable();
>> + local_irq_save(flags);
>> buf = per_cpu_ptr(bt->msg_data, smp_processor_id());
>> va_start(args, fmt);
>> n = vscnprintf(buf, BLK_TN_MAX_MSG, fmt, args);
>> va_end(args);
>>
>> trace_note(bt, 0, BLK_TN_MESSAGE, buf, n);
>> - preempt_enable();
>> + local_irq_restore(flags);
>
> Good spotting, applied! Thanks.
Hmm, applied where?
--
Carl Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-btrace" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
