| Message ID | 20180525104128.7990-7-ramon.fried@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Iotrace improvements | expand |
Hi Ramon, On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote: > When the buffer is full, there supposed to be no more > writes, the code however misses the else statement and > subsequently writes to arbitrary pointer location and increases > the offset. I don't think so. It writes to a local variable in this case. The point of this is to detect how much space would be needed to hold the I/O trace. Unless the pointer is incremented, there is no way to know. Perhaps instead, iotrace_get_buffer() should be updated to also return the number of valid records, as well as the pointer value? > This patch fixes that by returning immediately. > > Signed-off-by: Ramon Fried <ramon.fried@gmail.com> > --- > common/iotrace.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/common/iotrace.c b/common/iotrace.c > index 74408a5dbb..5f06d2b250 100644 > --- a/common/iotrace.c > +++ b/common/iotrace.c > @@ -55,6 +55,8 @@ static void add_record(int flags, const void *ptr, ulong value) > rec = (struct iotrace_record *)map_sysmem( > iotrace.start + iotrace.offset, > sizeof(value)); > + } else { > + return; > } > > rec->timestamp = get_ticks(); > -- > 2.17.0 > Regards, Simon
On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Ramon, > > On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote: >> When the buffer is full, there supposed to be no more >> writes, the code however misses the else statement and >> subsequently writes to arbitrary pointer location and increases >> the offset. > > I don't think so. It writes to a local variable in this case. The > point of this is to detect how much space would be needed to hold the > I/O trace. Unless the pointer is incremented, there is no way to know. You're right. I missed the initial assignment to rec. > > Perhaps instead, iotrace_get_buffer() should be updated to also return > the number of valid records, as well as the pointer value? > It's a valid option, another one I have in mind is that we can return immediately like I suggested but add one time warning that states that the buffer is full ? >> This patch fixes that by returning immediately. >> >> Signed-off-by: Ramon Fried <ramon.fried@gmail.com> >> --- >> common/iotrace.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/common/iotrace.c b/common/iotrace.c >> index 74408a5dbb..5f06d2b250 100644 >> --- a/common/iotrace.c >> +++ b/common/iotrace.c >> @@ -55,6 +55,8 @@ static void add_record(int flags, const void *ptr, ulong value) >> rec = (struct iotrace_record *)map_sysmem( >> iotrace.start + iotrace.offset, >> sizeof(value)); >> + } else { >> + return; >> } >> >> rec->timestamp = get_ticks(); >> -- >> 2.17.0 >> > > Regards, > Simon
Hi Ramon, On 26 May 2018 at 00:05, Ramon Fried <ramon.fried@gmail.com> wrote: > On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Ramon, >> >> On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote: >>> When the buffer is full, there supposed to be no more >>> writes, the code however misses the else statement and >>> subsequently writes to arbitrary pointer location and increases >>> the offset. >> >> I don't think so. It writes to a local variable in this case. The >> point of this is to detect how much space would be needed to hold the >> I/O trace. Unless the pointer is incremented, there is no way to know. > You're right. I missed the initial assignment to rec. > >> >> Perhaps instead, iotrace_get_buffer() should be updated to also return >> the number of valid records, as well as the pointer value? >> > It's a valid option, another one I have in mind is that > we can return immediately like I suggested but add one time warning > that states that the > buffer is full ? The problem is that people want to be able to resize the buffer so that they can try again, so they need to know the correct size. Can you make it report that it overflowed, and print the correct size? Regards, Simon
On Sun, May 27, 2018 at 1:18 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Ramon, > > On 26 May 2018 at 00:05, Ramon Fried <ramon.fried@gmail.com> wrote: >> On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Ramon, >>> >>> On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote: >>>> When the buffer is full, there supposed to be no more >>>> writes, the code however misses the else statement and >>>> subsequently writes to arbitrary pointer location and increases >>>> the offset. >>> >>> I don't think so. It writes to a local variable in this case. The >>> point of this is to detect how much space would be needed to hold the >>> I/O trace. Unless the pointer is incremented, there is no way to know. >> You're right. I missed the initial assignment to rec. >> >>> >>> Perhaps instead, iotrace_get_buffer() should be updated to also return >>> the number of valid records, as well as the pointer value? >>> >> It's a valid option, another one I have in mind is that >> we can return immediately like I suggested but add one time warning >> that states that the >> buffer is full ? > > The problem is that people want to be able to resize the buffer so > that they can try again, so they need to know the correct size. > > Can you make it report that it overflowed, and print the correct size? The correct size can only be printed in the end of the tracing. Maybe we can use WARN_ONCE when it first occurs and later when the user type "iotrace stats" it will state the required buffer size to accommodate all of the entries. What do you think ? > > Regards, > Simon
Hi Ramon, On 27 May 2018 at 02:35, Ramon Fried <ramon.fried@gmail.com> wrote: > On Sun, May 27, 2018 at 1:18 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Ramon, >> >> On 26 May 2018 at 00:05, Ramon Fried <ramon.fried@gmail.com> wrote: >>> On Sat, May 26, 2018 at 5:07 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Ramon, >>>> >>>> On 25 May 2018 at 04:41, Ramon Fried <ramon.fried@gmail.com> wrote: >>>>> When the buffer is full, there supposed to be no more >>>>> writes, the code however misses the else statement and >>>>> subsequently writes to arbitrary pointer location and increases >>>>> the offset. >>>> >>>> I don't think so. It writes to a local variable in this case. The >>>> point of this is to detect how much space would be needed to hold the >>>> I/O trace. Unless the pointer is incremented, there is no way to know. >>> You're right. I missed the initial assignment to rec. >>> >>>> >>>> Perhaps instead, iotrace_get_buffer() should be updated to also return >>>> the number of valid records, as well as the pointer value? >>>> >>> It's a valid option, another one I have in mind is that >>> we can return immediately like I suggested but add one time warning >>> that states that the >>> buffer is full ? >> >> The problem is that people want to be able to resize the buffer so >> that they can try again, so they need to know the correct size. >> >> Can you make it report that it overflowed, and print the correct size? > The correct size can only be printed in the end of the tracing. > Maybe we can use WARN_ONCE when it first occurs and later when the user > type "iotrace stats" it will state the required buffer size to > accommodate all of the > entries. > What do you think ? Sounds good to me. Regards, Simon
diff --git a/common/iotrace.c b/common/iotrace.c index 74408a5dbb..5f06d2b250 100644 --- a/common/iotrace.c +++ b/common/iotrace.c @@ -55,6 +55,8 @@ static void add_record(int flags, const void *ptr, ulong value) rec = (struct iotrace_record *)map_sysmem( iotrace.start + iotrace.offset, sizeof(value)); + } else { + return; } rec->timestamp = get_ticks();
When the buffer is full, there supposed to be no more writes, the code however misses the else statement and subsequently writes to arbitrary pointer location and increases the offset. This patch fixes that by returning immediately. Signed-off-by: Ramon Fried <ramon.fried@gmail.com> --- common/iotrace.c | 2 ++ 1 file changed, 2 insertions(+)