diff mbox series

[ovs-dev] netdev-dpdk: reset packet_type for reused dp_packets

Message ID AM2PR07MB1042D848A7965231EA9EB2878A970@AM2PR07MB1042.eurprd07.prod.outlook.com
State Superseded
Headers show
Series [ovs-dev] netdev-dpdk: reset packet_type for reused dp_packets | expand

Commit Message

Zoltan Balogh Sept. 6, 2017, 12:12 p.m. UTC 
DPDK uses dp-packet pool for storing received packets. The pool is
reused by rxq_recv funcions of the DPDK netdevs. The datapath is
capable to modify the packet_type property of packets. For instance
when encapsulated L3 packets are received on a ptap gre port.
In this case the packet_type property of struct dp_packet can be
modified and later the same dp_packet with the modified packet_type
can be reused in the rxq_rec function, so it can contain corrupted
data.

The dp_packet_batch_init_cutlen() in the rxq_recv functions iterates
over dp_packets and sets their cutlen. So I modified this function
to set packet_type to Ethernet for the dp_packets as well. I also
renamed this function because of the added functionality.

The dp_packet_batch_init_cutlen() iterates over batch->count dp_packet.
Therefore setting of batch->count = nb_rx needs to be done before the
former function is invoked. This is an additional fix.

Signed-off-by: Zoltán Balogh <zoltan.balogh@ericsson.com>
Signed-off-by: László Sürü <laszlo.suru@ericsson.com>
Co-authored-by: László Sürü <laszlo.suru@ericsson.com>
CC: Jan Scheurich <jan.scheurich@ericsson.com>
CC: Sugesh Chandran <sugesh.chandran@intel.com>
CC: Darrell Ball <dlu998@gmail.com>
---
 lib/dp-packet.h   | 4 +++-
 lib/netdev-dpdk.c | 5 +++--
 2 files changed, 6 insertions(+), 3 deletions(-)

Comments

Darrell Ball Sept. 6, 2017, 10:44 p.m. UTC | #1 
On 9/6/17, 5:12 AM, "ovs-dev-bounces@openvswitch.org on behalf of Zoltán Balogh" <ovs-dev-bounces@openvswitch.org on behalf of zoltan.balogh@ericsson.com> wrote:

    DPDK uses dp-packet pool for storing received packets. The pool is
    reused by rxq_recv funcions of the DPDK netdevs. The datapath is
    capable to modify the packet_type property of packets. For instance
    when encapsulated L3 packets are received on a ptap gre port.
    In this case the packet_type property of struct dp_packet can be
    modified and later the same dp_packet with the modified packet_type
    can be reused in the rxq_rec function, so it can contain corrupted
    data.
    
    The dp_packet_batch_init_cutlen() in the rxq_recv functions iterates
    over dp_packets and sets their cutlen. So I modified this function
    to set packet_type to Ethernet for the dp_packets as well. I also
    renamed this function because of the added functionality.
    
    The dp_packet_batch_init_cutlen() iterates over batch->count dp_packet.
    Therefore setting of batch->count = nb_rx needs to be done before the
    former function is invoked. This is an additional fix.
    
    Signed-off-by: Zoltán Balogh <zoltan.balogh@ericsson.com>

    Signed-off-by: László Sürü <laszlo.suru@ericsson.com>

    Co-authored-by: László Sürü <laszlo.suru@ericsson.com>
    CC: Jan Scheurich <jan.scheurich@ericsson.com>
    CC: Sugesh Chandran <sugesh.chandran@intel.com>
    CC: Darrell Ball <dlu998@gmail.com>
    ---
     lib/dp-packet.h   | 4 +++-
     lib/netdev-dpdk.c | 5 +++--
     2 files changed, 6 insertions(+), 3 deletions(-)
    
    diff --git a/lib/dp-packet.h b/lib/dp-packet.h
    index 046f3ab..0046d0a 100644
    --- a/lib/dp-packet.h
    +++ b/lib/dp-packet.h
    @@ -805,12 +805,14 @@ dp_packet_delete_batch(struct dp_packet_batch *batch, bool may_steal)
     }
     
     static inline void
    -dp_packet_batch_init_cutlen(struct dp_packet_batch *batch)
    +dp_packet_batch_init_transient_fields(struct dp_packet_batch *batch)

