From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <adrien.mazarguil@6wind.com>
Received: from mail-wm0-f65.google.com (mail-wm0-f65.google.com [74.125.82.65])
by dpdk.org (Postfix) with ESMTP id 34F611B536
for <stable@dpdk.org>; Fri, 3 Aug 2018 10:21:10 +0200 (CEST)
Received: by mail-wm0-f65.google.com with SMTP id c14-v6so5339780wmb.4
for <stable@dpdk.org>; Fri, 03 Aug 2018 01:21:10 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=6wind-com.20150623.gappssmtp.com; s=20150623;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:in-reply-to;
bh=O+vvyRDKkrB3p3a4GK6S6H2TNwXVLuzDFhqIMJxgjVQ=;
b=EOY+HQhpDyf/KCLaPdewdWyHCgGFeFZYbAZYjDJy0nkFguD5HP5FJEUEsD3aUGEkbM
95ZtXqaC2FfNmHHur0PFFFT6KbPvDppgXIlBn2WBmNtGD2A8pKKo113Bjylb2JSlOpgI
YDTMWIn9FZM8UroSVArXLN38WV3OEHLBtifk5ak7yokSFSAgf/TUg5xmicoctqkPZ1GA
li8JIQuSd2orD3ULmWvUdNLm13RhZUrJOqheoYQBfJ5k3Qo9J6+jDJrBXuPwEQIfD/ph
XZGvGJgccH+ER49dJAg6e3y5/1rJ9oMfuBbkj5C17tusz5MhBIdKtWy2yEg5izNNcI5r
4fIg==
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:references
:mime-version:content-disposition:in-reply-to;
bh=O+vvyRDKkrB3p3a4GK6S6H2TNwXVLuzDFhqIMJxgjVQ=;
b=c87s5iaIYcypddinKkIgdGGJv9T7cTec5znzzCWEG4acvKjRYe8FXzdh3zNNC2aLoK
aBKPfZnyszbdNqEITkjKRkmPUXsGMcT6Um4f5XY1eAFulS6xuD8ERC0ipqsAKsCmajMy
/i3Xy9tsC+iX+g+b6zGF7S8mF5oeL5Hmv6TAh76HAKdEkSgliIipZyqV5IeR6L1VbuRZ
1LgDfj9mp7/NxOj9/aJFvjMYqX3wlS9uEvA9qMRyp8XaJgsx3G3kk+yg3aBvQzpy4moF
EUjy3cCjfOAeMtl+LbL9n55D1uoD0P8ckZc6JdT6v1GK9exK+EZE5bvuUlQDvTimf7aS
EWDA==
X-Gm-Message-State: AOUpUlHejqzfsKlXIMDcEGAKiPjB3l5bVURq+6N5OQlP8jHIe6cdRbUg
7yZhwSSWbkyOLimce919fE1nkQ==
X-Google-Smtp-Source: AAOMgpeKnZdbO4r/a4eogKqbOehmt2J/IPrWMHWt4ZjFHk+My7vZ7mjKz+o++6abxJaeWP0ZtBgW8w==
X-Received: by 2002:a1c:1c92:: with SMTP id
c140-v6mr3981319wmc.155.1533284469812;
Fri, 03 Aug 2018 01:21:09 -0700 (PDT)
Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78])
by smtp.gmail.com with ESMTPSA id r17-v6sm2777318wrt.44.2018.08.03.01.21.08
(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
Fri, 03 Aug 2018 01:21:08 -0700 (PDT)
Date: Fri, 3 Aug 2018 10:20:51 +0200
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: Keith Wiles <keith.wiles@intel.com>, Ophir Munk <ophirmu@mellanox.com>,
"dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Message-ID: <20180803082051.GP5211@6wind.com>
References: <1533205980-7874-1-git-send-email-matan@mellanox.com>
<20180802142737.GO5211@6wind.com>
<AM0PR0502MB4019749C9255316DA451E785D22C0@AM0PR0502MB4019.eurprd05.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <AM0PR0502MB4019749C9255316DA451E785D22C0@AM0PR0502MB4019.eurprd05.prod.outlook.com>
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask
configurations
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 03 Aug 2018 08:21:10 -0000
Hi Matan,
On Thu, Aug 02, 2018 at 05:52:18PM +0000, Matan Azrad wrote:
> Hi Adrien
>
> From: Adrien Mazarguil
> > On Thu, Aug 02, 2018 at 10:33:00AM +0000, Matan Azrad wrote:
> > > The rte_flow meaning of zero flow mask configuration is to match all
> > > the range of the item value.
> > > For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> > > should much all the ipv4 traffic from the rte_flow API perspective.
> > >
> > > From some kernel perspectives the above rule means to ignore all the
> > > ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> > >
> > > Due to the fact that the tap PMD should provide the rte_flow meaning,
> > > it is necessary to ignore the spec in case the mask is zero when it
> > > forwards such like flows to the kernel.
> > > So, the above rule should be translated to eth / ipv4 to get the
> > > correct meaning.
> > >
> > > Ignore spec configurations when the mask is zero.
> >
> > I would go further, one should be able to match IP address 0.0.0.0 for instance.
> > The PMD should only trust the mask on all fields without looking at spec.
>
> The PMD should convert the RTE flow API to the device configuration,
> So I can think on scenarios that the PMD should look on spec.
Obviously the PMD needs to take spec into account. What I meant is that for
each field, spec must be taken into account according to mask only.
For any given field, when mask is empty, don't look at spec, it's like a
wildcard. When mask is full, take spec as is, even if spec only contains
zeroed bits.
User intent in that case is to match a zero value exactly, so it must not
result in a wildcard match. If supported, when mask is partial, masked bits
are also matched exactly, even if these turn out to be a zero
value. Unmasked bits are considered wildcards.
In short, to address both the issue mentioned in the commit log and the one
I'm talking about, you only need to replace "spec" with "mask" in the
original code. More below.
> See
> > below for suggestions.
> >
> > > Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and
> > > actions")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> > > drivers/net/tap/tap_flow.c | 13 ++++++++-----
> > > 1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> > > index 6b60e6d..993e6f6 100644
> > > --- a/drivers/net/tap/tap_flow.c
> > > +++ b/drivers/net/tap/tap_flow.c
> > > @@ -537,7 +537,8 @@ tap_flow_create_eth(const struct rte_flow_item
> > *item, void *data)
> > > if (!flow)
> > > return 0;
> > > msg = &flow->msg;
> > > - if (!is_zero_ether_addr(&spec->dst)) {
> > > + if (!is_zero_ether_addr(&spec->dst) &&
> >
> > This check should be removed.
>
> I don't know why we need this check, and the below checks
> So it should be tested before the change.
> It may be a different issue.
>
> >
> > > + !is_zero_ether_addr(&mask->dst)) {
Should read:
if (!is_zero_ether_addr(&mask->dst)) {
> > > tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_ETH_DST,
> > ETHER_ADDR_LEN,
> > > &spec->dst.addr_bytes);
> > > tap_nlattr_add(&msg->nh,
> > > @@ -651,13 +652,13 @@ tap_flow_create_ipv4(const struct rte_flow_item
> > *item, void *data)
> > > info->eth_type = htons(ETH_P_IP);
> > > if (!spec)
> > > return 0;
> > > - if (spec->hdr.dst_addr) {
> > > + if (spec->hdr.dst_addr && mask->hdr.dst_addr) {
> >
> > Ditto (before &&).
Should read:
if (mask->hdr.dst_addr) {
> >
> > > tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST,
> > > spec->hdr.dst_addr);
> > > tap_nlattr_add32(&msg->nh,
> > TCA_FLOWER_KEY_IPV4_DST_MASK,
> > > mask->hdr.dst_addr);
> > > }
> > > - if (spec->hdr.src_addr) {
> > > + if (spec->hdr.src_addr && mask->hdr.src_addr) {
> >
> > Ditto.
Should read:
if (mask->hdr.dst_addr) {
> > > tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC,
> > > spec->hdr.src_addr);
> > > tap_nlattr_add32(&msg->nh,
> > TCA_FLOWER_KEY_IPV4_SRC_MASK, @@ -707,13
> > > +708,15 @@ tap_flow_create_ipv6(const struct rte_flow_item *item, void
> > *data)
> > > info->eth_type = htons(ETH_P_IPV6);
> > > if (!spec)
> > > return 0;
> > > - if (memcmp(spec->hdr.dst_addr, empty_addr, 16)) {
> > > + if (memcmp(spec->hdr.dst_addr, empty_addr, 16) &&
> >
> > Ditto.
Should read:
if (memcmp(mask->hdr.dst_addr, empty_addr, 16)) {
> >
> > > + memcmp(mask->hdr.dst_addr, empty_addr, 16)) {
> > > tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST,
> > > sizeof(spec->hdr.dst_addr), &spec->hdr.dst_addr);
> > > tap_nlattr_add(&msg->nh,
> > TCA_FLOWER_KEY_IPV6_DST_MASK,
> > > sizeof(mask->hdr.dst_addr), &mask->hdr.dst_addr);
> > > }
> > > - if (memcmp(spec->hdr.src_addr, empty_addr, 16)) {
> > > + if (memcmp(spec->hdr.src_addr, empty_addr, 16) &&
> >
> > Ditto.
Should read:
if (memcmp(mask->hdr.src_addr, empty_addr, 16)) {
> >
> > > + memcmp(mask->hdr.src_addr, empty_addr, 16)) {
> > > tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC,
> > > sizeof(spec->hdr.src_addr), &spec->hdr.src_addr);
> > > tap_nlattr_add(&msg->nh,
> > TCA_FLOWER_KEY_IPV6_SRC_MASK,
> > > --
> > > 2.7.4
> > >
The same issue exists with UDP and TCP ports by the way:
-if (spec->hdr.dst_port & mask->hdr.dst_port)
+if (mask->hdr.dst_port)
-if (spec->hdr.src_port & mask->hdr.src_port)
+if (mask->hdr.src_port)
-if (spec->hdr.dst_port & mask->hdr.dst_port)
+if (mask->hdr.dst_port)
-if (spec->hdr.src_port & mask->hdr.src_port)
+if (mask->hdr.src_port)
Otherwise one can't match traffic where source/destination ports are 0. Yes
such traffic should be invalid, however that's precisely why one would want
to match it: drop before it reaches the protocol stack.
--
Adrien Mazarguil
6WIND