[v6,2/4] remoteproc: coredump: Add minidump functionality

This change adds a new kind of core dump mechanism which instead of dumping
entire program segments of the firmware, dumps sections of the remoteproc
memory which are sufficient to allow debugging the firmware. This function
thus uses section headers instead of program headers during creation of the
core dump elf.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/remoteproc_coredump.c    | 132 ++++++++++++++++++++++++++++
 drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++++++
 include/linux/remoteproc.h                  |   1 +
 3 files changed, 160 insertions(+)

On Fri 02 Oct 21:05 CDT 2020, Siddharth Gupta wrote:

> This change adds a new kind of core dump mechanism which instead of dumping
> entire program segments of the firmware, dumps sections of the remoteproc
> memory which are sufficient to allow debugging the firmware. This function
> thus uses section headers instead of program headers during creation of the
> core dump elf.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/remoteproc_coredump.c    | 132 ++++++++++++++++++++++++++++
>  drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++++++
>  include/linux/remoteproc.h                  |   1 +
>  3 files changed, 160 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
> index bb15a29..e7d1394 100644
> --- a/drivers/remoteproc/remoteproc_coredump.c
> +++ b/drivers/remoteproc/remoteproc_coredump.c
> @@ -13,6 +13,8 @@
>  #include "remoteproc_internal.h"
>  #include "remoteproc_elf_helpers.h"
>  
> +#define MAX_STRTBL_SIZE 512
> +
>  struct rproc_coredump_state {
>  	struct rproc *rproc;
>  	void *header;
> @@ -323,3 +325,133 @@ void rproc_coredump(struct rproc *rproc)
>  	 */
>  	wait_for_completion(&dump_state.dump_done);
>  }
> +
> +/**
> + * rproc_minidump() - perform minidump
> + * @rproc:	rproc handle
> + *
> + * This function will generate an ELF header for the registered sections of
> + * segments and create a devcoredump device associated with rproc. Based on
> + * the coredump configuration this function will directly copy the segments
> + * from device memory to userspace or copy segments from device memory to
> + * a separate buffer, which can then be read by userspace.
> + * The first approach avoids using extra vmalloc memory. But it will stall
> + * recovery flow until dump is read by userspace.
> + */
> +void rproc_minidump(struct rproc *rproc)

Just to confirm, this does the same thing as rproc_coredump() with the
difference that instead of storing the segments in program headers, you
reference them using section headers?