[Darrell]
Can we just call this function dp_packet_batch_init_packet_fields() now that it has wider scope than just cutlen,
as the name makes it clear that we are initing the individual packet fields of a batch, rather than the batch context fields ?
‘transient’ is maybe implied ?


     {
         struct dp_packet *packet;
     
         DP_PACKET_BATCH_FOR_EACH (packet, batch) {
             dp_packet_reset_cutlen(packet);
    +        /* Set packet_type to Ethernet. */
    +        packet->packet_type = PACKET_TYPE_BE(OFPHTN_ONF, 0x0000);


[Darrell] The original dp_packet init code for PTAP has
 b->packet_type = htonl(PT_ETH);
This form seems preferred and intuitive (comment would even be unnecessary); can we use this form ?

Also, the original dp_packet init code for PTAP was in dp_packet_init__ and then moved to dp_packet_init_dpdk
I think we can just remove it from there, as it is redundant with the new code.


         }
     }
     
    diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
    index f58e9be..2422741 100644
    --- a/lib/netdev-dpdk.c
    +++ b/lib/netdev-dpdk.c
    @@ -1644,8 +1644,9 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
                                              nb_rx, dropped);
         rte_spinlock_unlock(&dev->stats_lock);
     
    -    dp_packet_batch_init_cutlen(batch);
         batch->count = (int) nb_rx;

[Darrell] Although nothing to do with this patch, can we remove the cast ‘(int)’ above ?


    +    dp_packet_batch_init_transient_fields(batch);

[Darrell]
Good catch; this should work better than initializing 0 of the entries (

    +
         return 0;
     }
     
    @@ -1684,8 +1685,8 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)
             rte_spinlock_unlock(&dev->stats_lock);
         }
     
    -    dp_packet_batch_init_cutlen(batch);
         batch->count = nb_rx;
    +    dp_packet_batch_init_transient_fields(batch);

[Darrell]
Same comment as above.
     
         return 0;
     }
    -- 
    1.9.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIFAw&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=zcjvUT1lz85oUJWStkTlRzIdXuQnJ4HL-n6oiK99iuM&s=KmEO2izD43oNVigXXu6TlPCLNruTmVtKMGdxVb6ANYs&e=
Zoltan Balogh Sept. 8, 2017, 8:45 a.m. UTC | #2 
Hi Darrell,

Thank you for your comments! I created a second version and sent it to the dev list:
https://patchwork.ozlabs.org/patch/811387/

Best regards,
Zoltan

> -----Original Message-----

> From: Darrell Ball [mailto:dball@vmware.com]

> Sent: Thursday, September 07, 2017 12:45 AM

> To: Zoltán Balogh <zoltan.balogh@ericsson.com>; 'dev@openvswitch.org' <dev@openvswitch.org>

> Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: reset packet_type for reused dp_packets

> 

> 

> 

> On 9/6/17, 5:12 AM, "ovs-dev-bounces@openvswitch.org on behalf of Zoltán Balogh" <ovs-dev-bounces@openvswitch.org on

> behalf of zoltan.balogh@ericsson.com> wrote:

> 

>     DPDK uses dp-packet pool for storing received packets. The pool is

>     reused by rxq_recv funcions of the DPDK netdevs. The datapath is

>     capable to modify the packet_type property of packets. For instance

>     when encapsulated L3 packets are received on a ptap gre port.

>     In this case the packet_type property of struct dp_packet can be

>     modified and later the same dp_packet with the modified packet_type

>     can be reused in the rxq_rec function, so it can contain corrupted

>     data.

> 

>     The dp_packet_batch_init_cutlen() in the rxq_recv functions iterates

>     over dp_packets and sets their cutlen. So I modified this function

>     to set packet_type to Ethernet for the dp_packets as well. I also

>     renamed this function because of the added functionality.

> 

>     The dp_packet_batch_init_cutlen() iterates over batch->count dp_packet.

>     Therefore setting of batch->count = nb_rx needs to be done before the

>     former function is invoked. This is an additional fix.

> 

>     Signed-off-by: Zoltán Balogh <zoltan.balogh@ericsson.com>

>     Signed-off-by: László Sürü <laszlo.suru@ericsson.com>

>     Co-authored-by: László Sürü <laszlo.suru@ericsson.com>

>     CC: Jan Scheurich <jan.scheurich@ericsson.com>

>     CC: Sugesh Chandran <sugesh.chandran@intel.com>

>     CC: Darrell Ball <dlu998@gmail.com>

>     ---

>      lib/dp-packet.h   | 4 +++-

>      lib/netdev-dpdk.c | 5 +++--

>      2 files changed, 6 insertions(+), 3 deletions(-)

> 

