zram: block_state returns premature EOF with short read buffers

zram: block_state returns premature EOF with short read buffers

[Pooyan Azadparvar]

Hello,

The zram debugfs block_state file returns zero bytes when it is read
with a buffer smaller than one formatted record.

The issue appears to be in read_block_state() in
drivers/block/zram/zram_drv.c:

        copied = snprintf(kbuf + written, count, ...);

        if (count <= copied) {
                slot_unlock(zram, index);
                break;
        }

When the supplied buffer is too small, snprintf() returns the length
that would have been written, rather than the number of bytes copied.
Since copied is then greater than or equal to count, the function breaks
out of the loop without advancing *ppos or increasing written. It
returns zero even though the file contains an allocated block-state
record.

I reproduced this on:

        Linux debian-Utah-1gb 6.12.63+deb13-cloud-amd64
        x86_64

The relevant kernel configuration is:

        CONFIG_ZRAM=m
        CONFIG_ZRAM_MEMORY_TRACKING=y
        CONFIG_DEBUG_FS=y

Reproducer:

        # modprobe zram
        # echo 16M > /sys/block/zram0/disksize
        # dd if=/dev/urandom of=/dev/zram0 bs=4096 count=1 conv=fsync

The slot is allocated:

        # cat /sys/block/zram0/mm_stat
             4096     4096     4096        0        4096        0
  0        1        1

A normal read returns a record:

        # cat /sys/kernel/debug/zram/zram0/block_state
                   0         1161.269400 ..h...

However, reading the same file with a one-byte buffer returns zero
bytes:

        # python3 - <<'PY'
        import os

        path = "/sys/kernel/debug/zram/zram0/block_state"
        fd = os.open(path, os.O_RDONLY)

        try:
            for attempt in range(3):
                data = os.read(fd, 1)
                offset = os.lseek(fd, 0, os.SEEK_CUR)
                print(
                    f"read {attempt}: bytes={len(data)}, "
                    f"data={data!r}, offset={offset}"
                )
        finally:
            os.close(fd)
        PY

        read 0: bytes=0, data=b'', offset=0
        read 1: bytes=0, data=b'', offset=0
        read 2: bytes=0, data=b'', offset=0

The file contains a valid record, but the short-buffer reads return zero
bytes repeatedly and the file position remains unchanged.

This does not appear to be a memory-safety or security issue. The
observed impact is that userspace readers using a short buffer receive
zero bytes and cannot make progress through this debugfs file.

Could you please confirm whether this short-read behavior is expected
for block_state? If it is not expected, should this be fixed directly
in read_block_state(), or should the file be converted to use seq_file?

Thanks for your time and for maintaining zram.

Pooyan Azad

[PATCH] zram: fix short reads from block_state

[Pooyan Azad]

read_block_state() formats each entry directly into the buffer supplied
by read(). If the remaining buffer is too small for one complete record,
snprintf() returns the full record length and the function stops without
copying data or advancing the file position. A read smaller than a record
therefore returns zero at a non-EOF position and cannot make progress.

Convert block_state to seq_file so formatted records are buffered
independently of the userspace read size. Keep dev_lock held across each
seq_file iteration and continue to protect individual entries with their
slot locks.

Fixes: c0265342bff4 ("zram: introduce zram memory tracking")
Closes: https://lore.kernel.org/r/CANC3H+LdtoydSp+o2ecErAw7k6R2+gRf9LyxcaoHv_mGhJmyQQ@mail.gmail.com/
Signed-off-by: Pooyan Azad <pooyan.azadparvar@gmail.com>
---
Compile-tested with:

  make O=/tmp/zram-build W=1 -j$(nproc) \
       drivers/block/zram/zram_drv.o

No runtime testing of the patched kernel was performed.

 drivers/block/zram/zram_drv.c | 101 ++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 48 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a9b3bb1d3bef..66a2fdb1274b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1542,68 +1542,73 @@ static void zram_debugfs_destroy(void)
 	debugfs_remove_recursive(zram_debugfs_root);
 }
 
