Re: [PATCH] pstore/ram: Clarify resource reservation labels

[Kees Cook]

On Thu, Oct 18, 2018 at 2:35 PM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> On Thu, Oct 18, 2018 at 1:31 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>
>> On Thu, Oct 18, 2018 at 8:33 AM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>> > [ add Ross ]
>>
>> Hi Ross! :)
>>
>> > On Thu, Oct 18, 2018 at 12:15 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> >> As for nvdimm specifically, yes, I'd love to get pstore hooked up
>> >> correctly to nvdimm. How do the namespaces work? Right now pstore
>> >> depends one of platform driver data, device tree specification, or
>> >> manual module parameters.
>> >
>> > From the userspace side we have the ndctl utility to wrap
>> > personalities on top of namespaces. So for example, I envision we
>> > would be able to do:
>> >
>> >     ndctl create-namespace --mode=pstore --size=128M
>> >
>> > ...and create a small namespace that will register with the pstore sub-system.
>> >
>> > On the kernel side this would involve registering a 'pstore_dev' child
>> > / seed device under each region device. The 'seed-device' sysfs scheme
>> > is described in our documentation [1]. The short summary is ndctl
>> > finds a seed device assigns a namespace to it and then binding that
>> > device to a driver causes it to be initialized by the kernel.
>> >
>> > [1]: https://www.kernel.org/doc/Documentation/nvdimm/nvdimm.txt
>>
>> Interesting!
>>
>> Really, this would be a way to configure "ramoops" (the persistent RAM
>> backend to pstore), rather than pstore itself (pstore is just the
>> framework). From reading the ndctl man page it sounds like there isn't
>> a way to store configuration information beyond just size?
>>
>> ramoops will auto-configure itself and fill available space using its
>> default parameters, but it might be nice to have a way to store that
>> somewhere (traditionally it's part of device tree or platform data).
>> ramoops could grow a "header", but normally the regions are very small
>> so I've avoided that.
>>
>> I'm not sure I understand the right way to glue ramoops_probe() to the
>> "seed-device" stuff. (It needs to be probed VERY early to catch early
>> crashes -- ramoops uses postcore_initcall() normally.)
>
> Irk, yeah, that's early. On some configurations we can't delineate
> namespaces until after ACPI has come up. Ideally the address range
> would be reserved and communicated in the memory-map from the BIOS.

Yeah, I'm wondering if I should introduce a mode for ramoops where it
walks the memory regions looking for persistent ram areas, and uses
the first available. Something like "ramoops.mem_address=first
ramoops.mem_size=NNN"

> I cringe at users picking addresses because someone is going to enable
> ramoops on top of their persistent memory namespace and wonder why
> their filesystem got clobbered. Should attempts to specify an explicit
> ramoops range that intersects EfiPersistentMemory fail by default? The
> memmap=ss!nn parameter has burned us many times with users picking the
> wrong address, so I'd be inclined to hide this ramoops sharp edge from
> them.

Yeah, this is what I'm trying to solve. I'd like ramoops to find the
address itself, but it has to do it really early, so if I can't have
nvdimm handle it directly, will having regions already allocated with
request_mem_region() "get along" with the rest of nvdimm?

-Kees

-- 
Kees Cook
Pixel Security