>     diff --git a/lib/dp-packet.h b/lib/dp-packet.h

>     index 046f3ab..0046d0a 100644

>     --- a/lib/dp-packet.h

>     +++ b/lib/dp-packet.h

>     @@ -805,12 +805,14 @@ dp_packet_delete_batch(struct dp_packet_batch *batch, bool may_steal)

>      }

> 

>      static inline void

>     -dp_packet_batch_init_cutlen(struct dp_packet_batch *batch)

>     +dp_packet_batch_init_transient_fields(struct dp_packet_batch *batch)

> 

> [Darrell]

> Can we just call this function dp_packet_batch_init_packet_fields() now that it has wider scope than just cutlen,

> as the name makes it clear that we are initing the individual packet fields of a batch, rather than the batch

> context fields ?

> ‘transient’ is maybe implied ?

> 

> 

>      {

>          struct dp_packet *packet;

> 

>          DP_PACKET_BATCH_FOR_EACH (packet, batch) {

>              dp_packet_reset_cutlen(packet);

>     +        /* Set packet_type to Ethernet. */

>     +        packet->packet_type = PACKET_TYPE_BE(OFPHTN_ONF, 0x0000);

> 

> 

> [Darrell] The original dp_packet init code for PTAP has

>  b->packet_type = htonl(PT_ETH);

> This form seems preferred and intuitive (comment would even be unnecessary); can we use this form ?

> 

> Also, the original dp_packet init code for PTAP was in dp_packet_init__ and then moved to dp_packet_init_dpdk

> I think we can just remove it from there, as it is redundant with the new code.

> 

> 

>          }

>      }

> 

>     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c

>     index f58e9be..2422741 100644

>     --- a/lib/netdev-dpdk.c

>     +++ b/lib/netdev-dpdk.c

>     @@ -1644,8 +1644,9 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,

>                                               nb_rx, dropped);

>          rte_spinlock_unlock(&dev->stats_lock);

> 

>     -    dp_packet_batch_init_cutlen(batch);

>          batch->count = (int) nb_rx;

> 

> [Darrell] Although nothing to do with this patch, can we remove the cast ‘(int)’ above ?

> 

> 

>     +    dp_packet_batch_init_transient_fields(batch);

> 

> [Darrell]

> Good catch; this should work better than initializing 0 of the entries (

> 

>     +

>          return 0;

>      }

> 

>     @@ -1684,8 +1685,8 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)

>              rte_spinlock_unlock(&dev->stats_lock);

>          }

> 

>     -    dp_packet_batch_init_cutlen(batch);

>          batch->count = nb_rx;

>     +    dp_packet_batch_init_transient_fields(batch);

> 

> [Darrell]

> Same comment as above.

> 

>          return 0;

>      }

>     --

>     1.9.1

> 

>     _______________________________________________

>     dev mailing list

>     dev@openvswitch.org

>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

> 2Ddev&d=DwIFAw&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=zcjvUT1lz85oUJWStkTlRzIdXuQnJ4HL-

> n6oiK99iuM&s=KmEO2izD43oNVigXXu6TlPCLNruTmVtKMGdxVb6ANYs&e=

>
diff mbox series

Patch

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 046f3ab..0046d0a 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -805,12 +805,14 @@  dp_packet_delete_batch(struct dp_packet_batch *batch, bool may_steal)
 }
 
 static inline void
-dp_packet_batch_init_cutlen(struct dp_packet_batch *batch)
+dp_packet_batch_init_transient_fields(struct dp_packet_batch *batch)
 {
     struct dp_packet *packet;
 
     DP_PACKET_BATCH_FOR_EACH (packet, batch) {
         dp_packet_reset_cutlen(packet);
+        /* Set packet_type to Ethernet. */
+        packet->packet_type = PACKET_TYPE_BE(OFPHTN_ONF, 0x0000);
     }
 }
 
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index f58e9be..2422741 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1644,8 +1644,9 @@  netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
                                          nb_rx, dropped);
     rte_spinlock_unlock(&dev->stats_lock);
 
-    dp_packet_batch_init_cutlen(batch);
     batch->count = (int) nb_rx;
+    dp_packet_batch_init_transient_fields(batch);
+
     return 0;
 }
 
@@ -1684,8 +1685,8 @@  netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)
         rte_spinlock_unlock(&dev->stats_lock);
     }
 
-    dp_packet_batch_init_cutlen(batch);
     batch->count = nb_rx;
+    dp_packet_batch_init_transient_fields(batch);
 
     return 0;
 }