example/ipv4_multicast: fix app hanging when using clone

  This example was dropping packets when using clone (ip 224.0.0.103).
The problem was that mbufs were not freed. This was caused by coping
ol_flags from cloned mbuf to header mbufs.

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 examples/ipv4_multicast/main.c | 2 --
 1 file changed, 2 deletions(-)
  

> -----Original Message-----
> From: Lipiec, Herakliusz
> Sent: Monday, November 12, 2018 8:47 PM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>
> Subject: [PATCH] example/ipv4_multicast: fix app hanging when using clone
> 
> This example was dropping packets when using clone (ip 224.0.0.103).
> The problem was that mbufs were not freed. This was caused by coping
> ol_flags from cloned mbuf to header mbufs.
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
>  examples/ipv4_multicast/main.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/examples/ipv4_multicast/main.c b/examples/ipv4_multicast/main.c
> index 4073a4907..428ca4694 100644
> --- a/examples/ipv4_multicast/main.c
> +++ b/examples/ipv4_multicast/main.c
> @@ -266,8 +266,6 @@ mcast_out_pkt(struct rte_mbuf *pkt, int use_clone)
>  	hdr->tx_offload = pkt->tx_offload;
>  	hdr->hash = pkt->hash;
> 
> -	hdr->ol_flags = pkt->ol_flags;
> -
>  	__rte_mbuf_sanity_check(hdr, 1);
>  	return hdr;
>  }
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1
  

Hi,

12/11/2018 21:46, Herakliusz Lipiec:
> This example was dropping packets when using clone (ip 224.0.0.103).

What is this IP?
What is clone?

> The problem was that mbufs were not freed. This was caused by coping
> ol_flags from cloned mbuf to header mbufs.

Mbuf is not freed because of ol_flags?
I feel this description should be improved.

> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
[...]
> --- a/examples/ipv4_multicast/main.c
> +++ b/examples/ipv4_multicast/main.c
> @@ -266,8 +266,6 @@ mcast_out_pkt(struct rte_mbuf *pkt, int use_clone)
>  	hdr->tx_offload = pkt->tx_offload;
>  	hdr->hash = pkt->hash;
>  
> -	hdr->ol_flags = pkt->ol_flags;
> -
>  	__rte_mbuf_sanity_check(hdr, 1);
>  	return hdr;
>  }
  

> 
> Hi,
> 
> 12/11/2018 21:46, Herakliusz Lipiec:
> > This example was dropping packets when using clone (ip 224.0.0.103).

The problem is that ipv4_multicast app:
1. invokes rte_pktmbuf_clone() for the packet 
(that creates a new mbuf with  IND_ATTACHED_MBUF set in ol_flags).
2. creates new mbuf containing  L2 header and chains it with cloned at step 1 mbuf.
3. copy ol_flags from cloned mbuf to new header mbuf.

So after step 3 L2 header mbuf also has IND_ATTACHED_MBUF set in ol_flags.
That makes pktmbuf_free() wrongly assume that this is an indirect mbuf,
which causes   all sorts of problems: incorrect behavior, silent memory corruption, etc.
The easiest way to reproduce the problem:
- run ipv4_multicast, using two ports:
   ipv4_multicast -l 0,1 - -p 0x3
send  8K+ packets to one of the ports with dest ip address: 224.0.0.103
ipv4_multicast will stop forward any packets.

In fact, there is no reason to copy ol_flags from the cloned packet.
So the fix is just to remove that code.
Konstantin

> 
> What is this IP?
> What is clone?
> 
> > The problem was that mbufs were not freed. This was caused by coping
> > ol_flags from cloned mbuf to header mbufs.
> 
> Mbuf is not freed because of ol_flags?
> I feel this description should be improved.
> 
> > Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> [...]
> > --- a/examples/ipv4_multicast/main.c
> > +++ b/examples/ipv4_multicast/main.c
> > @@ -266,8 +266,6 @@ mcast_out_pkt(struct rte_mbuf *pkt, int use_clone)
> >  	hdr->tx_offload = pkt->tx_offload;
> >  	hdr->hash = pkt->hash;
> >
> > -	hdr->ol_flags = pkt->ol_flags;
> > -
> >  	__rte_mbuf_sanity_check(hdr, 1);
> >  	return hdr;
> >  }
> 
>
  

On 12-Nov-18 8:46 PM, Herakliusz Lipiec wrote:
> This example was dropping packets when using clone (ip 224.0.0.103).
> The problem was that mbufs were not freed. This was caused by coping
> ol_flags from cloned mbuf to header mbufs.
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

I agree with Thomas, needs clearer description. Suggest using some of 
the wording provided by Konstantin in later replies. Also, needs a 
Fixes: tag and a CC: stable, because it appears that this bug has been 
around for a few releases.
  

> > ---
> 
> I agree with Thomas, needs clearer description. Suggest using some of
> the wording provided by Konstantin in later replies. Also, needs a
> Fixes: tag and a CC: stable, because it appears that this bug has been
> around for a few releases.

Good point, I forgot about 'fixes' and 'stable'.
Thanks
Konstantin