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
