Re: [PATCH 2/8] pstore: Expose kmsg_bytes as a module parameter

[Guilherme G. Piccoli]

Hi Kees, my apologies for the (big) delay in answering that! I kept it
marked to respond after tests, ended-up forgetting, but now did all the
tests and finally I'm able to respond.


On 12/10/2022 14:58, Kees Cook wrote:
> [...]
>> I didn't understand exactly how the mount would override things; I've
>> done some tests:
>>
>> (1) booted with the new kmsg_bytes module parameter set to 64k, and it
>> was preserved across multiple mount/umount cycles.
>>
>> (2) When I manually had "-o kmsg_bytes=16k" set during the mount
>> operation, it worked as expected, setting the thing to 16k (and
>> reflecting in the module parameter, as observed in /sys/modules).
> 
> What I was imagining was the next step:
> 
> (3) umount, unload the backend, load a new backend, and mount it
>     without kmsg_bytes specified -- kmsg_bytes will be 16k, not 64k.
> 
> It's a pretty extreme corner-case, I realize. :) However, see below...

Oh okay, thanks for pointing that! Indeed, in your test-case I've faced
the issue of the retained kmsg_bytes...although, I agree it's an extreme
corner-case heheh



> [...]
> Right, kmsg_bytes is the maximum size to save from the console on a
> crash. The design of the ram backend was to handle really small amounts
> of persistent RAM -- if a single crash would eat all of it and possibly
> wrap around, it could write over useful parts at the end (since it's
> written from the end to the front). However, I think somewhere along
> the way, stricter logic was added to the ram backend:
> 
>         /*
>          * Explicitly only take the first part of any new crash.
>          * If our buffer is larger than kmsg_bytes, this can never happen,
>          * and if our buffer is smaller than kmsg_bytes, we don't want the
>          * report split across multiple records.
>          */
>         if (record->part != 1)
>                 return -ENOSPC;
> 
> This limits it to just a single record.

Indeed, and I already considered that in the past...why was that
restricted to a single record, right? I had plans to change it, lemme
know your thoughts. (Reference:
https://lore.kernel.org/linux-fsdevel/a21201cf-1e5f-fed1-356d-42c83a66fa57@xxxxxxxxxx/)



> However, this does _not_ exist for other backends, so they will see up
> to kmsg_bytes-size dumps split across psinfo->bufsize many records. For
> the backends, this record size is not always fixed:
> 
> - efi uses 1024, even though it allocates 4096 (as was pointed out earlier)
> - zone uses kmsg_bytes
> - acpi-erst uses some ACPI value from ACPI_ERST_GET_ERROR_LENGTH
> - ppc-nvram uses the configured size of nvram partition
> 
> Honestly, it seems like the 64k default is huge, but I don't think it
> should be "unlimited" given the behaviors of ppc-nvram, and acpi-erst.
> For ram and efi, it's effectively unlimited because of the small bufsizes
> (and the "only 1 record" logic in ram).
> 
> Existing documentation I can find online seem to imply making it smaller
> (8000 bytes[1], 16000 bytes), but without justification. Even the "main"
> documentation[2] doesn't mention it.

Right! Also, on top of that, there is a kind of "tricky" logic in which
this value is not always respected. For example, in the Steam Deck case
we have a region of ~10MB, and set record size of the ramoops backend to
2MB. This is the amount collected, it doesn't respect kmsg_bytes, since
it checks the amount dumped vs kmsg_bytes effectively _after_ the first
part is written (which in the ramoops case, it's a single write), hence
this check is never "respected" there. I don't consider that as a bug,
more a flexibility for the ramoops case heh

In any way, lemme know if you want to have a "revamp" in the meaning of
kmsg_bytes, I'd be glad in discuss/work on that =)

Thanks,


Guilherme