From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7D1BBA0613 for ; Tue, 30 Jul 2019 17:57:05 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4A9601C032; Tue, 30 Jul 2019 17:57:05 +0200 (CEST) Received: from mail-pl1-f194.google.com (mail-pl1-f194.google.com [209.85.214.194]) by dpdk.org (Postfix) with ESMTP id 0DE301BFF7 for ; Tue, 30 Jul 2019 17:57:04 +0200 (CEST) Received: by mail-pl1-f194.google.com with SMTP id b3so29094780plr.4 for ; Tue, 30 Jul 2019 08:57:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=OGwUEnT+JIZP4harzf26oJP7cyWZtrvEiG+C8tpjdOU=; b=YlDeLfj3SUOJEr/2JgcPDqiXL8ucCDSd+FJPNCAQ8tud2+2232hOdY4cOA3+bS979J A6i8E8r8mXuj/BsDw+i23pZZkdDyxV3NzlBg27NVYw1qFUmd/IjVlTTLs67pocX4abZd oMPZd92j7g1qMs3Cm7MG9DCw/rA8/pVTi3FRV4dWQwIjF81iGxTgdHoXwaDmAz0+P4Y7 qPlwnor39yOfS2EDKnajRRARFILBHB0xdgwZCUnrv49VgyenPNQnYCRKE8iCPUExvFQF 1KaGHYK6n2jc+HCTxDrM4zuoQk/Oxf7T+nY4icbf9rQeOtJrnJR8IYCSkS78gHtHL8gh MVXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=OGwUEnT+JIZP4harzf26oJP7cyWZtrvEiG+C8tpjdOU=; b=abvl2J+sQDwGrG/IqqO8XkRVnFdhHOykWd2N8bKIAS+OnEA568oyMY4aUsy+WtK0fl 4N1/KFOuf1SexTVOykAKDkUD+QAGJAJ/kopH6mMtmZXivUIhu+6YRSpGQiKgGwaCqEAT DlHmIhopdd8qTCEeqvZ3I1tI4sfwiOyDVxNcdm5tLYq0L5fTY/sjwxDBIn2yVys0d4A/ QxjoZnai/Her8xXqn0SdAL3OQNCbUJavuYYrv42vvt4w0Xdm2f4S/CyKgpEFn3IzQb6/ PbyXn7kLZZyZB6fsMWEMD9CdrYXihNT5cFnrcFWXSqSNJmg/9yymIpNPTxxYxC0uikSf H/+w== X-Gm-Message-State: APjAAAUHePxLoBmLtnwbkdZocqg07UJRRUP3I+S6MBBgAn7NkkwhDR1m NpHZrmQ7cJsD0vvAWfxWL8k= X-Google-Smtp-Source: APXvYqyOugMXR8wi3gJDm9q+aU8c66C/RcsJ9oPor03w5ENX4ekCZ8x5ON9+pLnvUNG2pFMxUyN8cQ== X-Received: by 2002:a17:902:f01:: with SMTP id 1mr115475407ply.170.1564502223280; Tue, 30 Jul 2019 08:57:03 -0700 (PDT) Received: from hermes.lan (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id w132sm67428087pfd.78.2019.07.30.08.57.03 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Tue, 30 Jul 2019 08:57:03 -0700 (PDT) Date: Tue, 30 Jul 2019 08:56:56 -0700 From: Stephen Hemminger To: Matan Azrad Cc: "dev@dpdk.org" , Stephen Hemminger Message-ID: <20190730085656.7c720f28@hermes.lan> In-Reply-To: References: <20190709150939.31338-1-stephen@networkplumber.org> <20190726165054.24078-1-stephen@networkplumber.org> <20190726165054.24078-2-stephen@networkplumber.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 1/4] examples/multi_process/client_server_mp: check port validity X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, 30 Jul 2019 09:21:02 +0000 Matan Azrad wrote: > Hi Stephen > > From: Stephen Hemminger > > From: Stephen Hemminger > > > > The mp_server incorrectly allows a port mask that included hidden ports and > > which later caused either lost packets or failed initialization. > > > > This fixes explicitly checking that each bit in portmask is a valid port before > > using it. > > > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership") > > Signed-off-by: Stephen Hemminger > > --- > > .../client_server_mp/mp_server/args.c | 46 +++++++++++++------ > > .../client_server_mp/mp_server/args.h | 2 +- > > .../client_server_mp/mp_server/init.c | 7 +-- > > 3 files changed, 35 insertions(+), 20 deletions(-) > > > > diff --git a/examples/multi_process/client_server_mp/mp_server/args.c > > b/examples/multi_process/client_server_mp/mp_server/args.c > > index b0d8d7665c85..c1ab12ad00d1 100644 > > --- a/examples/multi_process/client_server_mp/mp_server/args.c > > +++ b/examples/multi_process/client_server_mp/mp_server/args.c > > @@ -10,6 +10,7 @@ > > #include > > > > #include > > +#include > > #include > > > > #include "common.h" > > @@ -34,6 +35,22 @@ usage(void) > > , progname); > > } > > > > +/** > > + * Check if port is present in the system > > + * It maybe owned by a device and should not be used. > > + */ > > +static int > > +port_is_present(uint16_t portid) > > +{ > > + uint16_t id; > > + > > + RTE_ETH_FOREACH_DEV(id) { > > + if (id == portid) > > + return 1; > > + } > > + return 0; > > +} > > + > > /** > > * The ports to be used by the application are passed in > > * the form of a bitmask. This function parses the bitmask @@ -41,31 +58,32 > > @@ usage(void) > > * array variable > > */ > > static int > > -parse_portmask(uint8_t max_ports, const char *portmask) > > +parse_portmask(const char *portmask) > > { > > char *end = NULL; > > unsigned long pm; > > - uint16_t count = 0; > > + uint16_t count; > > > > if (portmask == NULL || *portmask == '\0') > > return -1; > > > > /* convert parameter to a number and verify */ > > pm = strtoul(portmask, &end, 16); > > - if (end == NULL || *end != '\0' || pm == 0) > > + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0) > > return -1; > > > > /* loop through bits of the mask and mark ports */ > > - while (pm != 0){ > > - if (pm & 0x01){ /* bit is set in mask, use port */ > > - if (count >= max_ports) > > - printf("WARNING: requested port %u not > > present" > > - " - ignoring\n", (unsigned)count); > > - else > > - ports->id[ports->num_ports++] = count; > > + for (count = 0; pm != 0; pm >>= 1, ++count) { > > + if ((pm & 0x1) == 0) > > + continue; > > + > > + if (!port_is_present(count)) { > > + printf("WARNING: requested port %u not present - > > ignoring\n", > > + count); > > + continue; > > } > > - pm = (pm >> 1); > > - count++; > > + > > + ports->id[ports->num_ports++] = count; > > } > > Why not just to do something like: > > RTE_ETH_FOREACH_DEV(id) { > If (pm & (1 << id)) { > pm &= ~(1 << id) > put in list > } > } > If (pm) > Warning > No real reason, was just trying to keep existing logic flow