| Message ID | CACKbW=igGudB6aw2chz6M1PmqfupmwVXOva9HNhs0w6n=7fiTQ@mail.gmail.com |
|---|---|
| State | Accepted |
| Commit | 8f52e363292369c369fe5bee28c82f5f00f57eb6 |
| Headers | show |
On 03/27/15 17:11, Stuart Haslam wrote: > It doesn't break non-jumbo tests, in fact all tests pass and I didn't > *notice* any adverse effects, I found the issue by inspection. > > The problem is that when generating non-jumbo packets (which most of > the tests do) the magic2 marker is written beyond the end of a > calloc'd block into un-allocated memory. The receiver doesn't check > magic2 for non-jumbo packets. > > The issue could be trivially avoided with the simpler change below, > but I wasn't happy with that as we'd still have a pointer to a > structure that's much larger than the memory allocated. > > Stuart. Yes this change is ok. Did that the same as https://bugs.linaro.org/show_bug.cgi?id=1365 ? Maxim. > > diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c > index e022c33..5ea45e4 100644 > --- a/test/validation/odp_pktio.c > +++ b/test/validation/odp_pktio.c > @@ -95,7 +95,8 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) > CU_ASSERT_FATAL(data != NULL); > data->head.magic = TEST_SEQ_MAGIC; > - data->magic2 = TEST_SEQ_MAGIC; > + if (test_jumbo) > + data->magic2 = TEST_SEQ_MAGIC; > data->head.seq = tstseq; > odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, > > > On 27 March 2015 at 13:15, Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> wrote: > > If I understand Stuart's patch correctly, he's saying that the > introduction of jumbo frame support in v1.0.1 broke non-jumbo > processing. Since that's a critical function I'd think we'd want > to fix that bug ASAP in v1.0.2. > > On Fri, Mar 27, 2015 at 7:04 AM, Mike Holmes > <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> wrote: > > > > On 26 March 2015 at 17:14, Bill Fischofer > <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> > wrote: > > Sounds like we should hold v1.0.2 for this bug fix? > Maxim: I think this review is properly yours as you did > the jumbo frame add for v1.0.1. > > > Bill can you describe the use case than for holding, if we > have a strong case for it then we can hold. > Normal procedure is that whatever makes the cut is there and > the release goes out unless there is a regression introduced. > > > On Thu, Mar 26, 2015 at 12:52 PM, Stuart Haslam > <stuart.haslam@linaro.org > <mailto:stuart.haslam@linaro.org>> wrote: > > A magic number, used to ensure jumbo frames are > transmitted and received > in full, is written to the end of a packet buffer at a > fixed offset of > 9170 bytes. However for non-jumbo tests this is way > beyond the end of > the allocated buffer. > > Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org > <mailto:stuart.haslam@linaro.org>> > --- > test/validation/odp_pktio.c | 84 > ++++++++++++++++++++++++++++----------------- > 1 file changed, 52 insertions(+), 32 deletions(-) > > diff --git a/test/validation/odp_pktio.c > b/test/validation/odp_pktio.c > index e022c33..d653da4 100644 > --- a/test/validation/odp_pktio.c > +++ b/test/validation/odp_pktio.c > @@ -44,7 +44,6 @@ typedef struct ODP_PACKED { > > /** structure of test packet UDP payload */ > typedef struct ODP_PACKED { > - pkt_head_t head; > char data[PKT_BUF_JUMBO_MAX_PAYLOAD - > sizeof(pkt_head_t) - > sizeof(uint32be_t)]; > uint32be_t magic2; > @@ -74,33 +73,47 @@ static void > pktio_pkt_set_macs(odp_packet_t pkt, > > static uint32_t pkt_payload_len(void) > { > - return test_jumbo ? sizeof(pkt_test_data_t) : > sizeof(pkt_head_t); > + uint32_t len; > + > + len = sizeof(pkt_head_t); > + > + if (test_jumbo) > + len += sizeof(pkt_test_data_t); > + > + return len; > } > > static int pktio_pkt_set_seq(odp_packet_t pkt) > { > static uint32_t tstseq; > - size_t l4_off; > - pkt_test_data_t *data; > - uint32_t len = pkt_payload_len(); > - > + size_t off; > + pkt_head_t head; > > - l4_off = odp_packet_l4_offset(pkt); > - if (!l4_off) { > + off = odp_packet_l4_offset(pkt); > + if (!off) { > CU_FAIL("packet L4 offset not set"); > return -1; > } > > - data = calloc(1, len); > - CU_ASSERT_FATAL(data != NULL); > + head.magic = TEST_SEQ_MAGIC; > + head.seq = tstseq; > + > + off += ODPH_UDPHDR_LEN; > + odp_packet_copydata_in(pkt, off, sizeof(head), &head); > + > + if (test_jumbo) { > + pkt_test_data_t *data; > + > + data = calloc(1, sizeof(*data)); > + CU_ASSERT_FATAL(data != NULL); > > - data->head.magic = TEST_SEQ_MAGIC; > - data->magic2 = TEST_SEQ_MAGIC; > - data->head.seq = tstseq; > + data->magic2 = TEST_SEQ_MAGIC; > + > + off += sizeof(head); > + odp_packet_copydata_in(pkt, off, sizeof(*data), data); > + free(data); > + } > > - odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, > - len, data); > - free(data); > tstseq++; > > return 0; > @@ -108,30 +121,37 @@ static int > pktio_pkt_set_seq(odp_packet_t pkt) > > static uint32_t pktio_pkt_seq(odp_packet_t pkt) > { > - size_t l4_off; > + size_t off; > uint32_t seq = TEST_SEQ_INVALID; > - pkt_test_data_t *data; > - uint32_t len = pkt_payload_len(); > + pkt_head_t head; > > - l4_off = odp_packet_l4_offset(pkt); > - if (l4_off == ODP_PACKET_OFFSET_INVALID) > + off = odp_packet_l4_offset(pkt); > + if (off == ODP_PACKET_OFFSET_INVALID) > return TEST_SEQ_INVALID; > > - data = calloc(1, len); > - CU_ASSERT_FATAL(data != NULL); > + off += ODPH_UDPHDR_LEN; > + odp_packet_copydata_out(pkt, off, sizeof(head), &head); > > - odp_packet_copydata_out(pkt, l4_off+ODPH_UDPHDR_LEN, > - len, data); > + if (head.magic != TEST_SEQ_MAGIC) > + return TEST_SEQ_INVALID; > > - if (data->head.magic == TEST_SEQ_MAGIC) { > - if (test_jumbo && data->magic2 != > TEST_SEQ_MAGIC) { > - free(data); > - return TEST_SEQ_INVALID; > - } > - seq = data->head.seq; > + seq = head.seq; > + > + if (test_jumbo) { > + pkt_test_data_t *data; > + > + data = calloc(1, sizeof(*data)); > + CU_ASSERT_FATAL(data != NULL); > + > + off += sizeof(head); > + odp_packet_copydata_out(pkt, off, sizeof(*data), data); > + > + if (data->magic2 != TEST_SEQ_MAGIC) > + seq = TEST_SEQ_INVALID; > + > + free(data); > } > > - free(data); > return seq; > } > > -- > 2.1.1 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software > for ARM SoCs > > > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On Fri, Mar 27, 2015 at 05:15:43PM +0300, Maxim Uvarov wrote: > On 03/27/15 17:11, Stuart Haslam wrote: > >It doesn't break non-jumbo tests, in fact all tests pass and I > >didn't *notice* any adverse effects, I found the issue by > >inspection. > > > >The problem is that when generating non-jumbo packets (which most > >of the tests do) the magic2 marker is written beyond the end of a > >calloc'd block into un-allocated memory. The receiver doesn't > >check magic2 for non-jumbo packets. > > > >The issue could be trivially avoided with the simpler change > >below, but I wasn't happy with that as we'd still have a pointer > >to a structure that's much larger than the memory allocated. > > > >Stuart. > > Yes this change is ok. Did that the same as > https://bugs.linaro.org/show_bug.cgi?id=1365 > ? > > Maxim. Bug 1365 (MMSG lock up) is a different issue, I'm working on that now.
Silent memory corruption is a problem even if specific tests don't reveal it. Due to the pool memory layout, the corruption is writing something into some other buffer, which may or may not be allocated. That's always a recipe for sudden and confusing failures so I'd view this as a critical bug. On Fri, Mar 27, 2015 at 9:11 AM, Stuart Haslam <stuart.haslam@linaro.org> wrote: > It doesn't break non-jumbo tests, in fact all tests pass and I didn't > *notice* any adverse effects, I found the issue by inspection. > > The problem is that when generating non-jumbo packets (which most of the > tests do) the magic2 marker is written beyond the end of a calloc'd block > into un-allocated memory. The receiver doesn't check magic2 for non-jumbo > packets. > > The issue could be trivially avoided with the simpler change below, but I > wasn't happy with that as we'd still have a pointer to a structure that's > much larger than the memory allocated. > > Stuart. > > diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c > index e022c33..5ea45e4 100644 > --- a/test/validation/odp_pktio.c > +++ b/test/validation/odp_pktio.c > @@ -95,7 +95,8 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) > CU_ASSERT_FATAL(data != NULL); > > data->head.magic = TEST_SEQ_MAGIC; > - data->magic2 = TEST_SEQ_MAGIC; > + if (test_jumbo) > + data->magic2 = TEST_SEQ_MAGIC; > data->head.seq = tstseq; > > odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, > > > On 27 March 2015 at 13:15, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> If I understand Stuart's patch correctly, he's saying that the >> introduction of jumbo frame support in v1.0.1 broke non-jumbo processing. >> Since that's a critical function I'd think we'd want to fix that bug ASAP >> in v1.0.2. >> >> On Fri, Mar 27, 2015 at 7:04 AM, Mike Holmes <mike.holmes@linaro.org> >> wrote: >> >>> >>> >>> On 26 March 2015 at 17:14, Bill Fischofer <bill.fischofer@linaro.org> >>> wrote: >>> >>>> Sounds like we should hold v1.0.2 for this bug fix? Maxim: I think >>>> this review is properly yours as you did the jumbo frame add for v1.0.1. >>>> >>> >>> Bill can you describe the use case than for holding, if we have a strong >>> case for it then we can hold. >>> >>> Normal procedure is that whatever makes the cut is there and the release >>> goes out unless there is a regression introduced. >>> >>> >>>> >>>> On Thu, Mar 26, 2015 at 12:52 PM, Stuart Haslam < >>>> stuart.haslam@linaro.org> wrote: >>>> >>>>> A magic number, used to ensure jumbo frames are transmitted and >>>>> received >>>>> in full, is written to the end of a packet buffer at a fixed offset of >>>>> 9170 bytes. However for non-jumbo tests this is way beyond the end of >>>>> the allocated buffer. >>>>> >>>>> Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> >>>>> --- >>>>> test/validation/odp_pktio.c | 84 >>>>> ++++++++++++++++++++++++++++----------------- >>>>> 1 file changed, 52 insertions(+), 32 deletions(-) >>>>> >>>>> diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c >>>>> index e022c33..d653da4 100644 >>>>> --- a/test/validation/odp_pktio.c >>>>> +++ b/test/validation/odp_pktio.c >>>>> @@ -44,7 +44,6 @@ typedef struct ODP_PACKED { >>>>> >>>>> /** structure of test packet UDP payload */ >>>>> typedef struct ODP_PACKED { >>>>> - pkt_head_t head; >>>>> char data[PKT_BUF_JUMBO_MAX_PAYLOAD - sizeof(pkt_head_t) - >>>>> sizeof(uint32be_t)]; >>>>> uint32be_t magic2; >>>>> @@ -74,33 +73,47 @@ static void pktio_pkt_set_macs(odp_packet_t pkt, >>>>> >>>>> static uint32_t pkt_payload_len(void) >>>>> { >>>>> - return test_jumbo ? sizeof(pkt_test_data_t) : >>>>> sizeof(pkt_head_t); >>>>> + uint32_t len; >>>>> + >>>>> + len = sizeof(pkt_head_t); >>>>> + >>>>> + if (test_jumbo) >>>>> + len += sizeof(pkt_test_data_t); >>>>> + >>>>> + return len; >>>>> } >>>>> >>>>> static int pktio_pkt_set_seq(odp_packet_t pkt) >>>>> { >>>>> static uint32_t tstseq; >>>>> - size_t l4_off; >>>>> - pkt_test_data_t *data; >>>>> - uint32_t len = pkt_payload_len(); >>>>> - >>>>> + size_t off; >>>>> + pkt_head_t head; >>>>> >>>>> - l4_off = odp_packet_l4_offset(pkt); >>>>> - if (!l4_off) { >>>>> + off = odp_packet_l4_offset(pkt); >>>>> + if (!off) { >>>>> CU_FAIL("packet L4 offset not set"); >>>>> return -1; >>>>> } >>>>> >>>>> - data = calloc(1, len); >>>>> - CU_ASSERT_FATAL(data != NULL); >>>>> + head.magic = TEST_SEQ_MAGIC; >>>>> + head.seq = tstseq; >>>>> + >>>>> + off += ODPH_UDPHDR_LEN; >>>>> + odp_packet_copydata_in(pkt, off, sizeof(head), &head); >>>>> + >>>>> + if (test_jumbo) { >>>>> + pkt_test_data_t *data; >>>>> + >>>>> + data = calloc(1, sizeof(*data)); >>>>> + CU_ASSERT_FATAL(data != NULL); >>>>> >>>>> - data->head.magic = TEST_SEQ_MAGIC; >>>>> - data->magic2 = TEST_SEQ_MAGIC; >>>>> - data->head.seq = tstseq; >>>>> + data->magic2 = TEST_SEQ_MAGIC; >>>>> + >>>>> + off += sizeof(head); >>>>> + odp_packet_copydata_in(pkt, off, sizeof(*data), data); >>>>> + free(data); >>>>> + } >>>>> >>>>> - odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, >>>>> - len, data); >>>>> - free(data); >>>>> tstseq++; >>>>> >>>>> return 0; >>>>> @@ -108,30 +121,37 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) >>>>> >>>>> static uint32_t pktio_pkt_seq(odp_packet_t pkt) >>>>> { >>>>> - size_t l4_off; >>>>> + size_t off; >>>>> uint32_t seq = TEST_SEQ_INVALID; >>>>> - pkt_test_data_t *data; >>>>> - uint32_t len = pkt_payload_len(); >>>>> + pkt_head_t head; >>>>> >>>>> - l4_off = odp_packet_l4_offset(pkt); >>>>> - if (l4_off == ODP_PACKET_OFFSET_INVALID) >>>>> + off = odp_packet_l4_offset(pkt); >>>>> + if (off == ODP_PACKET_OFFSET_INVALID) >>>>> return TEST_SEQ_INVALID; >>>>> >>>>> - data = calloc(1, len); >>>>> - CU_ASSERT_FATAL(data != NULL); >>>>> + off += ODPH_UDPHDR_LEN; >>>>> + odp_packet_copydata_out(pkt, off, sizeof(head), &head); >>>>> >>>>> - odp_packet_copydata_out(pkt, l4_off+ODPH_UDPHDR_LEN, >>>>> - len, data); >>>>> + if (head.magic != TEST_SEQ_MAGIC) >>>>> + return TEST_SEQ_INVALID; >>>>> >>>>> - if (data->head.magic == TEST_SEQ_MAGIC) { >>>>> - if (test_jumbo && data->magic2 != TEST_SEQ_MAGIC) { >>>>> - free(data); >>>>> - return TEST_SEQ_INVALID; >>>>> - } >>>>> - seq = data->head.seq; >>>>> + seq = head.seq; >>>>> + >>>>> + if (test_jumbo) { >>>>> + pkt_test_data_t *data; >>>>> + >>>>> + data = calloc(1, sizeof(*data)); >>>>> + CU_ASSERT_FATAL(data != NULL); >>>>> + >>>>> + off += sizeof(head); >>>>> + odp_packet_copydata_out(pkt, off, sizeof(*data), data); >>>>> + >>>>> + if (data->magic2 != TEST_SEQ_MAGIC) >>>>> + seq = TEST_SEQ_INVALID; >>>>> + >>>>> + free(data); >>>>> } >>>>> >>>>> - free(data); >>>>> return seq; >>>>> } >>>>> >>>>> -- >>>>> 2.1.1 >>>>> >>>>> >>>>> _______________________________________________ >>>>> lng-odp mailing list >>>>> lng-odp@lists.linaro.org >>>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>>> >>>> >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> lng-odp@lists.linaro.org >>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>> >>>> >>> >>> >>> -- >>> Mike Holmes >>> Technical Manager - Linaro Networking Group >>> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM >>> SoCs >>> >>> >>> >> >
ODP agreed that only critical bugs hold the release at the last minuet, this issue is not even a bug at this point - it should be so that users are aware of the issue. If this really is affecting a critical product a bug should be created with the rational for this to be in 1.0.2 and we can break our rules and work overtime testing it again - last minuet changes require effort. In this case only local developer tests might be affected - no one has noticed this yet, the fix is available on the list, it is likely to be in master on Monday and will be in 1.0.3 in two weeks, given that I cant see the urgency yet. I agree we want the fix in, but this has not even been on the list 24 hours which is a second gating item for ODP. On 27 March 2015 at 10:33, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Silent memory corruption is a problem even if specific tests don't reveal > it. Due to the pool memory layout, the corruption is writing something > into some other buffer, which may or may not be allocated. That's always a > recipe for sudden and confusing failures so I'd view this as a critical bug. > > On Fri, Mar 27, 2015 at 9:11 AM, Stuart Haslam <stuart.haslam@linaro.org> > wrote: > >> It doesn't break non-jumbo tests, in fact all tests pass and I didn't >> *notice* any adverse effects, I found the issue by inspection. >> >> The problem is that when generating non-jumbo packets (which most of the >> tests do) the magic2 marker is written beyond the end of a calloc'd block >> into un-allocated memory. The receiver doesn't check magic2 for non-jumbo >> packets. >> >> The issue could be trivially avoided with the simpler change below, but I >> wasn't happy with that as we'd still have a pointer to a structure that's >> much larger than the memory allocated. >> >> Stuart. >> >> diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c >> index e022c33..5ea45e4 100644 >> --- a/test/validation/odp_pktio.c >> +++ b/test/validation/odp_pktio.c >> @@ -95,7 +95,8 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) >> CU_ASSERT_FATAL(data != NULL); >> >> data->head.magic = TEST_SEQ_MAGIC; >> - data->magic2 = TEST_SEQ_MAGIC; >> + if (test_jumbo) >> + data->magic2 = TEST_SEQ_MAGIC; >> data->head.seq = tstseq; >> >> odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, >> >> >> On 27 March 2015 at 13:15, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >>> If I understand Stuart's patch correctly, he's saying that the >>> introduction of jumbo frame support in v1.0.1 broke non-jumbo processing. >>> Since that's a critical function I'd think we'd want to fix that bug ASAP >>> in v1.0.2. >>> >>> On Fri, Mar 27, 2015 at 7:04 AM, Mike Holmes <mike.holmes@linaro.org> >>> wrote: >>> >>>> >>>> >>>> On 26 March 2015 at 17:14, Bill Fischofer <bill.fischofer@linaro.org> >>>> wrote: >>>> >>>>> Sounds like we should hold v1.0.2 for this bug fix? Maxim: I think >>>>> this review is properly yours as you did the jumbo frame add for v1.0.1. >>>>> >>>> >>>> Bill can you describe the use case than for holding, if we have a >>>> strong case for it then we can hold. >>>> >>>> Normal procedure is that whatever makes the cut is there and the >>>> release goes out unless there is a regression introduced. >>>> >>>> >>>>> >>>>> On Thu, Mar 26, 2015 at 12:52 PM, Stuart Haslam < >>>>> stuart.haslam@linaro.org> wrote: >>>>> >>>>>> A magic number, used to ensure jumbo frames are transmitted and >>>>>> received >>>>>> in full, is written to the end of a packet buffer at a fixed offset of >>>>>> 9170 bytes. However for non-jumbo tests this is way beyond the end of >>>>>> the allocated buffer. >>>>>> >>>>>> Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org> >>>>>> --- >>>>>> test/validation/odp_pktio.c | 84 >>>>>> ++++++++++++++++++++++++++++----------------- >>>>>> 1 file changed, 52 insertions(+), 32 deletions(-) >>>>>> >>>>>> diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c >>>>>> index e022c33..d653da4 100644 >>>>>> --- a/test/validation/odp_pktio.c >>>>>> +++ b/test/validation/odp_pktio.c >>>>>> @@ -44,7 +44,6 @@ typedef struct ODP_PACKED { >>>>>> >>>>>> /** structure of test packet UDP payload */ >>>>>> typedef struct ODP_PACKED { >>>>>> - pkt_head_t head; >>>>>> char data[PKT_BUF_JUMBO_MAX_PAYLOAD - sizeof(pkt_head_t) - >>>>>> sizeof(uint32be_t)]; >>>>>> uint32be_t magic2; >>>>>> @@ -74,33 +73,47 @@ static void pktio_pkt_set_macs(odp_packet_t pkt, >>>>>> >>>>>> static uint32_t pkt_payload_len(void) >>>>>> { >>>>>> - return test_jumbo ? sizeof(pkt_test_data_t) : >>>>>> sizeof(pkt_head_t); >>>>>> + uint32_t len; >>>>>> + >>>>>> + len = sizeof(pkt_head_t); >>>>>> + >>>>>> + if (test_jumbo) >>>>>> + len += sizeof(pkt_test_data_t); >>>>>> + >>>>>> + return len; >>>>>> } >>>>>> >>>>>> static int pktio_pkt_set_seq(odp_packet_t pkt) >>>>>> { >>>>>> static uint32_t tstseq; >>>>>> - size_t l4_off; >>>>>> - pkt_test_data_t *data; >>>>>> - uint32_t len = pkt_payload_len(); >>>>>> - >>>>>> + size_t off; >>>>>> + pkt_head_t head; >>>>>> >>>>>> - l4_off = odp_packet_l4_offset(pkt); >>>>>> - if (!l4_off) { >>>>>> + off = odp_packet_l4_offset(pkt); >>>>>> + if (!off) { >>>>>> CU_FAIL("packet L4 offset not set"); >>>>>> return -1; >>>>>> } >>>>>> >>>>>> - data = calloc(1, len); >>>>>> - CU_ASSERT_FATAL(data != NULL); >>>>>> + head.magic = TEST_SEQ_MAGIC; >>>>>> + head.seq = tstseq; >>>>>> + >>>>>> + off += ODPH_UDPHDR_LEN; >>>>>> + odp_packet_copydata_in(pkt, off, sizeof(head), &head); >>>>>> + >>>>>> + if (test_jumbo) { >>>>>> + pkt_test_data_t *data; >>>>>> + >>>>>> + data = calloc(1, sizeof(*data)); >>>>>> + CU_ASSERT_FATAL(data != NULL); >>>>>> >>>>>> - data->head.magic = TEST_SEQ_MAGIC; >>>>>> - data->magic2 = TEST_SEQ_MAGIC; >>>>>> - data->head.seq = tstseq; >>>>>> + data->magic2 = TEST_SEQ_MAGIC; >>>>>> + >>>>>> + off += sizeof(head); >>>>>> + odp_packet_copydata_in(pkt, off, sizeof(*data), data); >>>>>> + free(data); >>>>>> + } >>>>>> >>>>>> - odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, >>>>>> - len, data); >>>>>> - free(data); >>>>>> tstseq++; >>>>>> >>>>>> return 0; >>>>>> @@ -108,30 +121,37 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) >>>>>> >>>>>> static uint32_t pktio_pkt_seq(odp_packet_t pkt) >>>>>> { >>>>>> - size_t l4_off; >>>>>> + size_t off; >>>>>> uint32_t seq = TEST_SEQ_INVALID; >>>>>> - pkt_test_data_t *data; >>>>>> - uint32_t len = pkt_payload_len(); >>>>>> + pkt_head_t head; >>>>>> >>>>>> - l4_off = odp_packet_l4_offset(pkt); >>>>>> - if (l4_off == ODP_PACKET_OFFSET_INVALID) >>>>>> + off = odp_packet_l4_offset(pkt); >>>>>> + if (off == ODP_PACKET_OFFSET_INVALID) >>>>>> return TEST_SEQ_INVALID; >>>>>> >>>>>> - data = calloc(1, len); >>>>>> - CU_ASSERT_FATAL(data != NULL); >>>>>> + off += ODPH_UDPHDR_LEN; >>>>>> + odp_packet_copydata_out(pkt, off, sizeof(head), &head); >>>>>> >>>>>> - odp_packet_copydata_out(pkt, l4_off+ODPH_UDPHDR_LEN, >>>>>> - len, data); >>>>>> + if (head.magic != TEST_SEQ_MAGIC) >>>>>> + return TEST_SEQ_INVALID; >>>>>> >>>>>> - if (data->head.magic == TEST_SEQ_MAGIC) { >>>>>> - if (test_jumbo && data->magic2 != TEST_SEQ_MAGIC) { >>>>>> - free(data); >>>>>> - return TEST_SEQ_INVALID; >>>>>> - } >>>>>> - seq = data->head.seq; >>>>>> + seq = head.seq; >>>>>> + >>>>>> + if (test_jumbo) { >>>>>> + pkt_test_data_t *data; >>>>>> + >>>>>> + data = calloc(1, sizeof(*data)); >>>>>> + CU_ASSERT_FATAL(data != NULL); >>>>>> + >>>>>> + off += sizeof(head); >>>>>> + odp_packet_copydata_out(pkt, off, sizeof(*data), >>>>>> data); >>>>>> + >>>>>> + if (data->magic2 != TEST_SEQ_MAGIC) >>>>>> + seq = TEST_SEQ_INVALID; >>>>>> + >>>>>> + free(data); >>>>>> } >>>>>> >>>>>> - free(data); >>>>>> return seq; >>>>>> } >>>>>> >>>>>> -- >>>>>> 2.1.1 >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> lng-odp mailing list >>>>>> lng-odp@lists.linaro.org >>>>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> lng-odp mailing list >>>>> lng-odp@lists.linaro.org >>>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>>> >>>>> >>>> >>>> >>>> -- >>>> Mike Holmes >>>> Technical Manager - Linaro Networking Group >>>> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM >>>> SoCs >>>> >>>> >>>> >>> >> >
On 03/27/15 19:21, Mike Holmes wrote: > ODP agreed that only critical bugs hold the release at the last > minuet, this issue is not even a bug at this point - it should be so > that users are aware of the issue. > > If this really is affecting a critical product a bug should be created > with the rational for this to be in 1.0.2 and we can break our rules > and work overtime testing it again - last minuet changes require effort. > > In this case only local developer tests might be affected - no one has > noticed this yet, the fix is available on the list, it is likely to be > in master on Monday and will be in 1.0.3 in two weeks, given that I > cant see the urgency yet. > > I agree we want the fix in, but this has not even been on the list 24 > hours which is a second gating item for ODP. > +1 no rush with such changes. That will be in 1.0.3 Maxim. > > > > On 27 March 2015 at 10:33, Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> wrote: > > Silent memory corruption is a problem even if specific tests don't > reveal it. Due to the pool memory layout, the corruption is > writing something into some other buffer, which may or may not be > allocated. That's always a recipe for sudden and confusing > failures so I'd view this as a critical bug. > > On Fri, Mar 27, 2015 at 9:11 AM, Stuart Haslam > <stuart.haslam@linaro.org <mailto:stuart.haslam@linaro.org>> wrote: > > It doesn't break non-jumbo tests, in fact all tests pass and I > didn't *notice* any adverse effects, I found the issue by > inspection. > > The problem is that when generating non-jumbo packets (which > most of the tests do) the magic2 marker is written beyond the > end of a calloc'd block into un-allocated memory. The receiver > doesn't check magic2 for non-jumbo packets. > > The issue could be trivially avoided with the simpler change > below, but I wasn't happy with that as we'd still have a > pointer to a structure that's much larger than the memory > allocated. > > Stuart. > > diff --git a/test/validation/odp_pktio.c > b/test/validation/odp_pktio.c > index e022c33..5ea45e4 100644 > --- a/test/validation/odp_pktio.c > +++ b/test/validation/odp_pktio.c > @@ -95,7 +95,8 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) > CU_ASSERT_FATAL(data != NULL); > data->head.magic = TEST_SEQ_MAGIC; > - data->magic2 = TEST_SEQ_MAGIC; > + if (test_jumbo) > + data->magic2 = TEST_SEQ_MAGIC; > data->head.seq = tstseq; > odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN, > > > On 27 March 2015 at 13:15, Bill Fischofer > <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> > wrote: > > If I understand Stuart's patch correctly, he's saying that > the introduction of jumbo frame support in v1.0.1 broke > non-jumbo processing. Since that's a critical function I'd > think we'd want to fix that bug ASAP in v1.0.2. > > On Fri, Mar 27, 2015 at 7:04 AM, Mike Holmes > <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> > wrote: > > > > On 26 March 2015 at 17:14, Bill Fischofer > <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> wrote: > > Sounds like we should hold v1.0.2 for this bug > fix? Maxim: I think this review is properly yours > as you did the jumbo frame add for v1.0.1. > > > Bill can you describe the use case than for holding, > if we have a strong case for it then we can hold. > Normal procedure is that whatever makes the cut is > there and the release goes out unless there is a > regression introduced. > > > On Thu, Mar 26, 2015 at 12:52 PM, Stuart Haslam > <stuart.haslam@linaro.org > <mailto:stuart.haslam@linaro.org>> wrote: > > A magic number, used to ensure jumbo frames > are transmitted and received > in full, is written to the end of a packet > buffer at a fixed offset of > 9170 bytes. However for non-jumbo tests this > is way beyond the end of > the allocated buffer. > > Signed-off-by: Stuart Haslam > <stuart.haslam@linaro.org > <mailto:stuart.haslam@linaro.org>> > --- > test/validation/odp_pktio.c | 84 > ++++++++++++++++++++++++++++----------------- > 1 file changed, 52 insertions(+), 32 deletions(-) > > diff --git a/test/validation/odp_pktio.c > b/test/validation/odp_pktio.c > index e022c33..d653da4 100644 > --- a/test/validation/odp_pktio.c > +++ b/test/validation/odp_pktio.c > @@ -44,7 +44,6 @@ typedef struct ODP_PACKED { > > /** structure of test packet UDP payload */ > typedef struct ODP_PACKED { > - pkt_head_t head; > char data[PKT_BUF_JUMBO_MAX_PAYLOAD - > sizeof(pkt_head_t) - > sizeof(uint32be_t)]; > uint32be_t magic2; > @@ -74,33 +73,47 @@ static void > pktio_pkt_set_macs(odp_packet_t pkt, > > static uint32_t pkt_payload_len(void) > { > - return test_jumbo ? > sizeof(pkt_test_data_t) : sizeof(pkt_head_t); > + uint32_t len; > + > + len = sizeof(pkt_head_t); > + > + if (test_jumbo) > + len += sizeof(pkt_test_data_t); > + > + return len; > } > > static int pktio_pkt_set_seq(odp_packet_t pkt) > { > static uint32_t tstseq; > - size_t l4_off; > - pkt_test_data_t *data; > - uint32_t len = pkt_payload_len(); > - > + size_t off; > + pkt_head_t head; > > - l4_off = odp_packet_l4_offset(pkt); > - if (!l4_off) { > + off = odp_packet_l4_offset(pkt); > + if (!off) { > CU_FAIL("packet L4 offset not set"); > return -1; > } > > - data = calloc(1, len); > - CU_ASSERT_FATAL(data != NULL); > + head.magic = TEST_SEQ_MAGIC; > + head.seq = tstseq; > + > + off += ODPH_UDPHDR_LEN; > + odp_packet_copydata_in(pkt, off, > sizeof(head), &head); > + > + if (test_jumbo) { > + pkt_test_data_t *data; > + > + data = calloc(1, sizeof(*data)); > + CU_ASSERT_FATAL(data != NULL); > > - data->head.magic = TEST_SEQ_MAGIC; > - data->magic2 = TEST_SEQ_MAGIC; > - data->head.seq = tstseq; > + data->magic2 = TEST_SEQ_MAGIC; > + > + off += sizeof(head); > + odp_packet_copydata_in(pkt, off, > sizeof(*data), data); > + free(data); > + } > > - odp_packet_copydata_in(pkt, > l4_off+ODPH_UDPHDR_LEN, > - len, data); > - free(data); > tstseq++; > > return 0; > @@ -108,30 +121,37 @@ static int > pktio_pkt_set_seq(odp_packet_t pkt) > > static uint32_t pktio_pkt_seq(odp_packet_t pkt) > { > - size_t l4_off; > + size_t off; > uint32_t seq = TEST_SEQ_INVALID; > - pkt_test_data_t *data; > - uint32_t len = pkt_payload_len(); > + pkt_head_t head; > > - l4_off = odp_packet_l4_offset(pkt); > - if (l4_off == ODP_PACKET_OFFSET_INVALID) > + off = odp_packet_l4_offset(pkt); > + if (off == ODP_PACKET_OFFSET_INVALID) > return TEST_SEQ_INVALID; > > - data = calloc(1, len); > - CU_ASSERT_FATAL(data != NULL); > + off += ODPH_UDPHDR_LEN; > + odp_packet_copydata_out(pkt, off, > sizeof(head), &head); > > - odp_packet_copydata_out(pkt, > l4_off+ODPH_UDPHDR_LEN, > - len, data); > + if (head.magic != TEST_SEQ_MAGIC) > + return TEST_SEQ_INVALID; > > - if (data->head.magic == TEST_SEQ_MAGIC) { > - if (test_jumbo && data->magic2 != > TEST_SEQ_MAGIC) { > - free(data); > - return TEST_SEQ_INVALID; > - } > - seq = data->head.seq; > + seq = head.seq; > + > + if (test_jumbo) { > + pkt_test_data_t *data; > + > + data = calloc(1, sizeof(*data)); > + CU_ASSERT_FATAL(data != NULL); > + > + off += sizeof(head); > + odp_packet_copydata_out(pkt, off, > sizeof(*data), data); > + > + if (data->magic2 != TEST_SEQ_MAGIC) > + seq = TEST_SEQ_INVALID; > + > + free(data); > } > > - free(data); > return seq; > } > > -- > 2.1.1 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source > software for ARM SoCs > > > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On Fri, Mar 27, 2015 at 07:26:36PM +0300, Maxim Uvarov wrote: > +1 no rush with such changes. That will be in 1.0.3 > > Maxim. I needed to make some further changes to this area to test the EMSGSIZE problem, so I've included a different version of this fix + rework in the series I just sent (https://patches.linaro.org/46721/ and https://patches.linaro.org/46722/). So please look to merge that series (once reviewed) rather than this patch.
diff --git a/test/validation/odp_pktio.c b/test/validation/odp_pktio.c index e022c33..5ea45e4 100644 --- a/test/validation/odp_pktio.c +++ b/test/validation/odp_pktio.c @@ -95,7 +95,8 @@ static int pktio_pkt_set_seq(odp_packet_t pkt) CU_ASSERT_FATAL(data != NULL); data->head.magic = TEST_SEQ_MAGIC; - data->magic2 = TEST_SEQ_MAGIC; + if (test_jumbo) + data->magic2 = TEST_SEQ_MAGIC; data->head.seq = tstseq; odp_packet_copydata_in(pkt, l4_off+ODPH_UDPHDR_LEN,