From patchwork Fri Mar 27 14:11:50 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Stuart Haslam X-Patchwork-Id: 46427 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-wi0-f198.google.com (mail-wi0-f198.google.com [209.85.212.198]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 328DC21597 for ; Fri, 27 Mar 2015 14:12:08 +0000 (UTC) Received: by wibdy1 with SMTP id dy1sf4985028wib.3 for ; Fri, 27 Mar 2015 07:12:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:from:to:cc:subject:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe :content-type:errors-to:sender:x-original-sender :x-original-authentication-results:mailing-list; bh=5yuOw19BPbpwqOIGK8lQlzEUytAe6Uo2BymTpXauSpA=; b=evrwLR6KrUr2w1VIfHV5Z81Lcy9WINU4uQ2p9zoto9L/KPLvf9hjN6Iec58c8eCQ6H orZRpKy+nOCdMtOfmcdOiGYFQXpcvLsbEdVZSIXcM8VKlWbSF34lGZV/nALGVdgaXVTr s3FxPe/pPYOBctyXpmfvZQKoLNn8yv4WqA8uwIvgRxy3Ut5HVZ1j+Ez6NswbZUSxmnBq 5x8j6wSDTgMR3/R3oYBWeEg9c8r0lN07nMvV0MvBJKHfgZP28VQk3UldVYiIoN6F/S8C glIl7lWmINDrXhPmilbtuz1aGq/nPqCjhXkC7A1mHtp0l9tewcNvEbt7zCoAtPaHcRq4 IXoQ== X-Gm-Message-State: ALoCoQlBmTsCDcqeXbPF8qjnEpC1Zweh/p9O9TwHhfTOveyOE0ekXFWpsS6IX5RovxIBdu8DOpaE X-Received: by 10.194.178.67 with SMTP id cw3mr4722929wjc.2.1427465527485; Fri, 27 Mar 2015 07:12:07 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.204.69 with SMTP id kw5ls348376lac.86.gmail; Fri, 27 Mar 2015 07:12:07 -0700 (PDT) X-Received: by 10.112.114.164 with SMTP id jh4mr18025566lbb.21.1427465527291; Fri, 27 Mar 2015 07:12:07 -0700 (PDT) Received: from mail-la0-f41.google.com (mail-la0-f41.google.com. [209.85.215.41]) by mx.google.com with ESMTPS id j9si1487395lbc.32.2015.03.27.07.12.07 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 27 Mar 2015 07:12:07 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.41 as permitted sender) client-ip=209.85.215.41; Received: by labe2 with SMTP id e2so71240642lab.3 for ; Fri, 27 Mar 2015 07:12:07 -0700 (PDT) X-Received: by 10.112.46.74 with SMTP id t10mr17693016lbm.73.1427465527139; Fri, 27 Mar 2015 07:12:07 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.57.201 with SMTP id k9csp1286013lbq; Fri, 27 Mar 2015 07:12:06 -0700 (PDT) X-Received: by 10.55.20.5 with SMTP id e5mr39754974qkh.43.1427465525576; Fri, 27 Mar 2015 07:12:05 -0700 (PDT) Received: from ip-10-35-177-41.ec2.internal (lists.linaro.org. [54.225.227.206]) by mx.google.com with ESMTPS id hi9si1973679qcb.46.2015.03.27.07.12.04 (version=TLSv1 cipher=RC4-SHA bits=128/128); Fri, 27 Mar 2015 07:12:05 -0700 (PDT) Received-SPF: none (google.com: lng-odp-bounces@lists.linaro.org does not designate permitted sender hosts) client-ip=54.225.227.206; Received: from localhost ([127.0.0.1] helo=ip-10-35-177-41.ec2.internal) by ip-10-35-177-41.ec2.internal with esmtp (Exim 4.76) (envelope-from ) id 1YbUzR-0004od-VM; Fri, 27 Mar 2015 14:12:01 +0000 Received: from mail-ob0-f173.google.com ([209.85.214.173]) by ip-10-35-177-41.ec2.internal with esmtp (Exim 4.76) (envelope-from ) id 1YbUzN-0004oW-0p for lng-odp@lists.linaro.org; Fri, 27 Mar 2015 14:11:57 +0000 Received: by obbgh1 with SMTP id gh1so9618002obb.1 for ; Fri, 27 Mar 2015 07:11:50 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.202.231.85 with SMTP id e82mr82054oih.104.1427465510818; Fri, 27 Mar 2015 07:11:50 -0700 (PDT) Received: by 10.202.72.204 with HTTP; Fri, 27 Mar 2015 07:11:50 -0700 (PDT) In-Reply-To: References: <1427392347-22709-1-git-send-email-stuart.haslam@linaro.org> Date: Fri, 27 Mar 2015 14:11:50 +0000 Message-ID: From: Stuart Haslam To: Bill Fischofer X-Topics: patch Cc: LNG ODP Mailman List Subject: Re: [lng-odp] [PATCH] validation: pktio: fix memory corruption X-BeenThere: lng-odp@lists.linaro.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Errors-To: lng-odp-bounces@lists.linaro.org Sender: lng-odp-bounces@lists.linaro.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: stuart.haslam@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.41 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 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. On 27 March 2015 at 13:15, Bill Fischofer 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 > wrote: > >> >> >> On 26 March 2015 at 17:14, Bill Fischofer >> 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 >>>> --- >>>> 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 *│ *Open source software for ARM SoCs >> >> >> > 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,