linux-kernel - Re: [PATCH 2/3] remoteproc: Add inline coredump functionality

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200512051321.GA16107@builder.lan>
Date:   Mon, 11 May 2020 22:13:21 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     rishabhb@...eaurora.org
Cc:     linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        ohad@...ery.com, mathieu.poirier@...aro.org, tsoni@...eaurora.org,
        psodagud@...eaurora.org, sidgup@...eaurora.org,
        linux-remoteproc-owner@...r.kernel.org
Subject: Re: [PATCH 2/3] remoteproc: Add inline coredump functionality

On Mon 11 May 17:41 PDT 2020, rishabhb@...eaurora.org wrote:

> On 2020-05-11 17:30, Bjorn Andersson wrote:
> > On Mon 11 May 17:11 PDT 2020, rishabhb@...eaurora.org wrote:
> > > On 2020-05-07 13:21, Bjorn Andersson wrote:
> > > > On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
> > > > > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > > > > b/drivers/remoteproc/remoteproc_coredump.c
[..]
> > > > > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
> > > > > count,
> > > > > +				void *data, size_t header_size)
> > > > > +{
> > > > > +	void *device_mem;
> > > > > +	size_t data_left, copy_size, bytes_left = count;
> > > > > +	unsigned long addr;
> > > > > +	struct rproc_coredump_state *dump_state = data;
> > > > > +	struct rproc *rproc = dump_state->rproc;
> > > > > +	void *elfcore = dump_state->header;
> > > > > +
> > > > > +	/* Copy the header first */
> > > > > +	if (offset < header_size) {
> > > > > +		copy_size = header_size - offset;
> > > > > +		copy_size = min(copy_size, bytes_left);
> > > > > +
> > > > > +		memcpy(buffer, elfcore + offset, copy_size);
> > > > > +		offset += copy_size;
> > > > > +		bytes_left -= copy_size;
> > > > > +		buffer += copy_size;
> > > > > +	}
> > > >
> > > > Perhaps you can take inspiration from devcd_readv() here?
> > > >
> > > > > +
> > > > > +	while (bytes_left) {
> > > > > +		addr = resolve_addr(offset - header_size,
> > > > > +				    &rproc->dump_segments, &data_left);
> > > > > +		/* EOF check */
> > > > > +		if (data_left == 0) {
> > > >
> > > > Afaict data_left denotes the amount of data left in this particular
> > > > segment, rather than in the entire core.
> > > >
> > > Yes, but it only returns 0 when the final segment has been copied
> > > completely.  Otherwise it gives data left to copy for every segment
> > > and moves to next segment once the current one is copied.
> > 
> > You're right.
> > 
> > > > I think you should start by making bytes_left the minimum of the core
> > > > size and @count and then have this loop as long as bytes_left, copying
> > > > data to the buffer either from header or an appropriate segment based on
> > > > the current offset.
> > > >
> > > That would require an extra function that calculates entire core size,
> > > as its not available right now. Do you see any missed corner cases
> > > with this
> > > approach?
> > 
> > You're looping over all the segments as you're building the header
> > anyways, so you could simply store this in the dump_state. I think this
> > depend more on the ability to reuse the read function between inline and
> > default coredump.
> > 
> > Regards,
> > Bjorn
> 
> Wouldn't the first if condition take care of "default" dump as it is?
> The header_size in that case would involve the 'header + all segments'.

Correct.

Regards,
Bjorn