Re: [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask configurations

[Matan Azrad <matan@mellanox.com>]

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: 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)) {
> >  		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