block: disable IRQs until data is written to relay

RFC PATCH In review Carl Henrik Lunde 2 participants 4 messages May 30, 2008 – Jun 11, 2008 Block layer

lore

Share & tools

lore

Carl Henrik Lunde

author

2008-05-30 11:04 UTC #0 lore raw

RFC PATCH 1/1 block: disable IRQs until data is written to relay

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

Signature · 1 line

Carl Henrik

From 30fce97a2d7c02ba265eceed59592dbdc9c34f26 Mon Sep 17 00:00:00 2001
From: Carl Henrik Lunde <[email protected]>
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 <[email protected]>

---
 block/blktrace.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

block/blktrace.c +4 -6

index 7ae87cc..8d3a277 100644
@@ -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);
 }
 EXPORT_SYMBOL_GPL(__trace_note_message);

@@ -158,10 +159,7 @@ void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 	/*
 	 * A word about the locking here - we disable interrupts to reserve
 	 * some space in the relay per-cpu buffer, to prevent an irq
-	 * from coming in and stepping on our toes. Once reserved, it's
-	 * enough to get preemption disabled to prevent read of this data
-	 * before we are through filling it. get_cpu()/put_cpu() does this
-	 * for us
+	 * from coming in and stepping on our toes.
 	 */
 	local_irq_save(flags);

Signature · 1 line

1.5.4.3

Jens Axboe

2008-05-30 11:44 UTC #1 lore raw

Re: [RFC PATCH] block: disable IRQs until data is written to relay channel

Quoted 55 lines · Carl Henrik Lunde

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

Signature · 17 lines

Carl Henrik

From 30fce97a2d7c02ba265eceed59592dbdc9c34f26 Mon Sep 17 00:00:00 2001
From: Carl Henrik Lunde <[email protected]>
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 <[email protected]>
---
 block/blktrace.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

block/blktrace.c +1 -0

index 7ae87cc..8d3a277 100644
@@ -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;

block/blktrace.c void __trace_note_message(struct blk_trace *bt, const char *fmt, ...) · line 92

Quoted 10 lines

block/blktrace.c +2 -2

-	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.

Signature · 1 line

Jens Axboe

Carl Henrik Lunde

author

2008-06-11 12:32 UTC #2 lore raw

Re: [RFC PATCH] block: disable IRQs until data is written to relay channel

In reply to Carl Henrik Lunde

Quoted 63 lines · Jens Axboe

Quoted 62 lines · Carl Henrik Lunde
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 <[email protected]>
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 <[email protected]>
---
 block/blktrace.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
block/blktrace.c +2 -1
index 7ae87cc..8d3a277 100644
@@ -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);
 

Quoted 5 lines

block/blktrace.c void __trace_note_message(struct blk_trace *bt, const char *fmt, ...) · line 92
Quoted 3 lines
block/blktrace.c +1 -1
      trace_note(bt, 0, BLK_TN_MESSAGE, buf, n);
-     preempt_enable();
+     local_irq_restore(flags);
Good spotting, applied! Thanks.

Hmm, applied where?

Signature · 1 line

Carl Henrik

Jens Axboe

2008-06-11 13:06 UTC #3 lore raw

Re: [RFC PATCH] block: disable IRQs until data is written to relay channel

In reply to Carl Henrik Lunde

Quoted 64 lines · Carl Henrik Lunde

Quoted 63 lines · Jens Axboe
Quoted 62 lines · Carl Henrik Lunde
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 <[email protected]>
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 <[email protected]>
---
 block/blktrace.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
block/blktrace.c +2 -1
index 7ae87cc..8d3a277 100644
@@ -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);
 

Quoted 7 lines

Quoted 5 lines
block/blktrace.c void __trace_note_message(struct blk_trace *bt, const char *fmt, ...) · line 92
Quoted 3 lines
block/blktrace.c +1 -1
      trace_note(bt, 0, BLK_TN_MESSAGE, buf, n);
-     preempt_enable();
+     local_irq_restore(flags);
Good spotting, applied! Thanks.
Hmm, applied where?

Still local, I'll get it pushed out for 2.6.26 final for sure.

Signature · 1 line

Jens Axboe