example/ipv4_multicast: fix app hanging when using clone
Checks
Commit Message
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(-)
Comments
> -----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
@@ -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;
}