-static ssize_t read_block_state(struct file *file, char __user *buf,
-				size_t count, loff_t *ppos)
+static void *zram_block_state_start(struct seq_file *seq, loff_t *pos)
 {
-	char *kbuf;
-	unsigned long index;
-	ssize_t written = 0;
-	struct zram *zram = file->private_data;
+	struct zram *zram = seq->private;
 	unsigned long nr_pages;
 
-	kbuf = kvmalloc(count, GFP_KERNEL);
-	if (!kbuf)
-		return -ENOMEM;
-
-	guard(rwsem_read)(&zram->dev_lock);
-	if (!init_done(zram)) {
-		kvfree(kbuf);
-		return -EINVAL;
-	}
+	down_read(&zram->dev_lock);
+	if (!init_done(zram))
+		return ERR_PTR(-EINVAL);
 
 	nr_pages = zram->disksize >> PAGE_SHIFT;
+	if (*pos >= nr_pages)
+		return NULL;
 
-	for (index = *ppos; index < nr_pages; index++) {
-		int copied;
+	return &zram->table[*pos];
+}
 
-		slot_lock(zram, index);
-		if (!slot_allocated(zram, index))
-			goto next;
+static void *zram_block_state_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+	struct zram *zram = seq->private;
+	unsigned long nr_pages = zram->disksize >> PAGE_SHIFT;
 
-		copied = snprintf(kbuf + written, count,
-			"%12lu %12u.%06d %c%c%c%c%c%c\n",
-			index, zram->table[index].attr.ac_time, 0,
-			test_slot_flag(zram, index, ZRAM_SAME) ? 's' : '.',
-			test_slot_flag(zram, index, ZRAM_WB) ? 'w' : '.',
-			test_slot_flag(zram, index, ZRAM_HUGE) ? 'h' : '.',
-			test_slot_flag(zram, index, ZRAM_IDLE) ? 'i' : '.',
-			get_slot_comp_priority(zram, index) ? 'r' : '.',
-			test_slot_flag(zram, index,
-				       ZRAM_INCOMPRESSIBLE) ? 'n' : '.');
-
-		if (count <= copied) {
-			slot_unlock(zram, index);
-			break;
-		}
-		written += copied;
-		count -= copied;
-next:
+	++*pos;
+	if (*pos >= nr_pages)
+		return NULL;
+
+	return &zram->table[*pos];
+}
+
+static void zram_block_state_stop(struct seq_file *seq, void *v)
+{
+	struct zram *zram = seq->private;
+
+	up_read(&zram->dev_lock);
+}
+
+static int zram_block_state_show(struct seq_file *seq, void *v)
+{
+	struct zram *zram = seq->private;
+	struct zram_table_entry *entry = v;
+	unsigned long index = entry - zram->table;
+
+	slot_lock(zram, index);
+	if (!slot_allocated(zram, index)) {
 		slot_unlock(zram, index);
-		*ppos += 1;
+		return SEQ_SKIP;
 	}
 
-	if (copy_to_user(buf, kbuf, written))
-		written = -EFAULT;
-	kvfree(kbuf);
+	seq_printf(seq, "%12lu %12u.%06d %c%c%c%c%c%c\n",
+		   index, zram->table[index].attr.ac_time, 0,
+		   test_slot_flag(zram, index, ZRAM_SAME) ? 's' : '.',
+		   test_slot_flag(zram, index, ZRAM_WB) ? 'w' : '.',
+		   test_slot_flag(zram, index, ZRAM_HUGE) ? 'h' : '.',
+		   test_slot_flag(zram, index, ZRAM_IDLE) ? 'i' : '.',
+		   get_slot_comp_priority(zram, index) ? 'r' : '.',
+		   test_slot_flag(zram, index, ZRAM_INCOMPRESSIBLE) ? 'n' : '.');
+	slot_unlock(zram, index);
 
-	return written;
+	return 0;
 }
 
