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

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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.