For a long time I've been bothered by the complexity of argument passing
in the pstore internals, which makes understanding things and changing
things extremely fragile.
With the proposal of a new backend (EPI capsules), and my attempts to
reorganize things for the proposed multiple-pmsg frontend, I've spent
some time refactoring the internal pstore API to pass a record structure
instead of lots of arguments. This hugely simplifies things, and lets me
document the API better, reduce memory copies, etc.
Included in the series are a few bug fixes as well.
If pstore backend authors could review the changes I've made in their
drivers, I'd appreciate it. Hopefully I got it all right. :) And unless
there are any objections, I'll put this into -next in a couple days.
I intend to continue working on this to fix up the "ecc_notification_size"
field and make it more generalized, and get multiple pmsg support added
too, but I'd like to get this first series out for review first, since
it's rather long already.
Thanks!
-Kees
This adds documentation for struct pstore_info, which also includes
the basic API the backends need to implement.
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/pstore.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 128 insertions(+), 5 deletions(-)
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 0da29cae009b..083b10bacd4a 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -30,7 +30,7 @@
#include <linux/time.h>
#include <linux/types.h>
-/* types */
+/* pstore record types (see fs/pstore/inode.c for filename templates) */
enum pstore_type_id {
PSTORE_TYPE_DMESG = 0,
PSTORE_TYPE_MCE = 1,
@@ -47,14 +47,138 @@ enum pstore_type_id {
struct module;
+/**
+ * struct pstore_info - backend pstore driver structure
+ *
+ * @owner: module which is repsonsible for this backend driver
+ * @name: name of the backend driver
+ *
+ * @buf_lock: spinlock to serialize access to @buf
+ * @buf: preallocated crash dump buffer
+ * @bufsize: size of @buf available for crash dump writes
+ *
+ * @read_mutex: mutex to serialize @open, @read, and @close callbacks
+ * @flags: bitfield of frontends the backend can accept writes for
+ * @data: backend-private pointer passed back during callbacks
+ *
+ * Callbacks:
+ *
+ * @open:
+ * Notify backend that pstore is starting a full read of backend
+ * records. Followed by one or more @read calls, and a final @close.
+ *
+ * @psi: in: pointer to the struct pstore_info for the backend
+ *
+ * Returns 0 on success, and non-zero on error.
+ *
+ * @close:
+ * Notify backend that pstore has finished a full read of backend
+ * records. Always preceded by an @open call and one or more @read
+ * calls.
+ *
+ * @psi: in: pointer to the struct pstore_info for the backend
+ *
+ * Returns 0 on success, and non-zero on error. (Though pstore will
+ * ignore the error.)
+ *
+ * @read:
+ * Read next available backend record. Called after a successful
+ * @open.
+ *
+ * @id: out: unique identifier for the record
+ * @type: out: pstore record type
+ * @count: out: for PSTORE_TYPE_DMESG, the Oops count.
+ * @time: out: timestamp for the record
+ * @buf: out: kmalloc copy of record contents, to be freed by pstore
+ * @compressed:
+ * out: if the record contents are compressed
+ * @ecc_notice_size:
+ * out: ECC information
+ * @psi: in: pointer to the struct pstore_info for the backend
+ *
+ * Returns record size on success, zero when no more records are
+ * available, or negative on error.
+ *
+ * @write:
+ * Perform a frontend notification of a write to a backend record. The
+ * data to be stored has already been written to the registered @buf
+ * of the @psi structure.
+ *
+ * @type: in: pstore record type to write
+ * @reason:
+ * in: pstore write reason
+ * @id: out: unique identifier for the record
+ * @part: in: position in a multipart write
+ * @count: in: increasing from 0 since boot, the number of this Oops
+ * @compressed:
+ * in: if the record is compressed
+ * @size: in: size of the write
+ * @psi: in: pointer to the struct pstore_info for the backend
+ *
+ * Returns 0 on success, and non-zero on error.
+ *
+ * @write_buf:
+ * Perform a frontend write to a backend record, using a specified
+ * buffer. Unlike @write, this does not use the @psi @buf.
+ *
+ * @type: in: pstore record type to write
+ * @reason:
+ * in: pstore write reason
+ * @id: out: unique identifier for the record
+ * @part: in: position in a multipart write
+ * @buf: in: pointer to contents to write to backend record
+ * @compressed:
+ * in: if the record is compressed
+ * @size: in: size of the write
+ * @psi: in: pointer to the struct pstore_info for the backend
+ *
+ * Returns 0 on success, and non-zero on error.
+ *
+ * @write_buf_user:
+ * Perform a frontend write to a backend record, using a specified
+ * buffer that is coming directly from userspace.
+ *
+ * @type: in: pstore record type to write
+ * @reason:
+ * in: pstore write reason
+ * @id: out: unique identifier for the record
+ * @part: in: position in a multipart write
+ * @buf: in: pointer to userspace contents to write to backend record
+ * @compressed:
+ * in: if the record is compressed
+ * @size: in: size of the write
+ * @psi: in: pointer to the struct pstore_info for the backend
+ *
+ * Returns 0 on success, and non-zero on error.
+ *
+ * @erase:
+ * Delete a record from backend storage. Different backends
+ * identify records differently, so all possible methods of
+ * identification are included to help the backend locate the
+ * record to remove.
+ *
+ * @type: in: pstore record type to write
+ * @id: in: per-type unique identifier for the record
+ * @count: in: Oops count
+ * @time: in: timestamp for the record
+ * @psi: in: pointer to the struct pstore_info for the backend
+ *
+ * Returns 0 on success, and non-zero on error.
+ *
+ */
struct pstore_info {
struct module *owner;
char *name;
- spinlock_t buf_lock; /* serialize access to 'buf' */
+
+ spinlock_t buf_lock;
char *buf;
size_t bufsize;
- struct mutex read_mutex; /* serialize open/read/close */
+
+ struct mutex read_mutex;
+
int flags;
+ void *data;
+
int (*open)(struct pstore_info *psi);
int (*close)(struct pstore_info *psi);
ssize_t (*read)(u64 *id, enum pstore_type_id *type,
@@ -76,11 +200,10 @@ struct pstore_info {
int (*erase)(enum pstore_type_id type, u64 id,
int count, struct timespec time,
struct pstore_info *psi);
- void *data;
};
+/* Supported frontends */
#define PSTORE_FLAGS_DMESG (1 << 0)
-#define PSTORE_FLAGS_FRAGILE PSTORE_FLAGS_DMESG
#define PSTORE_FLAGS_CONSOLE (1 << 1)
#define PSTORE_FLAGS_FTRACE (1 << 2)
#define PSTORE_FLAGS_PMSG (1 << 3)
--
2.7.4
When built as a module and running with update_ms >= 0, pstore will Oops
during module unload since the work timer is still running. This makes sure
the worker is stopped before unloading.
Signed-off-by: Kees Cook <[email protected]>
Cc: [email protected]
---
fs/pstore/platform.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cfc1abd264d9..074fe85a2078 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -709,6 +709,7 @@ int pstore_register(struct pstore_info *psi)
if (psi->flags & PSTORE_FLAGS_PMSG)
pstore_register_pmsg();
+ /* Start watching for new records, if desired. */
if (pstore_update_ms >= 0) {
pstore_timer.expires = jiffies +
msecs_to_jiffies(pstore_update_ms);
@@ -731,6 +732,11 @@ EXPORT_SYMBOL_GPL(pstore_register);
void pstore_unregister(struct pstore_info *psi)
{
+ /* Stop timer and make sure all work has finished. */
+ pstore_update_ms = -1;
+ del_timer_sync(&pstore_timer);
+ flush_work(&pstore_work);
+
if (psi->flags & PSTORE_FLAGS_PMSG)
pstore_unregister_pmsg();
if (psi->flags & PSTORE_FLAGS_FTRACE)
@@ -830,7 +836,9 @@ static void pstore_timefunc(unsigned long dummy)
schedule_work(&pstore_work);
}
- mod_timer(&pstore_timer, jiffies + msecs_to_jiffies(pstore_update_ms));
+ if (pstore_update_ms >= 0)
+ mod_timer(&pstore_timer,
+ jiffies + msecs_to_jiffies(pstore_update_ms));
}
module_param(backend, charp, 0444);
--
2.7.4
In preparation for handling records off to pstore_mkfile(), allocate the
record instead of reusing stack. This still always frees the record,
though, since pstore_mkfile() isn't yet keeping it.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/platform.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index d897e2f11b6a..072326625629 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -818,8 +818,7 @@ static void decompress_record(struct pstore_record *record)
void pstore_get_records(int quiet)
{
struct pstore_info *psi = psinfo;
- struct pstore_record record = { .psi = psi, };
- int failed = 0, rc;
+ int failed = 0;
if (!psi)
return;
@@ -833,19 +832,34 @@ void pstore_get_records(int quiet)
* may reallocate record.buf. On success, pstore_mkfile() will keep
* the record.buf, so free it only on failure.
*/
- while ((record.size = psi->read(&record)) > 0) {
- decompress_record(&record);
- rc = pstore_mkfile(&record);
+ for (;;) {
+ struct pstore_record *record;
+ int rc;
+
+ record = kzalloc(sizeof(*record), GFP_KERNEL);
+ if (!record) {
+ pr_err("out of memory creating record\n");
+ break;
+ }
+ record->psi = psi;
+
+ record->size = psi->read(record);
+
+ /* No more records left in backend? */
+ if (record->size <= 0)
+ break;
+
+ decompress_record(record);
+ rc = pstore_mkfile(record);
if (rc) {
/* pstore_mkfile() did not take buf, so free it. */
- kfree(record.buf);
+ kfree(record->buf);
if (rc != -EEXIST || !quiet)
failed++;
}
/* Reset for next record. */
- memset(&record, 0, sizeof(record));
- record.psi = psi;
+ kfree(record);
}
if (psi->close)
psi->close(psi);
--
2.7.4
Similar to the pstore_info read() callback, there were too many arguments.
This switches to the new struct pstore_record pointer instead. This adds
"reason" and "part" to the record structure as well.
Signed-off-by: Kees Cook <[email protected]>
---
arch/powerpc/kernel/nvram_64.c | 27 +++++------------
drivers/acpi/apei/erst.c | 18 +++++-------
drivers/firmware/efi/efi-pstore.c | 18 +++++-------
fs/pstore/platform.c | 62 ++++++++++++++++++++++-----------------
include/linux/pstore.h | 36 +++++++++++------------
5 files changed, 76 insertions(+), 85 deletions(-)
diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index 7f192001d09a..caf2e1f36d6b 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -389,51 +389,40 @@ static int nvram_pstore_open(struct pstore_info *psi)
/**
* nvram_pstore_write - pstore write callback for nvram
- * @type: Type of message logged
- * @reason: reason behind dump (oops/panic)
- * @id: identifier to indicate the write performed
- * @part: pstore writes data to registered buffer in parts,
- * part number will indicate the same.
- * @count: Indicates oops count
- * @compressed: Flag to indicate the log is compressed
- * @size: number of bytes written to the registered buffer
- * @psi: registered pstore_info structure
+ * @record: pstore record to write, with @id to be set
*
* Called by pstore_dump() when an oops or panic report is logged in the
* printk buffer.
* Returns 0 on successful write.
*/
-static int nvram_pstore_write(enum pstore_type_id type,
- enum kmsg_dump_reason reason,
- u64 *id, unsigned int part, int count,
- bool compressed, size_t size,
- struct pstore_info *psi)
+static int nvram_pstore_write(struct pstore_record *record)
{
int rc;
unsigned int err_type = ERR_TYPE_KERNEL_PANIC;
struct oops_log_info *oops_hdr = (struct oops_log_info *) oops_buf;
/* part 1 has the recent messages from printk buffer */
- if (part > 1 || (type != PSTORE_TYPE_DMESG))
+ if (record->part > 1 || (record->type != PSTORE_TYPE_DMESG))
return -1;
if (clobbering_unread_rtas_event())
return -1;
oops_hdr->version = cpu_to_be16(OOPS_HDR_VERSION);
- oops_hdr->report_length = cpu_to_be16(size);
+ oops_hdr->report_length = cpu_to_be16(record->size);
oops_hdr->timestamp = cpu_to_be64(ktime_get_real_seconds());
- if (compressed)
+ if (record->compressed)
err_type = ERR_TYPE_KERNEL_PANIC_GZ;
rc = nvram_write_os_partition(&oops_log_partition, oops_buf,
- (int) (sizeof(*oops_hdr) + size), err_type, count);
+ (int) (sizeof(*oops_hdr) + record->size), err_type,
+ record->count);
if (rc != 0)
return rc;
- *id = part;
+ record->id = record->part;
return 0;
}
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index bbefb7522f80..440588d189e7 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -926,9 +926,7 @@ static int erst_check_table(struct acpi_table_erst *erst_tab)
static int erst_open_pstore(struct pstore_info *psi);
static int erst_close_pstore(struct pstore_info *psi);
static ssize_t erst_reader(struct pstore_record *record);
-static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
- u64 *id, unsigned int part, int count, bool compressed,
- size_t size, struct pstore_info *psi);
+static int erst_writer(struct pstore_record *record);
static int erst_clearer(enum pstore_type_id type, u64 id, int count,
struct timespec time, struct pstore_info *psi);
@@ -1054,9 +1052,7 @@ static ssize_t erst_reader(struct pstore_record *record)
return (rc < 0) ? rc : (len - sizeof(*rcd));
}
-static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
- u64 *id, unsigned int part, int count, bool compressed,
- size_t size, struct pstore_info *psi)
+static int erst_writer(struct pstore_record *record)
{
struct cper_pstore_record *rcd = (struct cper_pstore_record *)
(erst_info.buf - sizeof(*rcd));
@@ -1071,21 +1067,21 @@ static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
/* timestamp valid. platform_id, partition_id are invalid */
rcd->hdr.validation_bits = CPER_VALID_TIMESTAMP;
rcd->hdr.timestamp = get_seconds();
- rcd->hdr.record_length = sizeof(*rcd) + size;
+ rcd->hdr.record_length = sizeof(*rcd) + record->size;
rcd->hdr.creator_id = CPER_CREATOR_PSTORE;
rcd->hdr.notification_type = CPER_NOTIFY_MCE;
rcd->hdr.record_id = cper_next_record_id();
rcd->hdr.flags = CPER_HW_ERROR_FLAGS_PREVERR;
rcd->sec_hdr.section_offset = sizeof(*rcd);
- rcd->sec_hdr.section_length = size;
+ rcd->sec_hdr.section_length = record->size;
rcd->sec_hdr.revision = CPER_SEC_REV;
/* fru_id and fru_text is invalid */
rcd->sec_hdr.validation_bits = 0;
rcd->sec_hdr.flags = CPER_SEC_PRIMARY;
- switch (type) {
+ switch (record->type) {
case PSTORE_TYPE_DMESG:
- if (compressed)
+ if (record->compressed)
rcd->sec_hdr.section_type = CPER_SECTION_TYPE_DMESG_Z;
else
rcd->sec_hdr.section_type = CPER_SECTION_TYPE_DMESG;
@@ -1099,7 +1095,7 @@ static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
rcd->sec_hdr.section_severity = CPER_SEV_FATAL;
ret = erst_write(&rcd->hdr);
- *id = rcd->hdr.record_id;
+ record->id = rcd->hdr.record_id;
return ret;
}
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index bda24129e85b..f81e3ec6f1c0 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -240,30 +240,28 @@ static ssize_t efi_pstore_read(struct pstore_record *record)
return size;
}
-static int efi_pstore_write(enum pstore_type_id type,
- enum kmsg_dump_reason reason, u64 *id,
- unsigned int part, int count, bool compressed, size_t size,
- struct pstore_info *psi)
+static int efi_pstore_write(struct pstore_record *record)
{
char name[DUMP_NAME_LEN];
efi_char16_t efi_name[DUMP_NAME_LEN];
efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
int i, ret = 0;
- sprintf(name, "dump-type%u-%u-%d-%lu-%c", type, part, count,
- get_seconds(), compressed ? 'C' : 'D');
+ snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu-%c",
+ record->type, record->part, record->count,
+ get_seconds(), record->compressed ? 'C' : 'D');
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = name[i];
efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
- !pstore_cannot_block_path(reason),
- size, psi->buf);
+ !pstore_cannot_block_path(record->reason),
+ record->size, record->psi->buf);
- if (reason == KMSG_DUMP_OOPS)
+ if (record->reason == KMSG_DUMP_OOPS)
efivar_run_worker();
- *id = part;
+ record->id = record->part;
return ret;
};
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 47968c2f2d0d..879658b4c679 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -484,7 +484,6 @@ static void pstore_dump(struct kmsg_dumper *dumper,
{
unsigned long total = 0;
const char *why;
- u64 id;
unsigned int part = 1;
unsigned long flags = 0;
int is_locked;
@@ -506,48 +505,59 @@ static void pstore_dump(struct kmsg_dumper *dumper,
oopscount++;
while (total < kmsg_bytes) {
char *dst;
- unsigned long size;
- int hsize;
+ size_t dst_size;
+ int header_size;
int zipped_len = -1;
- size_t len;
- bool compressed = false;
- size_t total_len;
+ size_t dump_size;
+ struct pstore_record record = {
+ .type = PSTORE_TYPE_DMESG,
+ .count = oopscount,
+ .reason = reason,
+ .part = part,
+ .compressed = false,
+ .buf = psinfo->buf,
+ .psi = psinfo,
+ };
if (big_oops_buf && is_locked) {
dst = big_oops_buf;
- size = big_oops_buf_sz;
+ dst_size = big_oops_buf_sz;
} else {
dst = psinfo->buf;
- size = psinfo->bufsize;
+ dst_size = psinfo->bufsize;
}
- hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount, part);
- size -= hsize;
+ /* Write dump header. */
+ header_size = snprintf(dst, dst_size, "%s#%d Part%u\n", why,
+ oopscount, part);
+ dst_size -= header_size;
- if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
- size, &len))
+ /* Write dump contents. */
+ if (!kmsg_dump_get_buffer(dumper, true, dst + header_size,
+ dst_size, &dump_size))
break;
if (big_oops_buf && is_locked) {
zipped_len = pstore_compress(dst, psinfo->buf,
- hsize + len, psinfo->bufsize);
+ header_size + dump_size,
+ psinfo->bufsize);
if (zipped_len > 0) {
- compressed = true;
- total_len = zipped_len;
+ record.compressed = true;
+ record.size = zipped_len;
} else {
- total_len = copy_kmsg_to_buffer(hsize, len);
+ record.size = copy_kmsg_to_buffer(header_size,
+ dump_size);
}
} else {
- total_len = hsize + len;
+ record.size = header_size + dump_size;
}
- ret = psinfo->write(PSTORE_TYPE_DMESG, reason, &id, part,
- oopscount, compressed, total_len, psinfo);
+ ret = psinfo->write(&record);
if (ret == 0 && reason == KMSG_DUMP_OOPS && pstore_is_mounted())
pstore_new_entry = 1;
- total += total_len;
+ total += record.size;
part++;
}
if (is_locked)
@@ -618,14 +628,12 @@ static void pstore_register_console(void) {}
static void pstore_unregister_console(void) {}
#endif
-static int pstore_write_compat(enum pstore_type_id type,
- enum kmsg_dump_reason reason,
- u64 *id, unsigned int part, int count,
- bool compressed, size_t size,
- struct pstore_info *psi)
+static int pstore_write_compat(struct pstore_record *record)
{
- return psi->write_buf(type, reason, id, part, psinfo->buf, compressed,
- size, psi);
+ return record->psi->write_buf(record->type, record->reason,
+ &record->id, record->part,
+ psinfo->buf, record->compressed,
+ record->size, record->psi);
}
static int pstore_write_buf_user_compat(enum pstore_type_id type,
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 8b51c9da8d16..c672de41e9b5 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -54,23 +54,32 @@ struct pstore_info;
* @type: pstore record type
* @id: per-type unique identifier for record
* @time: timestamp of the record
- * @count: for PSTORE_TYPE_DMESG, the Oops count.
- * @compressed: for PSTORE_TYPE_DMESG, whether the buffer is compressed
* @buf: pointer to record contents
* @size: size of @buf
* @ecc_notice_size:
* ECC information for @buf
+ *
+ * Valid for PSTORE_TYPE_DMESG @type:
+ *
+ * @count: Oops count since boot
+ * @reason: kdump reason for notification
+ * @part: position in a multipart record
+ * @compressed: whether the buffer is compressed
+ *
*/
struct pstore_record {
struct pstore_info *psi;
enum pstore_type_id type;
u64 id;
struct timespec time;
- int count;
- bool compressed;
char *buf;
ssize_t size;
ssize_t ecc_notice_size;
+
+ int count;
+ enum kmsg_dump_reason reason;
+ unsigned int part;
+ bool compressed;
};
/**
@@ -125,16 +134,10 @@ struct pstore_record {
* data to be stored has already been written to the registered @buf
* of the @psi structure.
*
- * @type: in: pstore record type to write
- * @reason:
- * in: pstore write reason
- * @id: out: unique identifier for the record
- * @part: in: position in a multipart write
- * @count: in: increasing from 0 since boot, the number of this Oops
- * @compressed:
- * in: if the record is compressed
- * @size: in: size of the write
- * @psi: in: pointer to the struct pstore_info for the backend
+ * @record:
+ * pointer to record metadata. Note that @buf is NULL, since
+ * the @buf registered with @psi is what has been written. The
+ * backend is expected to update @id.
*
* Returns 0 on success, and non-zero on error.
*
@@ -203,10 +206,7 @@ struct pstore_info {
int (*open)(struct pstore_info *psi);
int (*close)(struct pstore_info *psi);
ssize_t (*read)(struct pstore_record *record);
- int (*write)(enum pstore_type_id type,
- enum kmsg_dump_reason reason, u64 *id,
- unsigned int part, int count, bool compressed,
- size_t size, struct pstore_info *psi);
+ int (*write)(struct pstore_record *record);
int (*write_buf)(enum pstore_type_id type,
enum kmsg_dump_reason reason, u64 *id,
unsigned int part, const char *buf, bool compressed,
--
2.7.4
Technically, it might be possible for struct pstore_info to go out of
scope after the module_put(), so report the backend name first.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/platform.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 074fe85a2078..d69ef8a840b9 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -722,10 +722,10 @@ int pstore_register(struct pstore_info *psi)
*/
backend = psi->name;
- module_put(owner);
-
pr_info("Registered %s as persistent store backend\n", psi->name);
+ module_put(owner);
+
return 0;
}
EXPORT_SYMBOL_GPL(pstore_register);
--
2.7.4
Now that write() and write_buf() are functionally identical, this removes
write_buf(), and renames write_buf_user() to write_user(). Additionally
adds sanity-checks for pstore_info's declared functions and flags at
registration time.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ftrace.c | 4 ++--
fs/pstore/platform.c | 35 ++++++++++++++++++++---------------
fs/pstore/pmsg.c | 4 ++--
fs/pstore/ram.c | 10 +++++-----
include/linux/pstore.h | 29 ++++++++++-------------------
5 files changed, 39 insertions(+), 43 deletions(-)
diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index a5506ec6995e..06aab07b6bb7 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -53,7 +53,7 @@ static void notrace pstore_ftrace_call(unsigned long ip,
rec.parent_ip = parent_ip;
pstore_ftrace_write_timestamp(&rec, pstore_ftrace_stamp++);
pstore_ftrace_encode_cpu(&rec, raw_smp_processor_id());
- psinfo->write_buf(&record);
+ psinfo->write(&record);
local_irq_restore(flags);
}
@@ -122,7 +122,7 @@ void pstore_register_ftrace(void)
{
struct dentry *file;
- if (!psinfo->write_buf)
+ if (!psinfo->write)
return;
pstore_ftrace_dir = debugfs_create_dir("pstore", NULL);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 1e6642a2063e..e79f170fa79b 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -604,7 +604,7 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
}
record.buf = (char *)s;
record.size = c;
- psinfo->write_buf(&record);
+ psinfo->write(&record);
spin_unlock_irqrestore(&psinfo->buf_lock, flags);
s += c;
c = e - s;
@@ -632,15 +632,8 @@ static void pstore_register_console(void) {}
static void pstore_unregister_console(void) {}
#endif
-static int pstore_write_compat(struct pstore_record *record)
-{
- record->buf = psinfo->buf;
-
- return record->psi->write_buf(record);
-}
-
-static int pstore_write_buf_user_compat(struct pstore_record *record,
- const char __user *buf)
+static int pstore_write_user_compat(struct pstore_record *record,
+ const char __user *buf)
{
unsigned long flags = 0;
size_t i, bufsize, total_size = record->size;
@@ -662,7 +655,7 @@ static int pstore_write_buf_user_compat(struct pstore_record *record,
break;
}
record->size = c;
- ret = record->psi->write_buf(record);
+ ret = record->psi->write(record);
if (unlikely(ret < 0))
break;
i += c;
@@ -687,6 +680,20 @@ int pstore_register(struct pstore_info *psi)
return -EPERM;
}
+ /* Sanity check flags. */
+ if (!psi->flags) {
+ pr_warn("backend '%s' must support at least one frontend\n",
+ psi->name);
+ return -EINVAL;
+ }
+
+ /* Check for required functions. */
+ if (!psi->read || !psi->write) {
+ pr_warn("backend '%s' must implement read() and write()\n",
+ psi->name);
+ return -EINVAL;
+ }
+
spin_lock(&pstore_lock);
if (psinfo) {
pr_warn("backend '%s' already loaded: ignoring '%s'\n",
@@ -695,10 +702,8 @@ int pstore_register(struct pstore_info *psi)
return -EBUSY;
}
- if (!psi->write)
- psi->write = pstore_write_compat;
- if (!psi->write_buf_user)
- psi->write_buf_user = pstore_write_buf_user_compat;
+ if (!psi->write_user)
+ psi->write_user = pstore_write_user_compat;
psinfo = psi;
mutex_init(&psinfo->read_mutex);
spin_unlock(&pstore_lock);
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index ce35907602de..c16a2477e106 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -33,12 +33,12 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
if (!count)
return 0;
- /* check outside lock, page in any data. write_buf_user also checks */
+ /* check outside lock, page in any data. write_user also checks */
if (!access_ok(VERIFY_READ, buf, count))
return -EFAULT;
mutex_lock(&pmsg_lock);
- ret = psinfo->write_buf_user(&record, buf);
+ ret = psinfo->write_user(&record, buf);
mutex_unlock(&pmsg_lock);
return ret ? ret : count;
}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index d85e1adae1b6..5523df7f17ef 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -378,7 +378,7 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz,
return len;
}
-static int notrace ramoops_pstore_write_buf(struct pstore_record *record)
+static int notrace ramoops_pstore_write(struct pstore_record *record)
{
struct ramoops_context *cxt = record->psi->data;
struct persistent_ram_zone *prz;
@@ -451,8 +451,8 @@ static int notrace ramoops_pstore_write_buf(struct pstore_record *record)
return 0;
}
-static int notrace ramoops_pstore_write_buf_user(struct pstore_record *record,
- const char __user *buf)
+static int notrace ramoops_pstore_write_user(struct pstore_record *record,
+ const char __user *buf)
{
if (record->type == PSTORE_TYPE_PMSG) {
struct ramoops_context *cxt = record->psi->data;
@@ -503,8 +503,8 @@ static struct ramoops_context oops_cxt = {
.name = "ramoops",
.open = ramoops_pstore_open,
.read = ramoops_pstore_read,
- .write_buf = ramoops_pstore_write_buf,
- .write_buf_user = ramoops_pstore_write_buf_user,
+ .write = ramoops_pstore_write,
+ .write_user = ramoops_pstore_write_user,
.erase = ramoops_pstore_erase,
},
};
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 7f6eaa71504e..936744221171 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -130,27 +130,19 @@ struct pstore_record {
* available, or negative on error.
*
* @write:
- * Perform a frontend notification of a write to a backend record. The
- * data to be stored has already been written to the registered @buf
- * of the @psi structure.
+ * A newly generated record needs to be written to backend storage.
*
* @record:
- * pointer to record metadata. Note that @buf is NULL, since
- * the @buf registered with @psi is what has been written. The
- * backend is expected to update @id.
+ * pointer to record metadata. When @type is PSTORE_TYPE_DMESG,
+ * @buf will be pointing to the preallocated @psi.buf, since
+ * memory allocation may be broken during an Oops. Regardless,
+ * @buf must be proccesed or copied before returning. The
+ * backend is also expected to write @id with something that
+ 8 can help identify this record to a future @erase callback.
*
* Returns 0 on success, and non-zero on error.
*
- * @write_buf:
- * Perform a frontend write to a backend record. The record contains
- * all metadata and the buffer to write to backend storage. (Unlike
- * @write, this does not use the @psi @buf.)
- *
- * @record: pointer to record metadata.
- *
- * Returns 0 on success, and non-zero on error.
- *
- * @write_buf_user:
+ * @write_user:
* Perform a frontend write to a backend record, using a specified
* buffer that is coming directly from userspace, instead of the
* @record @buf.
@@ -188,9 +180,8 @@ struct pstore_info {
int (*close)(struct pstore_info *psi);
ssize_t (*read)(struct pstore_record *record);
int (*write)(struct pstore_record *record);
- int (*write_buf)(struct pstore_record *record);
- int (*write_buf_user)(struct pstore_record *record,
- const char __user *buf);
+ int (*write_user)(struct pstore_record *record,
+ const char __user *buf);
int (*erase)(struct pstore_record *record);
};
--
2.7.4
Uncommon errors are better to get reported to dmesg so developers can
more easily figure out why pstore is unhappy with a backend attempting
to register.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/platform.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index d69ef8a840b9..320a673ecb5b 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -673,11 +673,15 @@ int pstore_register(struct pstore_info *psi)
{
struct module *owner = psi->owner;
- if (backend && strcmp(backend, psi->name))
+ if (backend && strcmp(backend, psi->name)) {
+ pr_warn("ignoring unexpected backend '%s'\n", psi->name);
return -EPERM;
+ }
spin_lock(&pstore_lock);
if (psinfo) {
+ pr_warn("backend '%s' already loaded: ignoring '%s'\n",
+ psinfo->name, psi->name);
spin_unlock(&pstore_lock);
return -EBUSY;
}
--
2.7.4
Instead of the long list of arguments, just pass the new record struct.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/inode.c | 57 +++++++++++++++++++++++++++++-----------------------
fs/pstore/internal.h | 5 +----
fs/pstore/platform.c | 6 +-----
3 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 57c0646479f5..a98787bab3e6 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -302,9 +302,7 @@ bool pstore_is_mounted(void)
* Load it up with "size" bytes of data from "buf".
* Set the mtime & ctime to the date that this record was originally stored.
*/
-int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
- char *data, bool compressed, size_t size,
- struct timespec time, struct pstore_info *psi)
+int pstore_mkfile(struct pstore_record *record)
{
struct dentry *root = pstore_sb->s_root;
struct dentry *dentry;
@@ -313,12 +311,13 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
char name[PSTORE_NAMELEN];
struct pstore_private *private, *pos;
unsigned long flags;
+ size_t size = record->size + record->ecc_notice_size;
spin_lock_irqsave(&allpstore_lock, flags);
list_for_each_entry(pos, &allpstore, list) {
- if (pos->type == type &&
- pos->id == id &&
- pos->psi == psi) {
+ if (pos->type == record->type &&
+ pos->id == record->id &&
+ pos->psi == record->psi) {
rc = -EEXIST;
break;
}
@@ -336,48 +335,56 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
private = kmalloc(sizeof *private + size, GFP_KERNEL);
if (!private)
goto fail_alloc;
- private->type = type;
- private->id = id;
- private->count = count;
- private->psi = psi;
+ private->type = record->type;
+ private->id = record->id;
+ private->count = record->count;
+ private->psi = record->psi;
- switch (type) {
+ switch (record->type) {
case PSTORE_TYPE_DMESG:
scnprintf(name, sizeof(name), "dmesg-%s-%lld%s",
- psname, id, compressed ? ".enc.z" : "");
+ record->psi->name, record->id,
+ record->compressed ? ".enc.z" : "");
break;
case PSTORE_TYPE_CONSOLE:
- scnprintf(name, sizeof(name), "console-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "console-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_FTRACE:
- scnprintf(name, sizeof(name), "ftrace-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "ftrace-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_MCE:
- scnprintf(name, sizeof(name), "mce-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "mce-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_RTAS:
- scnprintf(name, sizeof(name), "rtas-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "rtas-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_OF:
scnprintf(name, sizeof(name), "powerpc-ofw-%s-%lld",
- psname, id);
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_COMMON:
scnprintf(name, sizeof(name), "powerpc-common-%s-%lld",
- psname, id);
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PMSG:
- scnprintf(name, sizeof(name), "pmsg-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "pmsg-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_OPAL:
- sprintf(name, "powerpc-opal-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "powerpc-opal-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_UNKNOWN:
- scnprintf(name, sizeof(name), "unknown-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "unknown-%s-%lld",
+ record->psi->name, record->id);
break;
default:
scnprintf(name, sizeof(name), "type%d-%s-%lld",
- type, psname, id);
+ record->type, record->psi->name, record->id);
break;
}
@@ -387,13 +394,13 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
if (!dentry)
goto fail_lockedalloc;
- memcpy(private->data, data, size);
+ memcpy(private->data, record->buf, size);
inode->i_size = private->size = size;
inode->i_private = private;
- if (time.tv_sec)
- inode->i_mtime = inode->i_ctime = time;
+ if (record->time.tv_sec)
+ inode->i_mtime = inode->i_ctime = record->time;
d_add(dentry, inode);
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index da416e6591c9..af1df5a36d86 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -25,10 +25,7 @@ extern struct pstore_info *psinfo;
extern void pstore_set_kmsg_bytes(int);
extern void pstore_get_records(int);
-extern int pstore_mkfile(enum pstore_type_id, char *psname, u64 id,
- int count, char *data, bool compressed,
- size_t size, struct timespec time,
- struct pstore_info *psi);
+extern int pstore_mkfile(struct pstore_record *record);
extern bool pstore_is_mounted(void);
#endif
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 0503380704de..168e03fd5e58 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -814,11 +814,7 @@ void pstore_get_records(int quiet)
record.psi)) > 0) {
decompress_record(&record);
- rc = pstore_mkfile(record.type, psi->name, record.id,
- record.count, record.buf,
- record.compressed,
- record.size + record.ecc_notice_size,
- record.time, record.psi);
+ rc = pstore_mkfile(&record);
/* Free buffer other than big oops */
if (record.buf != big_oops_buf)
--
2.7.4
Currently, pstore_mkfile() performs a memcpy() of the record contents,
so it can live anywhere. However, this is needlessly wasteful. In
preparation of pstore_mkfile() keeping the record contents, always
allocate a buffer for the contents.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/platform.c | 38 ++++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 879658b4c679..c0d401e732e6 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -768,6 +768,7 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
static void decompress_record(struct pstore_record *record)
{
int unzipped_len;
+ char *decompressed;
/* Only PSTORE_TYPE_DMESG support compression. */
if (!record->compressed || record->type != PSTORE_TYPE_DMESG) {
@@ -783,17 +784,29 @@ static void decompress_record(struct pstore_record *record)
unzipped_len = pstore_decompress(record->buf, big_oops_buf,
record->size, big_oops_buf_sz);
- if (unzipped_len > 0) {
- if (record->ecc_notice_size)
- memcpy(big_oops_buf + unzipped_len,
- record->buf + record->size,
- record->ecc_notice_size);
- kfree(record->buf);
- record->buf = big_oops_buf;
- record->size = unzipped_len;
- record->compressed = false;
- } else
+ if (unzipped_len <= 0) {
pr_err("decompression failed: %d\n", unzipped_len);
+ return;
+ }
+
+ /* Build new buffer for decompressed contents. */
+ decompressed = kmalloc(unzipped_len + record->ecc_notice_size,
+ GFP_KERNEL);
+ if (!decompressed) {
+ pr_err("decompression ran out of memory\n");
+ return;
+ }
+ memcpy(decompressed, big_oops_buf, unzipped_len);
+
+ /* Append ECC notice to decompressed buffer. */
+ memcpy(decompressed + unzipped_len, record->buf + record->size,
+ record->ecc_notice_size);
+
+ /* Swap out compresed contents with decompressed contents. */
+ kfree(record->buf);
+ record->buf = decompressed;
+ record->size = unzipped_len;
+ record->compressed = false;
}
/*
@@ -819,13 +832,10 @@ void pstore_get_records(int quiet)
decompress_record(&record);
rc = pstore_mkfile(&record);
- /* Free buffer other than big oops */
- if (record.buf != big_oops_buf)
- kfree(record.buf);
-
if (rc && (rc != -EEXIST || !quiet))
failed++;
+ kfree(record.buf);
memset(&record, 0, sizeof(record));
record.psi = psi;
}
--
2.7.4
This moves the record decompression logic out to a separate function
to avoid the deep indentation.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/platform.c | 67 +++++++++++++++++++++++++++++-----------------------
1 file changed, 37 insertions(+), 30 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 3fa1575a6e36..0503380704de 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -757,6 +757,37 @@ void pstore_unregister(struct pstore_info *psi)
}
EXPORT_SYMBOL_GPL(pstore_unregister);
+static void decompress_record(struct pstore_record *record)
+{
+ int unzipped_len;
+
+ /* Only PSTORE_TYPE_DMESG support compression. */
+ if (!record->compressed || record->type != PSTORE_TYPE_DMESG) {
+ pr_warn("ignored compressed record type %d\n", record->type);
+ return;
+ }
+
+ /* No compression method has created the common buffer. */
+ if (!big_oops_buf) {
+ pr_warn("no decompression buffer allocated\n");
+ return;
+ }
+
+ unzipped_len = pstore_decompress(record->buf, big_oops_buf,
+ record->size, big_oops_buf_sz);
+ if (unzipped_len > 0) {
+ if (record->ecc_notice_size)
+ memcpy(big_oops_buf + unzipped_len,
+ record->buf + record->size,
+ record->ecc_notice_size);
+ kfree(record->buf);
+ record->buf = big_oops_buf;
+ record->size = unzipped_len;
+ record->compressed = false;
+ } else
+ pr_err("decompression failed: %d\n", unzipped_len);
+}
+
/*
* Read all the records from the persistent store. Create
* files in our filesystem. Don't warn about -EEXIST errors
@@ -768,7 +799,6 @@ void pstore_get_records(int quiet)
struct pstore_info *psi = psinfo;
struct pstore_record record = { .psi = psi, };
int failed = 0, rc;
- int unzipped_len = -1;
if (!psi)
return;
@@ -782,41 +812,18 @@ void pstore_get_records(int quiet)
&record.buf, &record.compressed,
&record.ecc_notice_size,
record.psi)) > 0) {
- if (record.compressed &&
- record.type == PSTORE_TYPE_DMESG) {
- if (big_oops_buf)
- unzipped_len = pstore_decompress(
- record.buf,
- big_oops_buf,
- record.size,
- big_oops_buf_sz);
-
- if (unzipped_len > 0) {
- if (record.ecc_notice_size)
- memcpy(big_oops_buf + unzipped_len,
- record.buf + recorrecord.size,
- record.ecc_notice_size);
- kfree(record.buf);
- record.buf = big_oops_buf;
- record.size = unzipped_len;
- record.compressed = false;
- } else {
- pr_err("decompression failed;returned %d\n",
- unzipped_len);
- record.compressed = true;
- }
- }
+
+ decompress_record(&record);
rc = pstore_mkfile(record.type, psi->name, record.id,
record.count, record.buf,
record.compressed,
record.size + record.ecc_notice_size,
record.time, record.psi);
- if (unzipped_len < 0) {
- /* Free buffer other than big oops */
+
+ /* Free buffer other than big oops */
+ if (record.buf != big_oops_buf)
kfree(record.buf);
- record.buf = NULL;
- } else
- unzipped_len = -1;
+
if (rc && (rc != -EEXIST || !quiet))
failed++;
--
2.7.4
This removes the argument list for the erase() callback and replaces it
with a pointer to the backend record details to be removed.
Signed-off-by: Kees Cook <[email protected]>
---
drivers/acpi/apei/erst.c | 8 +++-----
drivers/firmware/efi/efi-pstore.c | 26 +++++++++++---------------
fs/pstore/inode.c | 12 +++++-------
fs/pstore/ram.c | 15 +++++++--------
include/linux/pstore.h | 16 +++++-----------
5 files changed, 31 insertions(+), 46 deletions(-)
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 440588d189e7..7207e5fc9d3d 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -927,8 +927,7 @@ static int erst_open_pstore(struct pstore_info *psi);
static int erst_close_pstore(struct pstore_info *psi);
static ssize_t erst_reader(struct pstore_record *record);
static int erst_writer(struct pstore_record *record);
-static int erst_clearer(enum pstore_type_id type, u64 id, int count,
- struct timespec time, struct pstore_info *psi);
+static int erst_clearer(struct pstore_record *record);
static struct pstore_info erst_info = {
.owner = THIS_MODULE,
@@ -1100,10 +1099,9 @@ static int erst_writer(struct pstore_record *record)
return ret;
}
-static int erst_clearer(enum pstore_type_id type, u64 id, int count,
- struct timespec time, struct pstore_info *psi)
+static int erst_clearer(struct pstore_record *record)
{
- return erst_clear(id);
+ return erst_clear(record->id);
}
static int __init erst_init(void)
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index f81e3ec6f1c0..93d8cdbe7ef4 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -266,10 +266,7 @@ static int efi_pstore_write(struct pstore_record *record)
};
struct pstore_erase_data {
- u64 id;
- enum pstore_type_id type;
- int count;
- struct timespec time;
+ struct pstore_record *record;
efi_char16_t *name;
};
@@ -295,8 +292,9 @@ static int efi_pstore_erase_func(struct efivar_entry *entry, void *data)
* Check if an old format, which doesn't support
* holding multiple logs, remains.
*/
- sprintf(name_old, "dump-type%u-%u-%lu", ed->type,
- (unsigned int)ed->id, ed->time.tv_sec);
+ snprintf(name_old, sizeof(name_old), "dump-type%u-%u-%lu",
+ ed->record->type, (unsigned int)ed->record->id,
+ ed->record->time.tv_sec);
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name_old[i] = name_old[i];
@@ -321,8 +319,7 @@ static int efi_pstore_erase_func(struct efivar_entry *entry, void *data)
return 1;
}
-static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
- struct timespec time, struct pstore_info *psi)
+static int efi_pstore_erase(struct pstore_record *record)
{
struct pstore_erase_data edata;
struct efivar_entry *entry = NULL;
@@ -331,17 +328,16 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
int found, i;
unsigned int part;
- do_div(id, 1000);
- part = do_div(id, 100);
- sprintf(name, "dump-type%u-%u-%d-%lu", type, part, count, time.tv_sec);
+ do_div(record->id, 1000);
+ part = do_div(record->id, 100);
+ snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu",
+ record->type, record->part, record->count,
+ record->time.tv_sec);
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = name[i];
- edata.id = part;
- edata.type = type;
- edata.count = count;
- edata.time = time;
+ edata.record = record;
edata.name = efi_name;
if (efivar_entry_iter_begin())
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 0ea281b457fa..06504b69575b 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -210,14 +210,12 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
if (err)
return err;
- if (record->psi->erase) {
- mutex_lock(&record->psi->read_mutex);
- record->psi->erase(record->type, record->id, record->count,
- d_inode(dentry)->i_ctime, record->psi);
- mutex_unlock(&record->psi->read_mutex);
- } else {
+ if (!record->psi->erase)
return -EPERM;
- }
+
+ mutex_lock(&record->psi->read_mutex);
+ record->psi->erase(record);
+ mutex_unlock(&record->psi->read_mutex);
return simple_unlink(dir, dentry);
}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6e2a814e37..a18575fe32e9 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -469,25 +469,24 @@ static int notrace ramoops_pstore_write_buf_user(enum pstore_type_id type,
return -EINVAL;
}
-static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
- struct timespec time, struct pstore_info *psi)
+static int ramoops_pstore_erase(struct pstore_record *record)
{
- struct ramoops_context *cxt = psi->data;
+ struct ramoops_context *cxt = record->psi->data;
struct persistent_ram_zone *prz;
- switch (type) {
+ switch (record->type) {
case PSTORE_TYPE_DMESG:
- if (id >= cxt->max_dump_cnt)
+ if (record->id >= cxt->max_dump_cnt)
return -EINVAL;
- prz = cxt->dprzs[id];
+ prz = cxt->dprzs[record->id];
break;
case PSTORE_TYPE_CONSOLE:
prz = cxt->cprz;
break;
case PSTORE_TYPE_FTRACE:
- if (id >= cxt->max_ftrace_cnt)
+ if (record->id >= cxt->max_ftrace_cnt)
return -EINVAL;
- prz = cxt->fprzs[id];
+ prz = cxt->fprzs[record->id];
break;
case PSTORE_TYPE_PMSG:
prz = cxt->mprz;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index c672de41e9b5..a4b2c21a512e 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -177,15 +177,11 @@ struct pstore_record {
*
* @erase:
* Delete a record from backend storage. Different backends
- * identify records differently, so all possible methods of
- * identification are included to help the backend locate the
- * record to remove.
+ * identify records differently, so entire original record is
+ * passed back to assist in identification of what the backend
+ * should remove from storage.
*
- * @type: in: pstore record type to write
- * @id: in: per-type unique identifier for the record
- * @count: in: Oops count
- * @time: in: timestamp for the record
- * @psi: in: pointer to the struct pstore_info for the backend
+ * @record: pointer to record metadata.
*
* Returns 0 on success, and non-zero on error.
*
@@ -215,9 +211,7 @@ struct pstore_info {
enum kmsg_dump_reason reason, u64 *id,
unsigned int part, const char __user *buf,
bool compressed, size_t size, struct pstore_info *psi);
- int (*erase)(enum pstore_type_id type, u64 id,
- int count, struct timespec time,
- struct pstore_info *psi);
+ int (*erase)(struct pstore_record *record);
};
/* Supported frontends */
--
2.7.4
As with the other API updates, this removes the long argument list in favor
of passing a single pstore recaord.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/ftrace.c | 9 +++++++--
fs/pstore/platform.c | 30 +++++++++++++++++++++---------
fs/pstore/ram.c | 44 ++++++++++++++++++++++----------------------
include/linux/pstore.h | 21 +++++----------------
4 files changed, 55 insertions(+), 49 deletions(-)
diff --git a/fs/pstore/ftrace.c b/fs/pstore/ftrace.c
index 899d0ba0bd6c..a5506ec6995e 100644
--- a/fs/pstore/ftrace.c
+++ b/fs/pstore/ftrace.c
@@ -37,6 +37,12 @@ static void notrace pstore_ftrace_call(unsigned long ip,
{
unsigned long flags;
struct pstore_ftrace_record rec = {};
+ struct pstore_record record = {
+ .type = PSTORE_TYPE_FTRACE,
+ .buf = (char *)&rec,
+ .size = sizeof(rec),
+ .psi = psinfo,
+ };
if (unlikely(oops_in_progress))
return;
@@ -47,8 +53,7 @@ static void notrace pstore_ftrace_call(unsigned long ip,
rec.parent_ip = parent_ip;
pstore_ftrace_write_timestamp(&rec, pstore_ftrace_stamp++);
pstore_ftrace_encode_cpu(&rec, raw_smp_processor_id());
- psinfo->write_buf(PSTORE_TYPE_FTRACE, 0, NULL, 0, (void *)&rec,
- 0, sizeof(rec), psinfo);
+ psinfo->write_buf(&record);
local_irq_restore(flags);
}
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index aa3d6e572ede..5eecf9012459 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -587,8 +587,11 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
const char *e = s + c;
while (s < e) {
+ struct pstore_record record = {
+ .type = PSTORE_TYPE_CONSOLE,
+ .psi = psinfo,
+ };
unsigned long flags;
- u64 id;
if (c > psinfo->bufsize)
c = psinfo->bufsize;
@@ -599,8 +602,9 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
} else {
spin_lock_irqsave(&psinfo->buf_lock, flags);
}
- psinfo->write_buf(PSTORE_TYPE_CONSOLE, 0, &id, 0,
- s, 0, c, psinfo);
+ record.buf = (char *)s;
+ record.size = c;
+ psinfo->write_buf(&record);
spin_unlock_irqrestore(&psinfo->buf_lock, flags);
s += c;
c = e - s;
@@ -630,10 +634,9 @@ static void pstore_unregister_console(void) {}
static int pstore_write_compat(struct pstore_record *record)
{
- return record->psi->write_buf(record->type, record->reason,
- &record->id, record->part,
- psinfo->buf, record->compressed,
- record->size, record->psi);
+ record->buf = psinfo->buf;
+
+ return record->psi->write_buf(record);
}
static int pstore_write_buf_user_compat(enum pstore_type_id type,
@@ -653,6 +656,15 @@ static int pstore_write_buf_user_compat(enum pstore_type_id type,
bufsize = psinfo->bufsize;
spin_lock_irqsave(&psinfo->buf_lock, flags);
for (i = 0; i < size; ) {
+ struct pstore_record record = {
+ .type = type,
+ .reason = reason,
+ .id = id,
+ .part = part,
+ .buf = psinfo->buf,
+ .compressed = compressed,
+ .psi = psi,
+ };
size_t c = min(size - i, bufsize);
ret = __copy_from_user(psinfo->buf, buf + i, c);
@@ -660,8 +672,8 @@ static int pstore_write_buf_user_compat(enum pstore_type_id type,
ret = -EFAULT;
break;
}
- ret = psi->write_buf(type, reason, id, part, psinfo->buf,
- compressed, c, psi);
+ record.size = c;
+ ret = psi->write_buf(&record);
if (unlikely(ret < 0))
break;
i += c;
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index a18575fe32e9..a7cdde60b1f9 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -378,23 +378,18 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz,
return len;
}
-static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
- enum kmsg_dump_reason reason,
- u64 *id, unsigned int part,
- const char *buf,
- bool compressed, size_t size,
- struct pstore_info *psi)
+static int notrace ramoops_pstore_write_buf(struct pstore_record *record)
{
- struct ramoops_context *cxt = psi->data;
+ struct ramoops_context *cxt = record->psi->data;
struct persistent_ram_zone *prz;
- size_t hlen;
+ size_t size, hlen;
- if (type == PSTORE_TYPE_CONSOLE) {
+ if (record->type == PSTORE_TYPE_CONSOLE) {
if (!cxt->cprz)
return -ENOMEM;
- persistent_ram_write(cxt->cprz, buf, size);
+ persistent_ram_write(cxt->cprz, record->buf, record->size);
return 0;
- } else if (type == PSTORE_TYPE_FTRACE) {
+ } else if (record->type == PSTORE_TYPE_FTRACE) {
int zonenum;
if (!cxt->fprzs)
@@ -407,33 +402,36 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
else
zonenum = 0;
- persistent_ram_write(cxt->fprzs[zonenum], buf, size);
+ persistent_ram_write(cxt->fprzs[zonenum], record->buf,
+ record->size);
return 0;
- } else if (type == PSTORE_TYPE_PMSG) {
+ } else if (record->type == PSTORE_TYPE_PMSG) {
pr_warn_ratelimited("PMSG shouldn't call %s\n", __func__);
return -EINVAL;
}
- if (type != PSTORE_TYPE_DMESG)
+ if (record->type != PSTORE_TYPE_DMESG)
return -EINVAL;
- /* Out of the various dmesg dump types, ramoops is currently designed
+ /*
+ * Out of the various dmesg dump types, ramoops is currently designed
* to only store crash logs, rather than storing general kernel logs.
*/
- if (reason != KMSG_DUMP_OOPS &&
- reason != KMSG_DUMP_PANIC)
+ if (record->reason != KMSG_DUMP_OOPS &&
+ record->reason != KMSG_DUMP_PANIC)
return -EINVAL;
/* Skip Oopes when configured to do so. */
- if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
+ if (record->reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
return -EINVAL;
- /* Explicitly only take the first part of any new crash.
+ /*
+ * 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)
+ if (record->part != 1)
return -ENOSPC;
if (!cxt->dprzs)
@@ -441,10 +439,12 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
prz = cxt->dprzs[cxt->dump_write_cnt];
- hlen = ramoops_write_kmsg_hdr(prz, compressed);
+ /* Build header and append record contents. */
+ hlen = ramoops_write_kmsg_hdr(prz, record->compressed);
+ size = record->size;
if (size + hlen > prz->buffer_size)
size = prz->buffer_size - hlen;
- persistent_ram_write(prz, buf, size);
+ persistent_ram_write(prz, record->buf, size);
cxt->dump_write_cnt = (cxt->dump_write_cnt + 1) % cxt->max_dump_cnt;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index a4b2c21a512e..351a22242518 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -142,19 +142,11 @@ struct pstore_record {
* Returns 0 on success, and non-zero on error.
*
* @write_buf:
- * Perform a frontend write to a backend record, using a specified
- * buffer. Unlike @write, this does not use the @psi @buf.
+ * Perform a frontend write to a backend record. The record contains
+ * all metadata and the buffer to write to backend storage. (Unlike
+ * @write, this does not use the @psi @buf.)
*
- * @type: in: pstore record type to write
- * @reason:
- * in: pstore write reason
- * @id: out: unique identifier for the record
- * @part: in: position in a multipart write
- * @buf: in: pointer to contents to write to backend record
- * @compressed:
- * in: if the record is compressed
- * @size: in: size of the write
- * @psi: in: pointer to the struct pstore_info for the backend
+ * @record: pointer to record metadata.
*
* Returns 0 on success, and non-zero on error.
*
@@ -203,10 +195,7 @@ struct pstore_info {
int (*close)(struct pstore_info *psi);
ssize_t (*read)(struct pstore_record *record);
int (*write)(struct pstore_record *record);
- int (*write_buf)(enum pstore_type_id type,
- enum kmsg_dump_reason reason, u64 *id,
- unsigned int part, const char *buf, bool compressed,
- size_t size, struct pstore_info *psi);
+ int (*write_buf)(struct pstore_record *record);
int (*write_buf_user)(enum pstore_type_id type,
enum kmsg_dump_reason reason, u64 *id,
unsigned int part, const char __user *buf,
--
2.7.4
This switches the inode-private data from carrying duplicate metadata to
keeping the record passed in during pstore_mkfile().
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/inode.c | 57 ++++++++++++++++++++++++++--------------------------
fs/pstore/platform.c | 6 ++----
2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 3d1f047e4f41..0ea281b457fa 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -47,12 +47,8 @@ static LIST_HEAD(allpstore);
struct pstore_private {
struct list_head list;
- struct pstore_info *psi;
- enum pstore_type_id type;
- u64 id;
- int count;
- ssize_t size;
- char *buf;
+ struct pstore_record *record;
+ size_t total_size;
};
struct pstore_ftrace_seq_data {
@@ -67,7 +63,10 @@ static void free_pstore_private(struct pstore_private *private)
{
if (!private)
return;
- kfree(private->buf);
+ if (private->record) {
+ kfree(private->record->buf);
+ kfree(private->record);
+ }
kfree(private);
}
@@ -80,9 +79,9 @@ static void *pstore_ftrace_seq_start(struct seq_file *s, loff_t *pos)
if (!data)
return NULL;
- data->off = ps->size % REC_SIZE;
+ data->off = ps->total_size % REC_SIZE;
data->off += *pos * REC_SIZE;
- if (data->off + REC_SIZE > ps->size) {
+ if (data->off + REC_SIZE > ps->total_size) {
kfree(data);
return NULL;
}
@@ -102,7 +101,7 @@ static void *pstore_ftrace_seq_next(struct seq_file *s, void *v, loff_t *pos)
struct pstore_ftrace_seq_data *data = v;
data->off += REC_SIZE;
- if (data->off + REC_SIZE > ps->size)
+ if (data->off + REC_SIZE > ps->total_size)
return NULL;
(*pos)++;
@@ -113,7 +112,9 @@ static int pstore_ftrace_seq_show(struct seq_file *s, void *v)
{
struct pstore_private *ps = s->private;
struct pstore_ftrace_seq_data *data = v;
- struct pstore_ftrace_record *rec = (void *)(ps->buf + data->off);
+ struct pstore_ftrace_record *rec;
+
+ rec = (struct pstore_ftrace_record *)(ps->record->buf + data->off);
seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %pf <- %pF\n",
pstore_ftrace_decode_cpu(rec),
@@ -133,7 +134,7 @@ static const struct seq_operations pstore_ftrace_seq_ops = {
static int pstore_check_syslog_permissions(struct pstore_private *ps)
{
- switch (ps->type) {
+ switch (ps->record->type) {
case PSTORE_TYPE_DMESG:
case PSTORE_TYPE_CONSOLE:
return check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
@@ -149,9 +150,10 @@ static ssize_t pstore_file_read(struct file *file, char __user *userbuf,
struct seq_file *sf = file->private_data;
struct pstore_private *ps = sf->private;
- if (ps->type == PSTORE_TYPE_FTRACE)
+ if (ps->record->type == PSTORE_TYPE_FTRACE)
return seq_read(file, userbuf, count, ppos);
- return simple_read_from_buffer(userbuf, count, ppos, ps->buf, ps->size);
+ return simple_read_from_buffer(userbuf, count, ppos,
+ ps->record->buf, ps->total_size);
}
static int pstore_file_open(struct inode *inode, struct file *file)
@@ -165,7 +167,7 @@ static int pstore_file_open(struct inode *inode, struct file *file)
if (err)
return err;
- if (ps->type == PSTORE_TYPE_FTRACE)
+ if (ps->record->type == PSTORE_TYPE_FTRACE)
sops = &pstore_ftrace_seq_ops;
err = seq_open(file, sops);
@@ -201,17 +203,18 @@ static const struct file_operations pstore_file_operations = {
static int pstore_unlink(struct inode *dir, struct dentry *dentry)
{
struct pstore_private *p = d_inode(dentry)->i_private;
+ struct pstore_record *record = p->record;
int err;
err = pstore_check_syslog_permissions(p);
if (err)
return err;
- if (p->psi->erase) {
- mutex_lock(&p->psi->read_mutex);
- p->psi->erase(p->type, p->id, p->count,
- d_inode(dentry)->i_ctime, p->psi);
- mutex_unlock(&p->psi->read_mutex);
+ if (record->psi->erase) {
+ mutex_lock(&record->psi->read_mutex);
+ record->psi->erase(record->type, record->id, record->count,
+ d_inode(dentry)->i_ctime, record->psi);
+ mutex_unlock(&record->psi->read_mutex);
} else {
return -EPERM;
}
@@ -323,9 +326,9 @@ int pstore_mkfile(struct pstore_record *record)
spin_lock_irqsave(&allpstore_lock, flags);
list_for_each_entry(pos, &allpstore, list) {
- if (pos->type == record->type &&
- pos->id == record->id &&
- pos->psi == record->psi) {
+ if (pos->record->type == record->type &&
+ pos->record->id == record->id &&
+ pos->record->psi == record->psi) {
rc = -EEXIST;
break;
}
@@ -343,10 +346,7 @@ int pstore_mkfile(struct pstore_record *record)
private = kzalloc(sizeof(*private), GFP_KERNEL);
if (!private)
goto fail_alloc;
- private->type = record->type;
- private->id = record->id;
- private->count = record->count;
- private->psi = record->psi;
+ private->record = record;
switch (record->type) {
case PSTORE_TYPE_DMESG:
@@ -402,8 +402,7 @@ int pstore_mkfile(struct pstore_record *record)
if (!dentry)
goto fail_lockedalloc;
- private->buf = record->buf;
- inode->i_size = private->size = size;
+ inode->i_size = private->total_size = size;
inode->i_private = private;
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 072326625629..aa3d6e572ede 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -852,14 +852,12 @@ void pstore_get_records(int quiet)
decompress_record(record);
rc = pstore_mkfile(record);
if (rc) {
- /* pstore_mkfile() did not take buf, so free it. */
+ /* pstore_mkfile() did not take record, so free it. */
kfree(record->buf);
+ kfree(record);
if (rc != -EEXIST || !quiet)
failed++;
}
-
- /* Reset for next record. */
- kfree(record);
}
if (psi->close)
psi->close(psi);
--
2.7.4
The per-prz spinlock should be using the dynamic initializer so that
lockdep can correctly track it. Without this, under lockdep, we get a
warning at boot that the lock is in non-static memory.
Fixes: 109704492ef6 ("pstore: Make spinlock per zone instead of global")
Fixes: 76d5692a5803 ("pstore: Correctly initialize spinlock and flags")
Signed-off-by: Kees Cook <[email protected]>
Cc: [email protected]
---
fs/pstore/ram_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index bc927e30bdcc..e11672aa4575 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -532,7 +532,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
}
/* Initialize general buffer state. */
- prz->buffer_lock = __RAW_SPIN_LOCK_UNLOCKED(buffer_lock);
+ raw_spin_lock_init(&prz->buffer_lock);
prz->flags = flags;
ret = persistent_ram_buffer_map(start, size, prz, memtype);
--
2.7.4
Removes argument list in favor of pstore record, though the user buffer
remains passed separately since it must carry the __user annotation.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/platform.c | 35 ++++++++++++-----------------------
fs/pstore/pmsg.c | 9 ++++++---
fs/pstore/ram.c | 14 +++++---------
include/linux/pstore.h | 23 +++++++----------------
4 files changed, 30 insertions(+), 51 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 5eecf9012459..1e6642a2063e 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -639,47 +639,36 @@ static int pstore_write_compat(struct pstore_record *record)
return record->psi->write_buf(record);
}
-static int pstore_write_buf_user_compat(enum pstore_type_id type,
- enum kmsg_dump_reason reason,
- u64 *id, unsigned int part,
- const char __user *buf,
- bool compressed, size_t size,
- struct pstore_info *psi)
+static int pstore_write_buf_user_compat(struct pstore_record *record,
+ const char __user *buf)
{
unsigned long flags = 0;
- size_t i, bufsize = size;
+ size_t i, bufsize, total_size = record->size;
long ret = 0;
- if (unlikely(!access_ok(VERIFY_READ, buf, size)))
+ if (unlikely(!access_ok(VERIFY_READ, buf, total_size)))
return -EFAULT;
+ bufsize = total_size;
if (bufsize > psinfo->bufsize)
bufsize = psinfo->bufsize;
+ record->buf = psinfo->buf;
spin_lock_irqsave(&psinfo->buf_lock, flags);
- for (i = 0; i < size; ) {
- struct pstore_record record = {
- .type = type,
- .reason = reason,
- .id = id,
- .part = part,
- .buf = psinfo->buf,
- .compressed = compressed,
- .psi = psi,
- };
- size_t c = min(size - i, bufsize);
+ for (i = 0; i < total_size; ) {
+ size_t c = min(total_size - i, bufsize);
- ret = __copy_from_user(psinfo->buf, buf + i, c);
+ ret = __copy_from_user(record->buf, buf + i, c);
if (unlikely(ret != 0)) {
ret = -EFAULT;
break;
}
- record.size = c;
- ret = psi->write_buf(&record);
+ record->size = c;
+ ret = record->psi->write_buf(record);
if (unlikely(ret < 0))
break;
i += c;
}
spin_unlock_irqrestore(&psinfo->buf_lock, flags);
- return unlikely(ret < 0) ? ret : size;
+ return unlikely(ret < 0) ? ret : total_size;
}
/*
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 78f6176c020f..ce35907602de 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -23,7 +23,11 @@ static DEFINE_MUTEX(pmsg_lock);
static ssize_t write_pmsg(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
- u64 id;
+ struct pstore_record record = {
+ .type = PSTORE_TYPE_PMSG,
+ .size = count,
+ .psi = psinfo,
+ };
int ret;
if (!count)
@@ -34,8 +38,7 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
return -EFAULT;
mutex_lock(&pmsg_lock);
- ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, &id, 0, buf, 0, count,
- psinfo);
+ ret = psinfo->write_buf_user(&record, buf);
mutex_unlock(&pmsg_lock);
return ret ? ret : count;
}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index a7cdde60b1f9..d85e1adae1b6 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -451,19 +451,15 @@ static int notrace ramoops_pstore_write_buf(struct pstore_record *record)
return 0;
}
-static int notrace ramoops_pstore_write_buf_user(enum pstore_type_id type,
- enum kmsg_dump_reason reason,
- u64 *id, unsigned int part,
- const char __user *buf,
- bool compressed, size_t size,
- struct pstore_info *psi)
+static int notrace ramoops_pstore_write_buf_user(struct pstore_record *record,
+ const char __user *buf)
{
- if (type == PSTORE_TYPE_PMSG) {
- struct ramoops_context *cxt = psi->data;
+ if (record->type == PSTORE_TYPE_PMSG) {
+ struct ramoops_context *cxt = record->psi->data;
if (!cxt->mprz)
return -ENOMEM;
- return persistent_ram_write_user(cxt->mprz, buf, size);
+ return persistent_ram_write_user(cxt->mprz, buf, record->size);
}
return -EINVAL;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 351a22242518..7f6eaa71504e 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -152,18 +152,11 @@ struct pstore_record {
*
* @write_buf_user:
* Perform a frontend write to a backend record, using a specified
- * buffer that is coming directly from userspace.
- *
- * @type: in: pstore record type to write
- * @reason:
- * in: pstore write reason
- * @id: out: unique identifier for the record
- * @part: in: position in a multipart write
- * @buf: in: pointer to userspace contents to write to backend record
- * @compressed:
- * in: if the record is compressed
- * @size: in: size of the write
- * @psi: in: pointer to the struct pstore_info for the backend
+ * buffer that is coming directly from userspace, instead of the
+ * @record @buf.
+ *
+ * @record: pointer to record metadata.
+ * @buf: pointer to userspace contents to write to backend
*
* Returns 0 on success, and non-zero on error.
*
@@ -196,10 +189,8 @@ struct pstore_info {
ssize_t (*read)(struct pstore_record *record);
int (*write)(struct pstore_record *record);
int (*write_buf)(struct pstore_record *record);
- int (*write_buf_user)(enum pstore_type_id type,
- enum kmsg_dump_reason reason, u64 *id,
- unsigned int part, const char __user *buf,
- bool compressed, size_t size, struct pstore_info *psi);
+ int (*write_buf_user)(struct pstore_record *record,
+ const char __user *buf);
int (*erase)(struct pstore_record *record);
};
--
2.7.4
The read/mkfile pair pass the same arguments and should be cleared
between calls. Move to a structure and wipe it after every loop.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/platform.c | 55 +++++++++++++++++++++++++++-----------------------
include/linux/pstore.h | 28 ++++++++++++++++++++++++-
2 files changed, 57 insertions(+), 26 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 320a673ecb5b..3fa1575a6e36 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -766,16 +766,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
void pstore_get_records(int quiet)
{
struct pstore_info *psi = psinfo;
- char *buf = NULL;
- ssize_t size;
- u64 id;
- int count;
- enum pstore_type_id type;
- struct timespec time;
+ struct pstore_record record = { .psi = psi, };
int failed = 0, rc;
- bool compressed;
int unzipped_len = -1;
- ssize_t ecc_notice_size = 0;
if (!psi)
return;
@@ -784,39 +777,51 @@ void pstore_get_records(int quiet)
if (psi->open && psi->open(psi))
goto out;
- while ((size = psi->read(&id, &type, &count, &time, &buf, &compressed,
- &ecc_notice_size, psi)) > 0) {
- if (compressed && (type == PSTORE_TYPE_DMESG)) {
+ while ((record.size = psi->read(&record.id, &record.type,
+ &record.count, &record.time,
+ &record.buf, &record.compressed,
+ &record.ecc_notice_size,
+ record.psi)) > 0) {
+ if (record.compressed &&
+ record.type == PSTORE_TYPE_DMESG) {
if (big_oops_buf)
- unzipped_len = pstore_decompress(buf,
- big_oops_buf, size,
+ unzipped_len = pstore_decompress(
+ record.buf,
+ big_oops_buf,
+ record.size,
big_oops_buf_sz);
if (unzipped_len > 0) {
- if (ecc_notice_size)
+ if (record.ecc_notice_size)
memcpy(big_oops_buf + unzipped_len,
- buf + size, ecc_notice_size);
- kfree(buf);
- buf = big_oops_buf;
- size = unzipped_len;
- compressed = false;
+ record.buf + recorrecord.size,
+ record.ecc_notice_size);
+ kfree(record.buf);
+ record.buf = big_oops_buf;
+ record.size = unzipped_len;
+ record.compressed = false;
} else {
pr_err("decompression failed;returned %d\n",
unzipped_len);
- compressed = true;
+ record.compressed = true;
}
}
- rc = pstore_mkfile(type, psi->name, id, count, buf,
- compressed, size + ecc_notice_size,
- time, psi);
+ rc = pstore_mkfile(record.type, psi->name, record.id,
+ record.count, record.buf,
+ record.compressed,
+ record.size + record.ecc_notice_size,
+ record.time, record.psi);
if (unzipped_len < 0) {
/* Free buffer other than big oops */
- kfree(buf);
- buf = NULL;
+ kfree(record.buf);
+ record.buf = NULL;
} else
unzipped_len = -1;
if (rc && (rc != -EEXIST || !quiet))
failed++;
+
+ memset(&record, 0, sizeof(record));
+ record.psi = psi;
}
if (psi->close)
psi->close(psi);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 083b10bacd4a..7b25f7f17915 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -30,6 +30,8 @@
#include <linux/time.h>
#include <linux/types.h>
+struct module;
+
/* pstore record types (see fs/pstore/inode.c for filename templates) */
enum pstore_type_id {
PSTORE_TYPE_DMESG = 0,
@@ -45,7 +47,31 @@ enum pstore_type_id {
PSTORE_TYPE_UNKNOWN = 255
};
-struct module;
+struct pstore_info;
+/**
+ * struct pstore_record - details of a pstore record entry
+ * @psi: pstore backend driver information
+ * @type: pstore record type
+ * @id: per-type unique identifier for record
+ * @time: timestamp of the record
+ * @count: for PSTORE_TYPE_DMESG, the Oops count.
+ * @compressed: for PSTORE_TYPE_DMESG, whether the buffer is compressed
+ * @buf: pointer to record contents
+ * @size: size of @buf
+ * @ecc_notice_size:
+ * ECC information for @buf
+ */
+struct pstore_record {
+ struct pstore_info *psi;
+ enum pstore_type_id type;
+ u64 id;
+ struct timespec time;
+ int count;
+ bool compressed;
+ char *buf;
+ ssize_t size;
+ ssize_t ecc_notice_size;
+};
/**
* struct pstore_info - backend pstore driver structure
--
2.7.4
pstore_mkfile() shouldn't have to memcpy the record contents. It can use
the existing copy instead. This adjusts the allocation lifetime management
and renames the contents variable from "data" to "buf" to assist moving to
struct pstore_record in the future.
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/inode.c | 22 +++++++++++++++-------
fs/pstore/platform.c | 16 ++++++++++++----
2 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index a98787bab3e6..3d1f047e4f41 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -52,7 +52,7 @@ struct pstore_private {
u64 id;
int count;
ssize_t size;
- char data[];
+ char *buf;
};
struct pstore_ftrace_seq_data {
@@ -63,6 +63,14 @@ struct pstore_ftrace_seq_data {
#define REC_SIZE sizeof(struct pstore_ftrace_record)
+static void free_pstore_private(struct pstore_private *private)
+{
+ if (!private)
+ return;
+ kfree(private->buf);
+ kfree(private);
+}
+
static void *pstore_ftrace_seq_start(struct seq_file *s, loff_t *pos)
{
struct pstore_private *ps = s->private;
@@ -105,7 +113,7 @@ static int pstore_ftrace_seq_show(struct seq_file *s, void *v)
{
struct pstore_private *ps = s->private;
struct pstore_ftrace_seq_data *data = v;
- struct pstore_ftrace_record *rec = (void *)(ps->data + data->off);
+ struct pstore_ftrace_record *rec = (void *)(ps->buf + data->off);
seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %pf <- %pF\n",
pstore_ftrace_decode_cpu(rec),
@@ -143,7 +151,7 @@ static ssize_t pstore_file_read(struct file *file, char __user *userbuf,
if (ps->type == PSTORE_TYPE_FTRACE)
return seq_read(file, userbuf, count, ppos);
- return simple_read_from_buffer(userbuf, count, ppos, ps->data, ps->size);
+ return simple_read_from_buffer(userbuf, count, ppos, ps->buf, ps->size);
}
static int pstore_file_open(struct inode *inode, struct file *file)
@@ -221,7 +229,7 @@ static void pstore_evict_inode(struct inode *inode)
spin_lock_irqsave(&allpstore_lock, flags);
list_del(&p->list);
spin_unlock_irqrestore(&allpstore_lock, flags);
- kfree(p);
+ free_pstore_private(p);
}
}
@@ -332,7 +340,7 @@ int pstore_mkfile(struct pstore_record *record)
goto fail;
inode->i_mode = S_IFREG | 0444;
inode->i_fop = &pstore_file_operations;
- private = kmalloc(sizeof *private + size, GFP_KERNEL);
+ private = kzalloc(sizeof(*private), GFP_KERNEL);
if (!private)
goto fail_alloc;
private->type = record->type;
@@ -394,7 +402,7 @@ int pstore_mkfile(struct pstore_record *record)
if (!dentry)
goto fail_lockedalloc;
- memcpy(private->data, record->buf, size);
+ private->buf = record->buf;
inode->i_size = private->size = size;
inode->i_private = private;
@@ -414,7 +422,7 @@ int pstore_mkfile(struct pstore_record *record)
fail_lockedalloc:
inode_unlock(d_inode(root));
- kfree(private);
+ free_pstore_private(private);
fail_alloc:
iput(inode);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index c0d401e732e6..d897e2f11b6a 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -828,14 +828,22 @@ void pstore_get_records(int quiet)
if (psi->open && psi->open(psi))
goto out;
+ /*
+ * Backend callback read() allocates record.buf. decompress_record()
+ * may reallocate record.buf. On success, pstore_mkfile() will keep
+ * the record.buf, so free it only on failure.
+ */
while ((record.size = psi->read(&record)) > 0) {
decompress_record(&record);
rc = pstore_mkfile(&record);
+ if (rc) {
+ /* pstore_mkfile() did not take buf, so free it. */
+ kfree(record.buf);
+ if (rc != -EEXIST || !quiet)
+ failed++;
+ }
- if (rc && (rc != -EEXIST || !quiet))
- failed++;
-
- kfree(record.buf);
+ /* Reset for next record. */
memset(&record, 0, sizeof(record));
record.psi = psi;
}
--
2.7.4
The argument list for the pstore_read() interface is unwieldy. This changes
passes the new struct pstore_record instead. The erst backend was already
doing something similar internally.
Signed-off-by: Kees Cook <[email protected]>
---
arch/powerpc/kernel/nvram_64.c | 61 +++++++++++-----------
drivers/acpi/apei/erst.c | 38 ++++++--------
drivers/firmware/efi/efi-pstore.c | 104 ++++++++++++++++----------------------
fs/pstore/platform.c | 7 +--
fs/pstore/ram.c | 53 ++++++++++---------
include/linux/pstore.h | 20 +++-----
6 files changed, 124 insertions(+), 159 deletions(-)
diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index d5e2b8309939..7f192001d09a 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -442,10 +442,7 @@ static int nvram_pstore_write(enum pstore_type_id type,
* Returns the length of the data we read from each partition.
* Returns 0 if we've been called before.
*/
-static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
- int *count, struct timespec *time, char **buf,
- bool *compressed, ssize_t *ecc_notice_size,
- struct pstore_info *psi)
+static ssize_t nvram_pstore_read(struct pstore_record *record)
{
struct oops_log_info *oops_hdr;
unsigned int err_type, id_no, size = 0;
@@ -459,40 +456,40 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
switch (nvram_type_ids[read_type]) {
case PSTORE_TYPE_DMESG:
part = &oops_log_partition;
- *type = PSTORE_TYPE_DMESG;
+ record->type = PSTORE_TYPE_DMESG;
break;
case PSTORE_TYPE_PPC_COMMON:
sig = NVRAM_SIG_SYS;
part = &common_partition;
- *type = PSTORE_TYPE_PPC_COMMON;
- *id = PSTORE_TYPE_PPC_COMMON;
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->type = PSTORE_TYPE_PPC_COMMON;
+ record->id = PSTORE_TYPE_PPC_COMMON;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
break;
#ifdef CONFIG_PPC_PSERIES
case PSTORE_TYPE_PPC_RTAS:
part = &rtas_log_partition;
- *type = PSTORE_TYPE_PPC_RTAS;
- time->tv_sec = last_rtas_event;
- time->tv_nsec = 0;
+ record->type = PSTORE_TYPE_PPC_RTAS;
+ record->time.tv_sec = last_rtas_event;
+ record->time.tv_nsec = 0;
break;
case PSTORE_TYPE_PPC_OF:
sig = NVRAM_SIG_OF;
part = &of_config_partition;
- *type = PSTORE_TYPE_PPC_OF;
- *id = PSTORE_TYPE_PPC_OF;
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->type = PSTORE_TYPE_PPC_OF;
+ record->id = PSTORE_TYPE_PPC_OF;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
break;
#endif
#ifdef CONFIG_PPC_POWERNV
case PSTORE_TYPE_PPC_OPAL:
sig = NVRAM_SIG_FW;
part = &skiboot_partition;
- *type = PSTORE_TYPE_PPC_OPAL;
- *id = PSTORE_TYPE_PPC_OPAL;
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->type = PSTORE_TYPE_PPC_OPAL;
+ record->id = PSTORE_TYPE_PPC_OPAL;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
break;
#endif
default:
@@ -520,10 +517,10 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
return 0;
}
- *count = 0;
+ record->count = 0;
if (part->os_partition)
- *id = id_no;
+ record->id = id_no;
if (nvram_type_ids[read_type] == PSTORE_TYPE_DMESG) {
size_t length, hdr_size;
@@ -533,28 +530,28 @@ static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
/* Old format oops header had 2-byte record size */
hdr_size = sizeof(u16);
length = be16_to_cpu(oops_hdr->version);
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
} else {
hdr_size = sizeof(*oops_hdr);
length = be16_to_cpu(oops_hdr->report_length);
- time->tv_sec = be64_to_cpu(oops_hdr->timestamp);
- time->tv_nsec = 0;
+ record->time.tv_sec = be64_to_cpu(oops_hdr->timestamp);
+ record->time.tv_nsec = 0;
}
- *buf = kmemdup(buff + hdr_size, length, GFP_KERNEL);
+ record->buf = kmemdup(buff + hdr_size, length, GFP_KERNEL);
kfree(buff);
- if (*buf == NULL)
+ if (record->buf == NULL)
return -ENOMEM;
- *ecc_notice_size = 0;
+ record->ecc_notice_size = 0;
if (err_type == ERR_TYPE_KERNEL_PANIC_GZ)
- *compressed = true;
+ record->compressed = true;
else
- *compressed = false;
+ record->compressed = false;
return length;
}
- *buf = buff;
+ record->buf = buff;
return part->size;
}
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index ec4f507b524f..bbefb7522f80 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -925,10 +925,7 @@ static int erst_check_table(struct acpi_table_erst *erst_tab)
static int erst_open_pstore(struct pstore_info *psi);
static int erst_close_pstore(struct pstore_info *psi);
-static ssize_t erst_reader(u64 *id, enum pstore_type_id *type, int *count,
- struct timespec *time, char **buf,
- bool *compressed, ssize_t *ecc_notice_size,
- struct pstore_info *psi);
+static ssize_t erst_reader(struct pstore_record *record);
static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
u64 *id, unsigned int part, int count, bool compressed,
size_t size, struct pstore_info *psi);
@@ -986,10 +983,7 @@ static int erst_close_pstore(struct pstore_info *psi)
return 0;
}
-static ssize_t erst_reader(u64 *id, enum pstore_type_id *type, int *count,
- struct timespec *time, char **buf,
- bool *compressed, ssize_t *ecc_notice_size,
- struct pstore_info *psi)
+static ssize_t erst_reader(struct pstore_record *record)
{
int rc;
ssize_t len = 0;
@@ -1027,33 +1021,33 @@ static ssize_t erst_reader(u64 *id, enum pstore_type_id *type, int *count,
if (uuid_le_cmp(rcd->hdr.creator_id, CPER_CREATOR_PSTORE) != 0)
goto skip;
- *buf = kmalloc(len, GFP_KERNEL);
- if (*buf == NULL) {
+ record->buf = kmalloc(len, GFP_KERNEL);
+ if (record->buf == NULL) {
rc = -ENOMEM;
goto out;
}
- memcpy(*buf, rcd->data, len - sizeof(*rcd));
- *id = record_id;
- *compressed = false;
- *ecc_notice_size = 0;
+ memcpy(record->buf, rcd->data, len - sizeof(*rcd));
+ record->id = record_id;
+ record->compressed = false;
+ record->ecc_notice_size = 0;
if (uuid_le_cmp(rcd->sec_hdr.section_type,
CPER_SECTION_TYPE_DMESG_Z) == 0) {
- *type = PSTORE_TYPE_DMESG;
- *compressed = true;
+ record->type = PSTORE_TYPE_DMESG;
+ record->compressed = true;
} else if (uuid_le_cmp(rcd->sec_hdr.section_type,
CPER_SECTION_TYPE_DMESG) == 0)
- *type = PSTORE_TYPE_DMESG;
+ record->type = PSTORE_TYPE_DMESG;
else if (uuid_le_cmp(rcd->sec_hdr.section_type,
CPER_SECTION_TYPE_MCE) == 0)
- *type = PSTORE_TYPE_MCE;
+ record->type = PSTORE_TYPE_MCE;
else
- *type = PSTORE_TYPE_UNKNOWN;
+ record->type = PSTORE_TYPE_UNKNOWN;
if (rcd->hdr.validation_bits & CPER_VALID_TIMESTAMP)
- time->tv_sec = rcd->hdr.timestamp;
+ record->time.tv_sec = rcd->hdr.timestamp;
else
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
out:
kfree(rcd);
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index f402ba2eed46..bda24129e85b 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -28,26 +28,16 @@ static int efi_pstore_close(struct pstore_info *psi)
return 0;
}
-struct pstore_read_data {
- u64 *id;
- enum pstore_type_id *type;
- int *count;
- struct timespec *timespec;
- bool *compressed;
- ssize_t *ecc_notice_size;
- char **buf;
-};
-
static inline u64 generic_id(unsigned long timestamp,
unsigned int part, int count)
{
return ((u64) timestamp * 100 + part) * 1000 + count;
}
-static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
+static int efi_pstore_read_func(struct efivar_entry *entry,
+ struct pstore_record *record)
{
efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
- struct pstore_read_data *cb_data = data;
char name[DUMP_NAME_LEN], data_type;
int i;
int cnt;
@@ -61,37 +51,37 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
name[i] = entry->var.VariableName[i];
if (sscanf(name, "dump-type%u-%u-%d-%lu-%c",
- cb_data->type, &part, &cnt, &time, &data_type) == 5) {
- *cb_data->id = generic_id(time, part, cnt);
- *cb_data->count = cnt;
- cb_data->timespec->tv_sec = time;
- cb_data->timespec->tv_nsec = 0;
+ &record->type, &part, &cnt, &time, &data_type) == 5) {
+ record->id = generic_id(time, part, cnt);
+ record->count = cnt;
+ record->time.tv_sec = time;
+ record->time.tv_nsec = 0;
if (data_type == 'C')
- *cb_data->compressed = true;
+ record->compressed = true;
else
- *cb_data->compressed = false;
- *cb_data->ecc_notice_size = 0;
+ record->compressed = false;
+ record->ecc_notice_size = 0;
} else if (sscanf(name, "dump-type%u-%u-%d-%lu",
- cb_data->type, &part, &cnt, &time) == 4) {
- *cb_data->id = generic_id(time, part, cnt);
- *cb_data->count = cnt;
- cb_data->timespec->tv_sec = time;
- cb_data->timespec->tv_nsec = 0;
- *cb_data->compressed = false;
- *cb_data->ecc_notice_size = 0;
+ &record->type, &part, &cnt, &time) == 4) {
+ record->id = generic_id(time, part, cnt);
+ record->count = cnt;
+ record->time.tv_sec = time;
+ record->time.tv_nsec = 0;
+ record->compressed = false;
+ record->ecc_notice_size = 0;
} else if (sscanf(name, "dump-type%u-%u-%lu",
- cb_data->type, &part, &time) == 3) {
+ &record->type, &part, &time) == 3) {
/*
* Check if an old format,
* which doesn't support holding
* multiple logs, remains.
*/
- *cb_data->id = generic_id(time, part, 0);
- *cb_data->count = 0;
- cb_data->timespec->tv_sec = time;
- cb_data->timespec->tv_nsec = 0;
- *cb_data->compressed = false;
- *cb_data->ecc_notice_size = 0;
+ record->id = generic_id(time, part, 0);
+ record->count = 0;
+ record->time.tv_sec = time;
+ record->time.tv_nsec = 0;
+ record->compressed = false;
+ record->ecc_notice_size = 0;
} else
return 0;
@@ -99,7 +89,7 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
__efivar_entry_get(entry, &entry->var.Attributes,
&entry->var.DataSize, entry->var.Data);
size = entry->var.DataSize;
- memcpy(*cb_data->buf, entry->var.Data,
+ memcpy(record->buf, entry->var.Data,
(size_t)min_t(unsigned long, EFIVARS_DATA_SIZE_MAX, size));
return size;
@@ -164,7 +154,7 @@ static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
/**
* efi_pstore_sysfs_entry_iter
*
- * @data: function-specific data to pass to callback
+ * @record: pstore record to pass to callback
* @pos: entry to begin iterating from
*
* You MUST call efivar_enter_iter_begin() before this function, and
@@ -175,7 +165,8 @@ static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
* the next entry of the last one passed to efi_pstore_read_func().
* To begin iterating from the beginning of the list @pos must be %NULL.
*/
-static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
+static int efi_pstore_sysfs_entry_iter(struct pstore_record *record,
+ struct efivar_entry **pos)
{
struct efivar_entry *entry, *n;
struct list_head *head = &efivar_sysfs_list;
@@ -186,7 +177,7 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
list_for_each_entry_safe(entry, n, head, list) {
efi_pstore_scan_sysfs_enter(entry, n, head);
- size = efi_pstore_read_func(entry, data);
+ size = efi_pstore_read_func(entry, record);
ret = efi_pstore_scan_sysfs_exit(entry, n, head,
size < 0);
if (ret)
@@ -201,7 +192,7 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
list_for_each_entry_safe_from((*pos), n, head, list) {
efi_pstore_scan_sysfs_enter((*pos), n, head);
- size = efi_pstore_read_func((*pos), data);
+ size = efi_pstore_read_func((*pos), record);
ret = efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
if (ret)
return ret;
@@ -225,36 +216,27 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
* size < 0: Failed to get data of entry logging via efi_pstore_write(),
* and pstore will stop reading entry.
*/
-static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
- int *count, struct timespec *timespec,
- char **buf, bool *compressed,
- ssize_t *ecc_notice_size,
- struct pstore_info *psi)
+static ssize_t efi_pstore_read(struct pstore_record *record)
{
- struct pstore_read_data data;
+ struct efivar_entry *entry = (struct efivar_entry *)record->psi->data;
ssize_t size;
- data.id = id;
- data.type = type;
- data.count = count;
- data.timespec = timespec;
- data.compressed = compressed;
- data.ecc_notice_size = ecc_notice_size;
- data.buf = buf;
-
- *data.buf = kzalloc(EFIVARS_DATA_SIZE_MAX, GFP_KERNEL);
- if (!*data.buf)
+ record->buf = kzalloc(EFIVARS_DATA_SIZE_MAX, GFP_KERNEL);
+ if (!record->buf)
return -ENOMEM;
if (efivar_entry_iter_begin()) {
- kfree(*data.buf);
- return -EINTR;
+ size = -EINTR;
+ goto out;
}
- size = efi_pstore_sysfs_entry_iter(&data,
- (struct efivar_entry **)&psi->data);
+ size = efi_pstore_sysfs_entry_iter(record, &entry);
efivar_entry_iter_end();
- if (size <= 0)
- kfree(*data.buf);
+
+out:
+ if (size <= 0) {
+ kfree(record->buf);
+ record->buf = NULL;
+ }
return size;
}
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 168e03fd5e58..47968c2f2d0d 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -807,12 +807,7 @@ void pstore_get_records(int quiet)
if (psi->open && psi->open(psi))
goto out;
- while ((record.size = psi->read(&record.id, &record.type,
- &record.count, &record.time,
- &record.buf, &record.compressed,
- &record.ecc_notice_size,
- record.psi)) > 0) {
-
+ while ((record.size = psi->read(&record)) > 0) {
decompress_record(&record);
rc = pstore_mkfile(&record);
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 11f918d34b1e..ca6e2a814e37 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -235,35 +235,34 @@ static ssize_t ftrace_log_combine(struct persistent_ram_zone *dest,
return 0;
}
-static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
- int *count, struct timespec *time,
- char **buf, bool *compressed,
- ssize_t *ecc_notice_size,
- struct pstore_info *psi)
+static ssize_t ramoops_pstore_read(struct pstore_record *record)
{
ssize_t size = 0;
- struct ramoops_context *cxt = psi->data;
+ struct ramoops_context *cxt = record->psi->data;
struct persistent_ram_zone *prz = NULL;
int header_length = 0;
bool free_prz = false;
- /* Ramoops headers provide time stamps for PSTORE_TYPE_DMESG, but
+ /*
+ * Ramoops headers provide time stamps for PSTORE_TYPE_DMESG, but
* PSTORE_TYPE_CONSOLE and PSTORE_TYPE_FTRACE don't currently have
* valid time stamps, so it is initialized to zero.
*/
- time->tv_sec = 0;
- time->tv_nsec = 0;
- *compressed = false;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
+ record->compressed = false;
/* Find the next valid persistent_ram_zone for DMESG */
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, &cxt->dump_read_cnt,
- cxt->max_dump_cnt, id, type,
+ cxt->max_dump_cnt, &record->id,
+ &record->type,
PSTORE_TYPE_DMESG, 1);
if (!prz_ok(prz))
continue;
header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
- time, compressed);
+ &record->time,
+ &record->compressed);
/* Clear and skip this DMESG record if it has no valid header */
if (!header_length) {
persistent_ram_free_old(prz);
@@ -274,18 +273,20 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
- 1, id, type, PSTORE_TYPE_CONSOLE, 0);
+ 1, &record->id, &record->type,
+ PSTORE_TYPE_CONSOLE, 0);
if (!prz_ok(prz))
prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
- 1, id, type, PSTORE_TYPE_PMSG, 0);
+ 1, &record->id, &record->type,
+ PSTORE_TYPE_PMSG, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
prz = ramoops_get_next_prz(cxt->fprzs,
- &cxt->ftrace_read_cnt, 1, id, type,
- PSTORE_TYPE_FTRACE, 0);
+ &cxt->ftrace_read_cnt, 1, &record->id,
+ &record->type, PSTORE_TYPE_FTRACE, 0);
} else {
/*
* Build a new dummy record which combines all the
@@ -302,8 +303,10 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
prz_next = ramoops_get_next_prz(cxt->fprzs,
&cxt->ftrace_read_cnt,
- cxt->max_ftrace_cnt, id,
- type, PSTORE_TYPE_FTRACE, 0);
+ cxt->max_ftrace_cnt,
+ &record->id,
+ &record->type,
+ PSTORE_TYPE_FTRACE, 0);
if (!prz_ok(prz_next))
continue;
@@ -316,7 +319,7 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
if (size)
goto out;
}
- *id = 0;
+ record->id = 0;
prz = tmp_prz;
}
}
@@ -329,17 +332,19 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
size = persistent_ram_old_size(prz) - header_length;
/* ECC correction notice */
- *ecc_notice_size = persistent_ram_ecc_string(prz, NULL, 0);
+ record->ecc_notice_size = persistent_ram_ecc_string(prz, NULL, 0);
- *buf = kmalloc(size + *ecc_notice_size + 1, GFP_KERNEL);
- if (*buf == NULL) {
+ record->buf = kmalloc(size + record->ecc_notice_size + 1, GFP_KERNEL);
+ if (record->buf == NULL) {
size = -ENOMEM;
goto out;
}
- memcpy(*buf, (char *)persistent_ram_old(prz) + header_length, size);
+ memcpy(record->buf, (char *)persistent_ram_old(prz) + header_length,
+ size);
- persistent_ram_ecc_string(prz, *buf + size, *ecc_notice_size + 1);
+ persistent_ram_ecc_string(prz, record->buf + size,
+ record->ecc_notice_size + 1);
out:
if (free_prz) {
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 7b25f7f17915..8b51c9da8d16 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -111,16 +111,11 @@ struct pstore_record {
* Read next available backend record. Called after a successful
* @open.
*
- * @id: out: unique identifier for the record
- * @type: out: pstore record type
- * @count: out: for PSTORE_TYPE_DMESG, the Oops count.
- * @time: out: timestamp for the record
- * @buf: out: kmalloc copy of record contents, to be freed by pstore
- * @compressed:
- * out: if the record contents are compressed
- * @ecc_notice_size:
- * out: ECC information
- * @psi: in: pointer to the struct pstore_info for the backend
+ * @record:
+ * pointer to record to populate. @buf should be allocated
+ * by the backend and filled. At least @type and @id should
+ * be populated, since these are used when creating pstorefs
+ * file names.
*
* Returns record size on success, zero when no more records are
* available, or negative on error.
@@ -207,10 +202,7 @@ struct pstore_info {
int (*open)(struct pstore_info *psi);
int (*close)(struct pstore_info *psi);
- ssize_t (*read)(u64 *id, enum pstore_type_id *type,
- int *count, struct timespec *time, char **buf,
- bool *compressed, ssize_t *ecc_notice_size,
- struct pstore_info *psi);
+ ssize_t (*read)(struct pstore_record *record);
int (*write)(enum pstore_type_id type,
enum kmsg_dump_reason reason, u64 *id,
unsigned int part, int count, bool compressed,
--
2.7.4
On Tue, Mar 7, 2017 at 6:55 AM, Kees Cook <[email protected]> wrote:
> The read/mkfile pair pass the same arguments and should be cleared
> between calls. Move to a structure and wipe it after every loop.
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/pstore/platform.c | 55 +++++++++++++++++++++++++++-----------------------
> include/linux/pstore.h | 28 ++++++++++++++++++++++++-
> 2 files changed, 57 insertions(+), 26 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 320a673ecb5b..3fa1575a6e36 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -766,16 +766,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
> void pstore_get_records(int quiet)
> {
> struct pstore_info *psi = psinfo;
> - char *buf = NULL;
> - ssize_t size;
> - u64 id;
> - int count;
> - enum pstore_type_id type;
> - struct timespec time;
> + struct pstore_record record = { .psi = psi, };
> int failed = 0, rc;
> - bool compressed;
> int unzipped_len = -1;
> - ssize_t ecc_notice_size = 0;
>
> if (!psi)
> return;
> @@ -784,39 +777,51 @@ void pstore_get_records(int quiet)
> if (psi->open && psi->open(psi))
> goto out;
>
> - while ((size = psi->read(&id, &type, &count, &time, &buf, &compressed,
> - &ecc_notice_size, psi)) > 0) {
> - if (compressed && (type == PSTORE_TYPE_DMESG)) {
> + while ((record.size = psi->read(&record.id, &record.type,
> + &record.count, &record.time,
> + &record.buf, &record.compressed,
> + &record.ecc_notice_size,
> + record.psi)) > 0) {
> + if (record.compressed &&
> + record.type == PSTORE_TYPE_DMESG) {
> if (big_oops_buf)
> - unzipped_len = pstore_decompress(buf,
> - big_oops_buf, size,
> + unzipped_len = pstore_decompress(
> + record.buf,
> + big_oops_buf,
> + record.size,
> big_oops_buf_sz);
>
> if (unzipped_len > 0) {
> - if (ecc_notice_size)
> + if (record.ecc_notice_size)
> memcpy(big_oops_buf + unzipped_len,
> - buf + size, ecc_notice_size);
> - kfree(buf);
> - buf = big_oops_buf;
> - size = unzipped_len;
> - compressed = false;
> + record.buf + recorrecord.size,
A typo on record.size.
Thanks,
Namhyung
Hi Kees,
On Tue, Mar 7, 2017 at 6:55 AM, Kees Cook <[email protected]> wrote:
> Technically, it might be possible for struct pstore_info to go out of
> scope after the module_put(), so report the backend name first.
But in that case, using pstore will crash the kernel anyway, right?
If so, why pstore doesn't keep a reference until unregister?
Do I miss something?
Thanks,
Namhyung
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/pstore/platform.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 074fe85a2078..d69ef8a840b9 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -722,10 +722,10 @@ int pstore_register(struct pstore_info *psi)
> */
> backend = psi->name;
>
> - module_put(owner);
> -
> pr_info("Registered %s as persistent store backend\n", psi->name);
>
> + module_put(owner);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(pstore_register);
> --
> 2.7.4
>
On Tue, Mar 7, 2017 at 8:16 AM, Namhyung Kim <[email protected]> wrote:
> Hi Kees,
>
> On Tue, Mar 7, 2017 at 6:55 AM, Kees Cook <[email protected]> wrote:
>> Technically, it might be possible for struct pstore_info to go out of
>> scope after the module_put(), so report the backend name first.
>
> But in that case, using pstore will crash the kernel anyway, right?
> If so, why pstore doesn't keep a reference until unregister?
> Do I miss something?
I could be wrong with this, since the backend can't call unregister
until register has finished... I'll drop this patch.
-Kees
>
> Thanks,
> Namhyung
>
>
>>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> fs/pstore/platform.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 074fe85a2078..d69ef8a840b9 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -722,10 +722,10 @@ int pstore_register(struct pstore_info *psi)
>> */
>> backend = psi->name;
>>
>> - module_put(owner);
>> -
>> pr_info("Registered %s as persistent store backend\n", psi->name);
>>
>> + module_put(owner);
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pstore_register);
>> --
>> 2.7.4
>>
--
Kees Cook
Pixel Security
On Tue, Mar 7, 2017 at 8:22 AM, Namhyung Kim <[email protected]> wrote:
> On Tue, Mar 7, 2017 at 6:55 AM, Kees Cook <[email protected]> wrote:
>> The read/mkfile pair pass the same arguments and should be cleared
>> between calls. Move to a structure and wipe it after every loop.
>>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> fs/pstore/platform.c | 55 +++++++++++++++++++++++++++-----------------------
>> include/linux/pstore.h | 28 ++++++++++++++++++++++++-
>> 2 files changed, 57 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 320a673ecb5b..3fa1575a6e36 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -766,16 +766,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
>> void pstore_get_records(int quiet)
>> {
>> struct pstore_info *psi = psinfo;
>> - char *buf = NULL;
>> - ssize_t size;
>> - u64 id;
>> - int count;
>> - enum pstore_type_id type;
>> - struct timespec time;
>> + struct pstore_record record = { .psi = psi, };
>> int failed = 0, rc;
>> - bool compressed;
>> int unzipped_len = -1;
>> - ssize_t ecc_notice_size = 0;
>>
>> if (!psi)
>> return;
>> @@ -784,39 +777,51 @@ void pstore_get_records(int quiet)
>> if (psi->open && psi->open(psi))
>> goto out;
>>
>> - while ((size = psi->read(&id, &type, &count, &time, &buf, &compressed,
>> - &ecc_notice_size, psi)) > 0) {
>> - if (compressed && (type == PSTORE_TYPE_DMESG)) {
>> + while ((record.size = psi->read(&record.id, &record.type,
>> + &record.count, &record.time,
>> + &record.buf, &record.compressed,
>> + &record.ecc_notice_size,
>> + record.psi)) > 0) {
>> + if (record.compressed &&
>> + record.type == PSTORE_TYPE_DMESG) {
>> if (big_oops_buf)
>> - unzipped_len = pstore_decompress(buf,
>> - big_oops_buf, size,
>> + unzipped_len = pstore_decompress(
>> + record.buf,
>> + big_oops_buf,
>> + record.size,
>> big_oops_buf_sz);
>>
>> if (unzipped_len > 0) {
>> - if (ecc_notice_size)
>> + if (record.ecc_notice_size)
>> memcpy(big_oops_buf + unzipped_len,
>> - buf + size, ecc_notice_size);
>> - kfree(buf);
>> - buf = big_oops_buf;
>> - size = unzipped_len;
>> - compressed = false;
>> + record.buf + recorrecord.size,
>
> A typo on record.size.
Thanks! Yeah, 0-day noticed this too. I've refreshed the patches in my
tree with the correction now.
-Kees
--
Kees Cook
Pixel Security