-static const struct file_operations proc_zram_block_state_op = {
-	.open = simple_open,
-	.read = read_block_state,
-	.llseek = default_llseek,
+static const struct seq_operations zram_block_state_sops = {
+	.start = zram_block_state_start,
+	.next = zram_block_state_next,
+	.stop = zram_block_state_stop,
+	.show = zram_block_state_show,
 };
+DEFINE_SEQ_ATTRIBUTE(zram_block_state);
 
 static void zram_debugfs_register(struct zram *zram)
 {
@@ -1613,7 +1618,7 @@ static void zram_debugfs_register(struct zram *zram)
 	zram->debugfs_dir = debugfs_create_dir(zram->disk->disk_name,
 						zram_debugfs_root);
 	debugfs_create_file("block_state", 0400, zram->debugfs_dir,
-				zram, &proc_zram_block_state_op);
+				zram, &zram_block_state_fops);
 }
 
 static void zram_debugfs_unregister(struct zram *zram)
-- 
2.43.0

Re: zram: block_state returns premature EOF with short read buffers

[Sergey Senozhatsky]

Sorry for the delay in response, your email somehow fell through
the cracks.

On (26/09/20 18:27), Pooyan Azadparvar wrote:
[..]
> However, reading the same file with a one-byte buffer returns zero
> bytes:
> 
>         # python3 - <<'PY'
>         import os
> 
>         path = "/sys/kernel/debug/zram/zram0/block_state"
>         fd = os.open(path, os.O_RDONLY)
> 
>         try:
>             for attempt in range(3):
>                 data = os.read(fd, 1)
>                 offset = os.lseek(fd, 0, os.SEEK_CUR)
>                 print(
>                     f"read {attempt}: bytes={len(data)}, "
>                     f"data={data!r}, offset={offset}"
>                 )
>         finally:
>             os.close(fd)
>         PY
> 
>         read 0: bytes=0, data=b'', offset=0
>         read 1: bytes=0, data=b'', offset=0
>         read 2: bytes=0, data=b'', offset=0
> 
> The file contains a valid record, but the short-buffer reads return zero
> bytes repeatedly and the file position remains unchanged.
> 
> This does not appear to be a memory-safety or security issue. The
> observed impact is that userspace readers using a short buffer receive
> zero bytes and cannot make progress through this debugfs file.
> 
> Could you please confirm whether this short-read behavior is expected
> for block_state?

I don't think I tend to see this as a problem.  Why would anyone do
a 1-byte (or any other tiny buffer) read in a loop?  block_state holds
a lot of data, just pass a huge buffer maybe?

Re: zram: block_state returns premature EOF with short read buffers

[Sergey Senozhatsky]

On (26/09/29 13:13), Sergey Senozhatsky wrote:
> > However, reading the same file with a one-byte buffer returns zero
> > bytes:
> > 
> >         # python3 - <<'PY'
> >         import os
> > 
> >         path = "/sys/kernel/debug/zram/zram0/block_state"
> >         fd = os.open(path, os.O_RDONLY)
> > 
> >         try:
> >             for attempt in range(3):
> >                 data = os.read(fd, 1)
> >                 offset = os.lseek(fd, 0, os.SEEK_CUR)
> >                 print(
> >                     f"read {attempt}: bytes={len(data)}, "
> >                     f"data={data!r}, offset={offset}"
> >                 )
> >         finally:
> >             os.close(fd)
> >         PY
> > 
> >         read 0: bytes=0, data=b'', offset=0
> >         read 1: bytes=0, data=b'', offset=0
> >         read 2: bytes=0, data=b'', offset=0
> > 
> > The file contains a valid record, but the short-buffer reads return zero
> > bytes repeatedly and the file position remains unchanged.
> > 
> > This does not appear to be a memory-safety or security issue. The
> > observed impact is that userspace readers using a short buffer receive
> > zero bytes and cannot make progress through this debugfs file.
> > 
> > Could you please confirm whether this short-read behavior is expected
> > for block_state?
> 
> I don't think I tend to see this as a problem.  Why would anyone do
> a 1-byte (or any other tiny buffer) read in a loop?  block_state holds
> a lot of data, just pass a huge buffer maybe?

