[PATCH] blktrace: always record ftrace events as blk_io_trace2

[PATCH] blktrace: always record ftrace events as blk_io_trace2

[Adriano Cordova]

The ftrace ring buffer always uses the v2 (blk_io_trace2) format, but
__blk_add_trace() switched the reserve size and record format on
bt->version. That field only describes the relay/classic blktrace record
format and must not change what goes into the ftrace buffer: the ftrace
readers (print_one_line() and friends) unconditionally parse blk_io_trace2.

The BLKTRACESETUP ioctl sets bt->version to 1. With the blk tracer also
enabled, __blk_add_trace() recorded a 48-byte v1 event into the ftrace
ring buffer, but the reader parses the 64-byte v2 layout, so pdu_start()
points 16 bytes past the PDU and blk_log_remap() reads out of bounds - a
use-after-free when the ring buffer page is resized concurrently.

Always use the v2 format in the blk_tracer path, and initialize
bt->version to 2 in blk_trace_setup_queue() so the sysfs-enabled path no
longer leaves it uninitialized.

Fixes: e48886b9d668 ("blktrace: for ftrace use correct trace format ver")
Reported-by: syzbot+4dfd96209d744263a972@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=4dfd96209d744263a972
Tested-by: syzbot+4dfd96209d744263a972@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
 kernel/trace/blktrace.c | 74 +++++++++++------------------------------
 1 file changed, 20 insertions(+), 54 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 8cd2520b4c99..ae010969c144 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -385,66 +385,26 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 	if (blk_tracer) {
 		buffer = blk_tr->array_buffer.buffer;
 		trace_ctx = tracing_gen_ctx_flags(0);
-		switch (bt->version) {
-		case 1:
-			trace_len = sizeof(struct blk_io_trace);
-			break;
-		case 2:
-		default:
-			/*
-			 * ftrace always uses v2 (blk_io_trace2) format.
-			 *
-			 * For sysfs-enabled tracing path (enabled via
-			 * /sys/block/DEV/trace/enable), blk_trace_setup_queue()
-			 * never initializes bt->version, leaving it 0 from
-			 * kzalloc(). We must handle version==0 safely here.
-			 *
-			 * Fall through to default to ensure we never hit the
-			 * old bug where default set trace_len=0, causing
-			 * buffer underflow and memory corruption.
-			 *
-			 * Always use v2 format for ftrace and normalize
-			 * bt->version to 2 when uninitialized.
-			 */
-			trace_len = sizeof(struct blk_io_trace2);
-			if (bt->version == 0)
-				bt->version = 2;
-			break;
-		}
-		trace_len += pdu_len + cgid_len;
+		/*
+		 * The ftrace ring buffer always uses the v2 (blk_io_trace2)
+		 * format; the ftrace readers parse only that. bt->version
+		 * describes just the relay/classic blktrace record format.
+		 * Recording a v1-sized event here would shift pdu_start() 16
+		 * bytes past the PDU and make blk_log_remap() read past the
+		 * event (a use-after-free on concurrent buffer resize), and v1
+		 * cannot represent the newer 64-bit zone actions anyway.
+		 */
+		trace_len = sizeof(struct blk_io_trace2) + pdu_len + cgid_len;
 		event = trace_buffer_lock_reserve(buffer, TRACE_BLK,
 						  trace_len, trace_ctx);
 		if (!event)
 			return;
 
 		tracing_record_cmdline(current);
-		switch (bt->version) {
-		case 1:
-			record_blktrace_event(ring_buffer_event_data(event),
-					      pid, cpu, sector, bytes,
-					      what, bt->dev, error, cgid, cgid_len,
-					      pdu_data, pdu_len);
-			break;
-		case 2:
-		default:
-			/*
-			 * Use v2 recording function (record_blktrace_event2)
-			 * which writes blk_io_trace2 structure with correct
-			 * field layout:
-			 *   - 32-bit pid at offset 28
-			 *   - 64-bit action at offset 32
-			 *
-			 * Fall through to default handles version==0 case
-			 * (from sysfs path), ensuring we always use correct
-			 * v2 recording function to match the v2 buffer
-			 * allocated above.
-			 */
-			record_blktrace_event2(ring_buffer_event_data(event),
-					       pid, cpu, sector, bytes,
-					       what, bt->dev, error, cgid, cgid_len,
-					       pdu_data, pdu_len);
-			break;
-		}
+		record_blktrace_event2(ring_buffer_event_data(event),
+				       pid, cpu, sector, bytes,
+				       what, bt->dev, error, cgid, cgid_len,
+				       pdu_data, pdu_len);
 
 		trace_buffer_unlock_commit(blk_tr, buffer, event, trace_ctx);
 		return;
@@ -1913,6 +1873,12 @@ static int blk_trace_setup_queue(struct request_queue *q,
 
 	bt->dev = bdev->bd_dev;
 	bt->act_mask = (u16)-1;
+	/*
+	 * This sysfs-enabled path feeds the ftrace blk tracer, which always
+	 * uses the v2 (blk_io_trace2) format. Initialize the version so it is
+	 * never left dangling as 0 for future consumers.
+	 */
+	bt->version = 2;
 
 	blk_trace_setup_lba(bt, bdev);
 
-- 
2.51.0

Re: [PATCH] blktrace: always record ftrace events as blk_io_trace2

[syzbot]

> #syz test

"" does not look like a valid git branch or commit.

> The ftrace ring buffer always uses the v2 (blk_io_trace2) format, but
> __blk_add_trace() switched the reserve size and record format on
> bt->version. That field only describes the relay/classic blktrace record
> format and must not change what goes into the ftrace buffer: the ftrace
> readers (print_one_line() and friends) unconditionally parse blk_io_trace2.
>
> The BLKTRACESETUP ioctl sets bt->version to 1. With the blk tracer also
> enabled, __blk_add_trace() recorded a 48-byte v1 event into the ftrace
> ring buffer, but the reader parses the 64-byte v2 layout, so pdu_start()
> points 16 bytes past the PDU and blk_log_remap() reads out of bounds - a
> use-after-free when the ring buffer page is resized concurrently.
>
> Always use the v2 format in the blk_tracer path, and initialize
> bt->version to 2 in blk_trace_setup_queue() so the sysfs-enabled path no
> longer leaves it uninitialized.
>
> Fixes: e48886b9d668 ("blktrace: for ftrace use correct trace format ver")
> Assisted-by: opencode:deepseek v4 flash
> ---
>  kernel/trace/blktrace.c | 74 +++++++++++------------------------------
>  1 file changed, 20 insertions(+), 54 deletions(-)
>
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 8cd2520b4c99..ae010969c144 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -385,66 +385,26 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
>  	if (blk_tracer) {
>  		buffer = blk_tr->array_buffer.buffer;
>  		trace_ctx = tracing_gen_ctx_flags(0);
> -		switch (bt->version) {
> -		case 1:
> -			trace_len = sizeof(struct blk_io_trace);
> -			break;
> -		case 2:
> -		default:
> -			/*
> -			 * ftrace always uses v2 (blk_io_trace2) format.
> -			 *
> -			 * For sysfs-enabled tracing path (enabled via
> -			 * /sys/block/DEV/trace/enable), blk_trace_setup_queue()
> -			 * never initializes bt->version, leaving it 0 from
> -			 * kzalloc(). We must handle version==0 safely here.
> -			 *
> -			 * Fall through to default to ensure we never hit the
> -			 * old bug where default set trace_len=0, causing
> -			 * buffer underflow and memory corruption.
> -			 *
> -			 * Always use v2 format for ftrace and normalize
> -			 * bt->version to 2 when uninitialized.
> -			 */
> -			trace_len = sizeof(struct blk_io_trace2);
> -			if (bt->version == 0)
> -				bt->version = 2;
> -			break;
> -		}
> -		trace_len += pdu_len + cgid_len;
> +		/*
> +		 * The ftrace ring buffer always uses the v2 (blk_io_trace2)
> +		 * format; the ftrace readers parse only that. bt->version
> +		 * describes just the relay/classic blktrace record format.
> +		 * Recording a v1-sized event here would shift pdu_start() 16
> +		 * bytes past the PDU and make blk_log_remap() read past the
> +		 * event (a use-after-free on concurrent buffer resize), and v1
> +		 * cannot represent the newer 64-bit zone actions anyway.
> +		 */
> +		trace_len = sizeof(struct blk_io_trace2) + pdu_len + cgid_len;
>  		event = trace_buffer_lock_reserve(buffer, TRACE_BLK,
>  						  trace_len, trace_ctx);
>  		if (!event)
>  			return;
>  
>  		tracing_record_cmdline(current);
> -		switch (bt->version) {
> -		case 1:
> -			record_blktrace_event(ring_buffer_event_data(event),
> -					      pid, cpu, sector, bytes,
> -					      what, bt->dev, error, cgid, cgid_len,
> -					      pdu_data, pdu_len);
> -			break;
> -		case 2:
> -		default:
> -			/*
> -			 * Use v2 recording function (record_blktrace_event2)
> -			 * which writes blk_io_trace2 structure with correct
> -			 * field layout:
> -			 *   - 32-bit pid at offset 28
> -			 *   - 64-bit action at offset 32
> -			 *
> -			 * Fall through to default handles version==0 case
> -			 * (from sysfs path), ensuring we always use correct
> -			 * v2 recording function to match the v2 buffer
> -			 * allocated above.
> -			 */
> -			record_blktrace_event2(ring_buffer_event_data(event),
> -					       pid, cpu, sector, bytes,
> -					       what, bt->dev, error, cgid, cgid_len,
> -					       pdu_data, pdu_len);
> -			break;
> -		}
> +		record_blktrace_event2(ring_buffer_event_data(event),
> +				       pid, cpu, sector, bytes,
> +				       what, bt->dev, error, cgid, cgid_len,
> +				       pdu_data, pdu_len);
>  
>  		trace_buffer_unlock_commit(blk_tr, buffer, event, trace_ctx);
>  		return;
> @@ -1913,6 +1873,12 @@ static int blk_trace_setup_queue(struct request_queue *q,
>  
>  	bt->dev = bdev->bd_dev;
>  	bt->act_mask = (u16)-1;
> +	/*
> +	 * This sysfs-enabled path feeds the ftrace blk tracer, which always
> +	 * uses the v2 (blk_io_trace2) format. Initialize the version so it is
> +	 * never left dangling as 0 for future consumers.
> +	 */
> +	bt->version = 2;
>  
>  	blk_trace_setup_lba(bt, bdev);
>  
> -- 
> 2.51.0
>