> +{
> +	struct rproc_dump_segment *segment;
> +	void *shdr;
> +	void *ehdr;
> +	size_t data_size;
> +	size_t offset;
> +	void *data;
> +	u8 class = rproc->elf_class;
> +	int shnum;
> +	struct rproc_coredump_state dump_state;
> +	unsigned int dump_conf = rproc->dump_conf;
> +	char *str_tbl = "STR_TBL";
> +
> +	if (list_empty(&rproc->dump_segments) ||
> +	    dump_conf == RPROC_COREDUMP_DISABLED)
> +		return;
> +
> +	if (class == ELFCLASSNONE) {
> +		dev_err(&rproc->dev, "Elf class is not set\n");
> +		return;
> +	}
> +
> +	/*
> +	 * We allocate two extra section headers. The first one is null.
> +	 * Second section header is for the string table. Also space is
> +	 * allocated for string table.
> +	 */
> +	data_size = elf_size_of_hdr(class) + 2 * elf_size_of_shdr(class) +
> +		    MAX_STRTBL_SIZE;

Once you start populating the string table there's no checks that this
isn't overrun.

But really

> +	shnum = 2;
> +
> +	list_for_each_entry(segment, &rproc->dump_segments, node) {
> +		data_size += elf_size_of_shdr(class);
> +		if (dump_conf == RPROC_COREDUMP_DEFAULT)
> +			data_size += segment->size;
> +		shnum++;
> +	}
> +
> +	data = vmalloc(data_size);
> +	if (!data)
> +		return;
> +
> +	ehdr = data;
> +	memset(ehdr, 0, elf_size_of_hdr(class));
> +	/* e_ident field is common for both elf32 and elf64 */
> +	elf_hdr_init_ident(ehdr, class);
> +
> +	elf_hdr_set_e_type(class, ehdr, ET_CORE);
> +	elf_hdr_set_e_machine(class, ehdr, rproc->elf_machine);
> +	elf_hdr_set_e_version(class, ehdr, EV_CURRENT);
> +	elf_hdr_set_e_entry(class, ehdr, rproc->bootaddr);
> +	elf_hdr_set_e_shoff(class, ehdr, elf_size_of_hdr(class));
> +	elf_hdr_set_e_ehsize(class, ehdr, elf_size_of_hdr(class));
> +	elf_hdr_set_e_shentsize(class, ehdr, elf_size_of_shdr(class));
> +	elf_hdr_set_e_shnum(class, ehdr, shnum);
> +	elf_hdr_set_e_shstrndx(class, ehdr, 1);
> +
> +	/* Set the first section header as null and move to the next one. */
> +	shdr = data + elf_hdr_get_e_shoff(class, ehdr);
> +	memset(shdr, 0, elf_size_of_shdr(class));
> +	shdr += elf_size_of_shdr(class);
> +
> +	/* Initialize the string table. */
> +	offset = elf_hdr_get_e_shoff(class, ehdr) +
> +		 elf_size_of_shdr(class) * elf_hdr_get_e_shnum(class, ehdr);
> +	memset(data + offset, 0, MAX_STRTBL_SIZE);
> +
> +	/* Fill in the string table section header. */
> +	memset(shdr, 0, elf_size_of_shdr(class));
> +	elf_shdr_set_sh_type(class, shdr, SHT_STRTAB);
> +	elf_shdr_set_sh_offset(class, shdr, offset);
> +	elf_shdr_set_sh_size(class, shdr, MAX_STRTBL_SIZE);
> +	elf_shdr_set_sh_entsize(class, shdr, 0);
> +	elf_shdr_set_sh_flags(class, shdr, 0);
> +	elf_shdr_set_sh_name(class, shdr, set_section_name(str_tbl, ehdr, class));
> +	offset += elf_shdr_get_sh_size(class, shdr);
> +	shdr += elf_size_of_shdr(class);

I assume this last part creates the null entry? How about mentioning
that in a comment - and perhaps why there needs to be a null entry.

> +
> +	list_for_each_entry(segment, &rproc->dump_segments, node) {
> +		memset(shdr, 0, elf_size_of_shdr(class));
> +		elf_shdr_set_sh_type(class, shdr, SHT_PROGBITS);
> +		elf_shdr_set_sh_offset(class, shdr, offset);
> +		elf_shdr_set_sh_addr(class, shdr, segment->da);
> +		elf_shdr_set_sh_size(class, shdr, segment->size);
> +		elf_shdr_set_sh_entsize(class, shdr, 0);
> +		elf_shdr_set_sh_flags(class, shdr, SHF_WRITE);
> +		elf_shdr_set_sh_name(class, shdr,
> +				     set_section_name(segment->priv, ehdr, class));
> +
> +		/* No need to copy segments for inline dumps */
> +		if (dump_conf == RPROC_COREDUMP_DEFAULT)
> +			rproc_copy_segment(rproc, data + offset, segment, 0,
> +					   segment->size);
> +		offset += elf_shdr_get_sh_size(class, shdr);
> +		shdr += elf_size_of_shdr(class);
> +	}
> +
> +	if (dump_conf == RPROC_COREDUMP_DEFAULT) {
> +		dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
> +		return;
> +	}
> +
> +	/* Initialize the dump state struct to be used by rproc_coredump_read */
> +	dump_state.rproc = rproc;
> +	dump_state.header = data;
> +	init_completion(&dump_state.dump_done);
> +
> +	dev_coredumpm(&rproc->dev, NULL, &dump_state, data_size, GFP_KERNEL,
> +		      rproc_coredump_read, rproc_coredump_free);
> +
> +	/* Wait until the dump is read and free is called. Data is freed
> +	 * by devcoredump framework automatically after 5 minutes.
> +	 */
> +	wait_for_completion(&dump_state.dump_done);
> +}
> +EXPORT_SYMBOL(rproc_minidump);
> diff --git a/drivers/remoteproc/remoteproc_elf_helpers.h b/drivers/remoteproc/remoteproc_elf_helpers.h
> index 4b6be7b..d83ebca 100644
> --- a/drivers/remoteproc/remoteproc_elf_helpers.h
> +++ b/drivers/remoteproc/remoteproc_elf_helpers.h
> @@ -11,6 +11,7 @@
>  #include <linux/elf.h>
>  #include <linux/types.h>
>  
> +#define MAX_NAME_LENGTH 16