If we can find a real-world library/tool (rust, go, etc.) that does those
short reads then I guess we might want to fix this.

Re: [PATCH] zram: fix short reads from block_state

[Sergey Senozhatsky]

On (26/09/28 18:48), Pooyan Azad wrote:
> read_block_state() formats each entry directly into the buffer supplied
> by read(). If the remaining buffer is too small for one complete record,
> snprintf() returns the full record length and the function stops without
> copying data or advancing the file position. A read smaller than a record
> therefore returns zero at a non-EOF position and cannot make progress.
> 
> Convert block_state to seq_file so formatted records are buffered
> independently of the userspace read size. Keep dev_lock held across each
> seq_file iteration and continue to protect individual entries with their
> slot locks.
> 
> Fixes: c0265342bff4 ("zram: introduce zram memory tracking")
> Closes: https://lore.kernel.org/r/CANC3H+LdtoydSp+o2ecErAw7k6R2+gRf9LyxcaoHv_mGhJmyQQ@mail.gmail.com/
> Signed-off-by: Pooyan Azad <pooyan.azadparvar@gmail.com>

Overall looks good, some comments below.

[..]
> No runtime testing of the patched kernel was performed.

I would prefer some testing, especially given that you have a repro script.

[..]
> +static void *zram_block_state_next(struct seq_file *seq, void *v, loff_t *pos)
> +{
> +	struct zram *zram = seq->private;
> +	unsigned long nr_pages = zram->disksize >> PAGE_SHIFT;
>  
> -		copied = snprintf(kbuf + written, count,
> -			"%12lu %12u.%06d %c%c%c%c%c%c\n",
> -			index, zram->table[index].attr.ac_time, 0,
> -			test_slot_flag(zram, index, ZRAM_SAME) ? 's' : '.',
> -			test_slot_flag(zram, index, ZRAM_WB) ? 'w' : '.',
> -			test_slot_flag(zram, index, ZRAM_HUGE) ? 'h' : '.',
> -			test_slot_flag(zram, index, ZRAM_IDLE) ? 'i' : '.',
> -			get_slot_comp_priority(zram, index) ? 'r' : '.',
> -			test_slot_flag(zram, index,
> -				       ZRAM_INCOMPRESSIBLE) ? 'n' : '.');
> -
> -		if (count <= copied) {
> -			slot_unlock(zram, index);
> -			break;
> -		}
> -		written += copied;
> -		count -= copied;
> -next:
> +	++*pos;
> +	if (*pos >= nr_pages)
> +		return NULL;
> +
> +	return &zram->table[*pos];
> +}

Can you return pos instead?  (and handle v as a pointer to offset in
other functions.)

