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.
See
> below for suggestions.
>
> > Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and
> > actions")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Matan Azrad <ma...@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)) {
> > 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 &&).
>
> > 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.
>
> > 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.
>
> > + 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.
>
> > + 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
> >
>
> --
> Adrien Mazarguil
> 6WIND
