ramoops: use pstore interface
Subsystems (2)
PATCH v4 1/1 ramoops: use pstore interface
Instead of using /dev/mem directly, use the common pstore infrastructure to handle Oops gathering and extraction.
---
v4:
- rebased to Linus's tree. Still time to get into merge window?
v3:
- Added more documentation about which memory regions it would be used with.
v2:
- general reworking.
--- Documentation/ramoops.txt | 8 +- drivers/char/Kconfig | 1 + drivers/char/ramoops.c | 218 +++++++++++++++++++++++++++++++++----------- 3 files changed, 168 insertions(+), 59 deletions(-)
Documentation/ramoops.txt +4 -4
@@ -3,7 +3,7 @@ Ramoops oops/panic logger Sergiu Iordache <[email protected]> -Updated: 8 August 2011 +Updated: 17 November 2011 0. Introduction
@@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues with the actual data. 4. Reading the data -The dump data can be read from memory (through /dev/mem or other means). -Getting the module parameters, which are needed in order to parse the data, can -be done through /sys/module/ramoops/parameters/* . +The dump data can be read from the pstore filesystem. The format for these +files is "dmesg-ramoops-N", where N is the record number in memory. To delete +a stored record from RAM, simply unlink the respective pstore file.
drivers/char/Kconfig +1 -0
@@ -603,6 +603,7 @@ source "drivers/s390/char/Kconfig" config RAMOOPS tristate "Log panic/oops to a RAM buffer" depends on HAS_IOMEM + depends on PSTORE default n help This enables panic and oops messages to be logged to a circular
drivers/char/ramoops.c +163 -55
318 lines not on this page · raw message
Signature · 6 lines
1.7.4.1 -- Kees Cook ChromeOS Security
Quoted 2 lines · On Sat, 7 Jan 2012 09:15:16 -0800 Kees Cook <[email protected]> wrote:
Instead of using
/dev/memdirectly, use the common pstore infrastructure to handle Oops gathering and extraction.
um, why? This changelog provides no reason for anyone to apply the patch!
drivers/char/ramoops.c module_param(dump_oops, int, 0600); · line 77
Quoted 9 lines
...
drivers/char/ramoops.c +3
-3-static void ramoops_do_dump (struct kmsg_dumper *dumper, -enum kmsg_dump_reason reason,const char *s1,unsigned long l1, -const char *s2,unsigned long l2) +static int ramoops_pstore_open (struct pstore_info *psi) +{ +struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
Unneeded and undesirable cast of void*. Multiple instances of this.
Quoted 9 lines
drivers/char/ramoops.c +9
-0+ cxt->read_count =0 ; +return 0 ; +} + +static ssize_t ramoops_pstore_read (u64 *id,enum pstore_type_id *type, +struct timespec *time, +char **buf, +struct pstore_info *psi) +{
drivers/char/ramoops.c module_param(dump_oops, int, 0600); · line 97
Quoted 10 lines
drivers/char/ramoops.c +10
-0+ssize_t size; +char *rambuf; +struct ramoops_context *cxt = (struct ramoops_context *)psi->data; + +if (cxt->read_count >= cxt->max_count) +return -EINVAL ; + *id = cxt->read_count++; +/* Only supports dmesg output so far. */ + *type =PSTORE_TYPE_DMESG ; +/* TODO(kees): Bogus time for the moment. */
Is this hard to fix now?
drivers/char/ramoops.c module_param(dump_oops, int, 0600); · line 109
Quoted 12 lines
drivers/char/ramoops.c +12
-0+ time->tv_sec =0 ; + time->tv_nsec =0 ; + + rambuf = cxt->virt_addr + (*id * cxt->record_size); + size =strnlen (rambuf, cxt->record_size); + *buf =kmalloc (size,GFP_KERNEL ); +if (*buf ==NULL ) +return -ENOMEM ; +memcpy (*buf, rambuf, size); + +return size; +}
Note that pstore_get_records() will treat the -ve errno returns from ->read() in the same manner as EOF. IOW, your error codes will be dropped on the floor. This appears to be a bug in pstore_get_records().
Quoted 22 lines
drivers/char/ramoops.c +13
-6+static int ramoops_pstore_write (enum pstore_type_id type, +enum kmsg_dump_reason reason, +u64 *id, +unsigned int part, +size_t size,struct pstore_info *psi) { -struct ramoops_context *cxt =container_of (dumper, - struct ramoops_context, dump); -unsigned long s1_start, s2_start; -unsigned long l1_cpy, l2_cpy; -int res, hdr_size; -char *buf, *buf_orig; +char *buf; +size_t res;struct timeval timestamp; +struct ramoops_context *cxt = (struct ramoops_context *)psi->data; +size_t available = cxt->record_size; +/* Only store dmesg dumps. */ +if (type !=PSTORE_TYPE_DMESG ) +return -EINVAL ; +
drivers/char/ramoops.c module_param(dump_oops, int, 0600); · line 133
Quoted 10 lines
drivers/char/ramoops.c +3
-3+/* Only store crash dumps. */ if (reason !=KMSG_DUMP_OOPS && - reason !=KMSG_DUMP_PANIC && - reason !=KMSG_DUMP_KEXEC ) -return ; + reason !=KMSG_DUMP_PANIC ) +return -EINVAL ;/* Only dump oopses if dump_oops is set */ if (reason ==KMSG_DUMP_OOPS && !cxt->dump_oops)
The above three comments describe what the code does, which was obvious. They failed to describe why it does this, which was unobvious. Sigh.
Quoted 22 lines
drivers/char/ramoops.c +10
-3-return ; +return -EINVAL ; + +/* 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 (part !=1 ) +return -ENOSPC ; buf = cxt->virt_addr + (cxt->count * cxt->record_size); - buf_orig = buf; -memset (buf,'\0' , cxt->record_size); res =sprintf (buf,"%s" ,RAMOOPS_KERNMSG_HDR ); buf += res; + available -= res; +do_gettimeofday (×tamp);
Quoted 8 lines · Andrew Morton
Quoted 2 lines · On Sat, 7 Jan 2012 09:15:16 -0800 Kees Cook <[email protected]> wrote:
Instead of using
/dev/memdirectly, use the common pstore infrastructure to handle Oops gathering and extraction.um, why? This changelog provides no reason for anyone to apply the patch!
Good point; this was lost as the patch cycled. I will expand this.
Quoted 3 lines
Quoted 1 line
+ struct
ramoops_context*cxt = (structramoops_context*)psi->data;Unneeded and undesirable cast of void*. Multiple instances of this.
Ah yes; I will fix this.
Quoted 3 lines
Quoted 1 line
+ /* TODO(kees): Bogus time for the moment. */
Is this hard to fix now?
I felt it was out of scope for the moment. There was enough change happening for bolting it to pstore that adding a header with magic values, etc, seemed like a logically separate task. As such, I left this as TODO.
Quoted 3 lines
Note that
pstore_get_records()will treat the -ve errno returns from ->read() in the same manner as EOF. IOW, your error codes will be dropped on the floor. This appears to be a bug inpstore_get_records().
Well, IIUC, it just means the file doesn't get populated at all; there is no userspace interface to finding out why a file didn't appear in the pstore fliesystem. But yes, the specifics of the error are ignored by pstore_get_records(). It didn't seem wrong to produce meaningful codes in ramoops, though.
Quoted 12 lines
Quoted 12 lines
+ /* Only store dmesg dumps. */
+ if (type !=PSTORE_TYPE_DMESG)
+ return-EINVAL;
+
+ /* Only store crash dumps. */
if (reason !=KMSG_DUMP_OOPS&&
- reason !=KMSG_DUMP_PANIC&&
- reason !=KMSG_DUMP_KEXEC)
- return;
+ reason !=KMSG_DUMP_PANIC)
+ return-EINVAL;
Quoted 6 lines
Quoted 2 lines
/* Only dump oopses if
dump_oopsis set */ if (reason ==KMSG_DUMP_OOPS&& !cxt->dump_oops)The above three comments describe what the code does, which was obvious. They failed to describe why it does this, which was unobvious. Sigh.
They were terse; I will attempt to expand on them.
Thanks for the review! I will send v5 shortly...
-Kees
Signature · 2 lines
Kees Cook ChromeOS Security
Quoted 10 lines · On Tue, 17 Jan 2012 15:48:25 -0800 Kees Cook <[email protected]> wrote:
Quoted 3 lines
Note that
pstore_get_records()will treat the -ve errno returns from ->read() in the same manner as EOF. IOW, your error codes will be dropped on the floor. This appears to be a bug inpstore_get_records().Well, IIUC, it just means the file doesn't get populated at all; there is no userspace interface to finding out why a file didn't appear in the pstore fliesystem. But yes, the specifics of the error are ignored by
pstore_get_records(). It didn't seem wrong to produce meaningful codes in ramoops, though.
Well, there's a printk() in there. But it only comes out if it was pstore_mkfile() which failed.