[..]
> +static int zram_block_state_show(struct seq_file *seq, void *v)
> +{
> +	struct zram *zram = seq->private;
> +	struct zram_table_entry *entry = v;
> +	unsigned long index = entry - zram->table;
> +
> +	slot_lock(zram, index);
> +	if (!slot_allocated(zram, index)) {
>  		slot_unlock(zram, index);
> -		*ppos += 1;
> +		return SEQ_SKIP;
>  	}

I guess we can just do

    +static int block_state_show(struct seq_file *s, void *v)
    +{
    +    struct zram *zram = s->private;
    +    unsigned long index = *(loff_t *)v;
    +
    +    slot_lock(zram, index);
    +    if (slot_allocated(zram, index)) {
    +        seq_printf(s, "%12lu %12u.%06d %c%c%c%c%c%c\n",
    +               index, zram->table[index].attr.ac_time, 0,
    +               test_slot_flag(zram, index, ZRAM_SAME) ? 's' : '.',
    +               test_slot_flag(zram, index, ZRAM_WB) ? 'w' : '.',
    +               test_slot_flag(zram, index, ZRAM_HUGE) ? 'h' : '.',
    +               test_slot_flag(zram, index, ZRAM_IDLE) ? 'i' : '.',
    +               get_slot_comp_priority(zram, index) ? 'r' : '.',
    +               test_slot_flag(zram, index,
    +                      ZRAM_INCOMPRESSIBLE) ? 'n' : '.');
         }
    +    slot_unlock(zram, index);

    -    if (copy_to_user(buf, kbuf, written))
    -        written = -EFAULT;
    -    kvfree(kbuf);
    -
    -    return written;
    +    return 0;
     }

The SEQ_SKIP branch is probably not needed.  We don't advance s->count
for un-allocated entries, which should be enough, I guess.

[PATCH v2] zram: fix short reads from block_state

[Pooyan Azad]

read_block_state() formats each entry directly into the buffer supplied
by read(). If the remaining buffer is too small for one complete record,
snprintf() returns the full record length and the function stops without
copying data or advancing the file position. A read smaller than a record
therefore returns zero at a non-EOF position and cannot make progress.

Convert block_state to seq_file so formatted records are buffered
independently of the userspace read size. Keep dev_lock held across each
seq_file iteration and continue to protect individual entries with their
slot locks.

Fixes: c0265342bff4 ("zram: introduce zram memory tracking")
Closes: https://lore.kernel.org/r/CANC3H+LdtoydSp+o2ecErAw7k6R2+gRf9LyxcaoHv_mGhJmyQQ@mail.gmail.com/
Signed-off-by: Pooyan Azad <pooyan.azadparvar@gmail.com>
---
Changes in v2:
- Return the seq_file position from the iterator callbacks.
- Treat the iterator value as a pointer to the current offset.
- Avoid SEQ_SKIP for unallocated slots.
- Runtime-tested full and repeated one-byte reads under UML.

The one-byte reader is a boundary-case regression test. The same
zero-progress behavior occurs with any userspace buffer smaller than one
formatted block_state record.

Runtime testing was performed under UML with
CONFIG_ZRAM_MEMORY_TRACKING=y. A full read and repeated one-byte reads
produced matching output, and every short read advanced the file position.

 drivers/block/zram/zram_drv.c | 102 +++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 50 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a9b3bb1d3bef..6b39c22823f5 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1542,68 +1542,70 @@ static void zram_debugfs_destroy(void)
 	debugfs_remove_recursive(zram_debugfs_root);
 }
 
