Re: [dpdk-dev] [PATCH v5 1/4] examples/multi_process/client_server_mp: check port validity

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 2 Aug 2019 05:33:20 +0000
Matan Azrad <matan@mellanox.com> wrote:

> Hi Stephen
> 
> One more small  comment inline
> 
> From:  Stephen Hemminger
> > Sent: Friday, August 2, 2019 5:58 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > Subject: [dpdk-dev] [PATCH v5 1/4]
> > examples/multi_process/client_server_mp: check port validity
> > 
> > From: Stephen Hemminger <sthemmin@microsoft.com>
> > 
> > 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 <sthemmin@microsoft.com>
> > ---
> >  .../client_server_mp/mp_server/args.c         | 35 ++++++++++---------
> >  .../client_server_mp/mp_server/args.h         |  2 +-
> >  .../client_server_mp/mp_server/init.c         |  7 ++--
> >  3 files changed, 22 insertions(+), 22 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..fdc008b3d677 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 <errno.h>
> > 
> >  #include <rte_memory.h>
> > +#include <rte_ethdev.h>
> >  #include <rte_string_fns.h>
> > 
> >  #include "common.h"
> > @@ -41,31 +42,33 @@ 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 id;
> > 
> >  	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)  
> 
> Why pm > UINT16_MAX ? should be something like > (1 << RTE_MAX_ETHPORTS) - 1.
> And need to be sure pm type can hold RTE_MAX_ETHPORTS bits, otherwise port 0 may unlikely be all the time visible in the loop below.
>

The DPDK assumes a lot of places that unsigned long will hold a port mask.
If some extra bits are set, the error is visible later when the bits are leftover
after finding ports.

The original code had worse problems, it would not catch invalid pm values at all
and truncate silently.