This name is too generic. Why is it 16?

> +static inline unsigned int set_section_name(const char *name, void *ehdr,
> +					    u8 class)
> +{
> +	u16 shstrndx = elf_hdr_get_e_shstrndx(class, ehdr);
> +	void *shdr;
> +	char *strtab;
> +	static int strtable_idx = 1;

This can't be static as this will only start at 1 on the first
invocation of rproc_minidump().

I think it would be perfectly fine if you simply scan the string list to
find the next available slot.

> +	int idx, ret = 0;

No need to initialize ret as the first usage is an assignment.

Regards,
Bjorn

On 10/26/2020 2:09 PM, Bjorn Andersson wrote:
> On Fri 02 Oct 21:05 CDT 2020, Siddharth Gupta wrote:
>
>> This change adds a new kind of core dump mechanism which instead of dumping
>> entire program segments of the firmware, dumps sections of the remoteproc
>> memory which are sufficient to allow debugging the firmware. This function
>> thus uses section headers instead of program headers during creation of the
>> core dump elf.
>>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> ---
>>   drivers/remoteproc/remoteproc_coredump.c    | 132 ++++++++++++++++++++++++++++
>>   drivers/remoteproc/remoteproc_elf_helpers.h |  27 ++++++
>>   include/linux/remoteproc.h                  |   1 +
>>   3 files changed, 160 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
>> index bb15a29..e7d1394 100644
>> --- a/drivers/remoteproc/remoteproc_coredump.c
>> +++ b/drivers/remoteproc/remoteproc_coredump.c
>> @@ -13,6 +13,8 @@
>>   #include "remoteproc_internal.h"
>>   #include "remoteproc_elf_helpers.h"
>>   
>> +#define MAX_STRTBL_SIZE 512
>> +
>>   struct rproc_coredump_state {
>>   	struct rproc *rproc;
>>   	void *header;
>> @@ -323,3 +325,133 @@ void rproc_coredump(struct rproc *rproc)
>>   	 */
>>   	wait_for_completion(&dump_state.dump_done);
>>   }
>> +
>> +/**
>> + * rproc_minidump() - perform minidump
>> + * @rproc:	rproc handle
>> + *
>> + * This function will generate an ELF header for the registered sections of
>> + * segments and create a devcoredump device associated with rproc. Based on
>> + * the coredump configuration this function will directly copy the segments
>> + * from device memory to userspace or copy segments from device memory to
>> + * a separate buffer, which can then be read by userspace.
>> + * The first approach avoids using extra vmalloc memory. But it will stall
>> + * recovery flow until dump is read by userspace.
>> + */
>> +void rproc_minidump(struct rproc *rproc)
> Just to confirm, this does the same thing as rproc_coredump() with the
> difference that instead of storing the segments in program headers, you
> reference them using section headers?

Yes, that is correct.

>
>> +{
>> +	struct rproc_dump_segment *segment;
>> +	void *shdr;
>> +	void *ehdr;
>> +	size_t data_size;
>> +	size_t offset;
>> +	void *data;
>> +	u8 class = rproc->elf_class;
>> +	int shnum;
>> +	struct rproc_coredump_state dump_state;
>> +	unsigned int dump_conf = rproc->dump_conf;
>> +	char *str_tbl = "STR_TBL";
>> +
>> +	if (list_empty(&rproc->dump_segments) ||
>> +	    dump_conf == RPROC_COREDUMP_DISABLED)
>> +		return;
>> +
>> +	if (class == ELFCLASSNONE) {
>> +		dev_err(&rproc->dev, "Elf class is not set\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * We allocate two extra section headers. The first one is null.
>> +	 * Second section header is for the string table. Also space is
>> +	 * allocated for string table.
>> +	 */
>> +	data_size = elf_size_of_hdr(class) + 2 * elf_size_of_shdr(class) +
>> +		    MAX_STRTBL_SIZE;
> Once you start populating the string table there's no checks that this
> isn't overrun.
I will update the code to dynamically allocate space for the STRTBL.
>
> But really
>
>> +	shnum = 2;
>> +
>> +	list_for_each_entry(segment, &rproc->dump_segments, node) {
>> +		data_size += elf_size_of_shdr(class);
>> +		if (dump_conf == RPROC_COREDUMP_DEFAULT)
>> +			data_size += segment->size;
>> +		shnum++;
>> +	}
>> +
>> +	data = vmalloc(data_size);
>> +	if (!data)
>> +		return;
>> +
>> +	ehdr = data;
>> +	memset(ehdr, 0, elf_size_of_hdr(class));
>> +	/* e_ident field is common for both elf32 and elf64 */
>> +	elf_hdr_init_ident(ehdr, class);
>> +
>> +	elf_hdr_set_e_type(class, ehdr, ET_CORE);
>> +	elf_hdr_set_e_machine(class, ehdr, rproc->elf_machine);
>> +	elf_hdr_set_e_version(class, ehdr, EV_CURRENT);
>> +	elf_hdr_set_e_entry(class, ehdr, rproc->bootaddr);
>> +	elf_hdr_set_e_shoff(class, ehdr, elf_size_of_hdr(class));
>> +	elf_hdr_set_e_ehsize(class, ehdr, elf_size_of_hdr(class));
>> +	elf_hdr_set_e_shentsize(class, ehdr, elf_size_of_shdr(class));
>> +	elf_hdr_set_e_shnum(class, ehdr, shnum);
>> +	elf_hdr_set_e_shstrndx(class, ehdr, 1);
>> +
>> +	/* Set the first section header as null and move to the next one. */
>> +	shdr = data + elf_hdr_get_e_shoff(class, ehdr);
>> +	memset(shdr, 0, elf_size_of_shdr(class));
>> +	shdr += elf_size_of_shdr(class);
>> +
>> +	/* Initialize the string table. */
>> +	offset = elf_hdr_get_e_shoff(class, ehdr) +
>> +		 elf_size_of_shdr(class) * elf_hdr_get_e_shnum(class, ehdr);
>> +	memset(data + offset, 0, MAX_STRTBL_SIZE);
>> +
>> +	/* Fill in the string table section header. */
>> +	memset(shdr, 0, elf_size_of_shdr(class));
>> +	elf_shdr_set_sh_type(class, shdr, SHT_STRTAB);
>> +	elf_shdr_set_sh_offset(class, shdr, offset);
>> +	elf_shdr_set_sh_size(class, shdr, MAX_STRTBL_SIZE);
>> +	elf_shdr_set_sh_entsize(class, shdr, 0);
>> +	elf_shdr_set_sh_flags(class, shdr, 0);
>> +	elf_shdr_set_sh_name(class, shdr, set_section_name(str_tbl, ehdr, class));
>> +	offset += elf_shdr_get_sh_size(class, shdr);
>> +	shdr += elf_size_of_shdr(class);
> I assume this last part creates the null entry? How about mentioning
> that in a comment - and perhaps why there needs to be a null entry.
Okay sure. I will add that comment.
>
>> +
>> +	list_for_each_entry(segment, &rproc->dump_segments, node) {
>> +		memset(shdr, 0, elf_size_of_shdr(class));
>> +		elf_shdr_set_sh_type(class, shdr, SHT_PROGBITS);
>> +		elf_shdr_set_sh_offset(class, shdr, offset);
>> +		elf_shdr_set_sh_addr(class, shdr, segment->da);
>> +		elf_shdr_set_sh_size(class, shdr, segment->size);
>> +		elf_shdr_set_sh_entsize(class, shdr, 0);
>> +		elf_shdr_set_sh_flags(class, shdr, SHF_WRITE);
>> +		elf_shdr_set_sh_name(class, shdr,
>> +				     set_section_name(segment->priv, ehdr, class));
>> +
>> +		/* No need to copy segments for inline dumps */
>> +		if (dump_conf == RPROC_COREDUMP_DEFAULT)
>> +			rproc_copy_segment(rproc, data + offset, segment, 0,
>> +					   segment->size);
>> +		offset += elf_shdr_get_sh_size(class, shdr);
>> +		shdr += elf_size_of_shdr(class);
>> +	}
>> +
>> +	if (dump_conf == RPROC_COREDUMP_DEFAULT) {
>> +		dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
>> +		return;
>> +	}
>> +
>> +	/* Initialize the dump state struct to be used by rproc_coredump_read */
>> +	dump_state.rproc = rproc;
>> +	dump_state.header = data;
>> +	init_completion(&dump_state.dump_done);
>> +
>> +	dev_coredumpm(&rproc->dev, NULL, &dump_state, data_size, GFP_KERNEL,
>> +		      rproc_coredump_read, rproc_coredump_free);
>> +
>> +	/* Wait until the dump is read and free is called. Data is freed
>> +	 * by devcoredump framework automatically after 5 minutes.
>> +	 */
>> +	wait_for_completion(&dump_state.dump_done);
>> +}
>> +EXPORT_SYMBOL(rproc_minidump);
>> diff --git a/drivers/remoteproc/remoteproc_elf_helpers.h b/drivers/remoteproc/remoteproc_elf_helpers.h
>> index 4b6be7b..d83ebca 100644
>> --- a/drivers/remoteproc/remoteproc_elf_helpers.h
>> +++ b/drivers/remoteproc/remoteproc_elf_helpers.h
>> @@ -11,6 +11,7 @@
>>   #include <linux/elf.h>
>>   #include <linux/types.h>
>>   
>> +#define MAX_NAME_LENGTH 16
> This name is too generic. Why is it 16?

I will update the name to  MAX_SHDR_NAME_LEN. In our usecase we didn't 
expect a length of the section name to exceed
16 characters (MAX_REGION_NAME_LENGTH defined in qcom_minidump.h in 
patch 03/04). It might change later if users
want to increase the size. What would you prefer the max name length for 
the section header to be?

>
>> +static inline unsigned int set_section_name(const char *name, void *ehdr,
>> +					    u8 class)
>> +{
>> +	u16 shstrndx = elf_hdr_get_e_shstrndx(class, ehdr);
>> +	void *shdr;
>> +	char *strtab;
>> +	static int strtable_idx = 1;
> This can't be static as this will only start at 1 on the first
> invocation of rproc_minidump().
>
> I think it would be perfectly fine if you simply scan the string list to
> find the next available slot.
Right. I will update this as well.
>
>> +	int idx, ret = 0;
> No need to initialize ret as the first usage is an assignment.
>
> Regards,
> Bjorn

I will make this change as well.

Thanks,
Siddharth

On Thu 29 Oct 18:54 CDT 2020, Siddharth Gupta wrote:

> 

> On 10/26/2020 2:09 PM, Bjorn Andersson wrote:

> > On Fri 02 Oct 21:05 CDT 2020, Siddharth Gupta wrote:

[..]
> > > diff --git a/drivers/remoteproc/remoteproc_elf_helpers.h b/drivers/remoteproc/remoteproc_elf_helpers.h

> > > index 4b6be7b..d83ebca 100644

> > > --- a/drivers/remoteproc/remoteproc_elf_helpers.h

> > > +++ b/drivers/remoteproc/remoteproc_elf_helpers.h

> > > @@ -11,6 +11,7 @@

> > >   #include <linux/elf.h>

> > >   #include <linux/types.h>

> > > +#define MAX_NAME_LENGTH 16

> > This name is too generic. Why is it 16?

> 

> I will update the name to  MAX_SHDR_NAME_LEN. In our usecase we didn't

> expect a length of the section name to exceed

> 16 characters (MAX_REGION_NAME_LENGTH defined in qcom_minidump.h in patch

> 03/04). It might change later if users

> want to increase the size. What would you prefer the max name length for the

> section header to be?

> 


If you calculate the size of the region based on the strings I don't see
why you need to limit it here - and you shouldn't use a bounded version
of strcpy in this case either.

I don't think this part of the code should truncate the strings, if we
need to sanitize the strings make sure to do that when you populate the
list.

Thanks,
Bjorn