-static ssize_t read_block_state(struct file *file, char __user *buf,
-				size_t count, loff_t *ppos)
+static void *zram_block_state_start(struct seq_file *seq, loff_t *pos)
 {
-	char *kbuf;
-	unsigned long index;
-	ssize_t written = 0;
-	struct zram *zram = file->private_data;
+	struct zram *zram = seq->private;
 	unsigned long nr_pages;
 
-	kbuf = kvmalloc(count, GFP_KERNEL);
-	if (!kbuf)
-		return -ENOMEM;
-
-	guard(rwsem_read)(&zram->dev_lock);
-	if (!init_done(zram)) {
-		kvfree(kbuf);
-		return -EINVAL;
-	}
+	down_read(&zram->dev_lock);
+	if (!init_done(zram))
+		return ERR_PTR(-EINVAL);
 
 	nr_pages = zram->disksize >> PAGE_SHIFT;
+	if (*pos >= nr_pages)
+		return NULL;
 
-	for (index = *ppos; index < nr_pages; index++) {
-		int copied;
+	return pos;
+}
 
-		slot_lock(zram, index);
-		if (!slot_allocated(zram, index))
-			goto next;
+static void *zram_block_state_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+	struct zram *zram = seq->private;
+	unsigned long nr_pages = zram->disksize >> PAGE_SHIFT;
 
-		copied = snprintf(kbuf + written, count,
-			"%12lu %12u.%06d %c%c%c%c%c%c\n",
-			index, zram->table[index].attr.ac_time, 0,
-			test_slot_flag(zram, index, ZRAM_SAME) ? 's' : '.',
-			test_slot_flag(zram, index, ZRAM_WB) ? 'w' : '.',
-			test_slot_flag(zram, index, ZRAM_HUGE) ? 'h' : '.',
-			test_slot_flag(zram, index, ZRAM_IDLE) ? 'i' : '.',
-			get_slot_comp_priority(zram, index) ? 'r' : '.',
-			test_slot_flag(zram, index,
-				       ZRAM_INCOMPRESSIBLE) ? 'n' : '.');
-
-		if (count <= copied) {
-			slot_unlock(zram, index);
-			break;
-		}
-		written += copied;
-		count -= copied;
-next:
-		slot_unlock(zram, index);
-		*ppos += 1;
-	}
+	++*pos;
+	if (*pos >= nr_pages)
+		return NULL;
+
+	return pos;
+}
+
+static void zram_block_state_stop(struct seq_file *seq, void *v)
+{
+	struct zram *zram = seq->private;
 
-	if (copy_to_user(buf, kbuf, written))
-		written = -EFAULT;
-	kvfree(kbuf);
+	up_read(&zram->dev_lock);
+}
+
+static int zram_block_state_show(struct seq_file *seq, void *v)
+{
+	struct zram *zram = seq->private;
+	unsigned long index = *(loff_t *)v;
+
+	slot_lock(zram, index);
+	if (slot_allocated(zram, index)) {
+		seq_printf(seq, "%12lu %12u.%06d %c%c%c%c%c%c\n",
+			   index, zram->table[index].attr.ac_time, 0,
+			   test_slot_flag(zram, index, ZRAM_SAME) ? 's' : '.',
+			   test_slot_flag(zram, index, ZRAM_WB) ? 'w' : '.',
+			   test_slot_flag(zram, index, ZRAM_HUGE) ? 'h' : '.',
+			   test_slot_flag(zram, index, ZRAM_IDLE) ? 'i' : '.',
+			   get_slot_comp_priority(zram, index) ? 'r' : '.',
+			   test_slot_flag(zram, index,
+					  ZRAM_INCOMPRESSIBLE) ? 'n' : '.');
+	}
+	slot_unlock(zram, index);
 
-	return written;
+	return 0;
 }
 
-static const struct file_operations proc_zram_block_state_op = {
-	.open = simple_open,
-	.read = read_block_state,
-	.llseek = default_llseek,
+static const struct seq_operations zram_block_state_sops = {
+	.start = zram_block_state_start,
+	.next = zram_block_state_next,
+	.stop = zram_block_state_stop,
+	.show = zram_block_state_show,
 };
+DEFINE_SEQ_ATTRIBUTE(zram_block_state);
 
 static void zram_debugfs_register(struct zram *zram)
 {
@@ -1613,7 +1615,7 @@ static void zram_debugfs_register(struct zram *zram)
 	zram->debugfs_dir = debugfs_create_dir(zram->disk->disk_name,
 						zram_debugfs_root);
 	debugfs_create_file("block_state", 0400, zram->debugfs_dir,
-				zram, &proc_zram_block_state_op);
+				zram, &zram_block_state_fops);
 }
 
 static void zram_debugfs_unregister(struct zram *zram)
-- 
2.43.0

Re: [PATCH v2] zram: fix short reads from block_state

[Sergey Senozhatsky]

On (26/09/29 09:18), Pooyan Azad wrote:
> read_block_state() formats each entry directly into the buffer supplied
> by read(). If the remaining buffer is too small for one complete record,
> snprintf() returns the full record length and the function stops without
> copying data or advancing the file position. A read smaller than a record
> therefore returns zero at a non-EOF position and cannot make progress.
> 
> Convert block_state to seq_file so formatted records are buffered
> independently of the userspace read size. Keep dev_lock held across each
> seq_file iteration and continue to protect individual entries with their
> slot locks.
> 
> Fixes: c0265342bff4 ("zram: introduce zram memory tracking")
> Closes: https://lore.kernel.org/r/CANC3H+LdtoydSp+o2ecErAw7k6R2+gRf9LyxcaoHv_mGhJmyQQ@mail.gmail.com/
> Signed-off-by: Pooyan Azad <pooyan.azadparvar@gmail.com>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>