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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> >> wrote: >> >>> >>> >>> On 26 March 2015 at 17:14, Bill Fischofer <[email protected]> >>> 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 < >>>> [email protected]> 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 <[email protected]> >>>>> --- >>>>> 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 >>>>> [email protected] >>>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>>>> >>>> >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> [email protected] >>>> 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 [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
