diff mbox series

[ovs-dev,v8,02/13] dp-packet: Init specific mbuf fields.

Message ID 1528734090-220990-3-git-send-email-tiago.lam@intel.com
State Superseded
Headers show
Series Support multi-segment mbufs | expand

Commit Message

Lam, Tiago June 11, 2018, 4:21 p.m. UTC 
From: Mark Kavanagh <mark.b.kavanagh@intel.com>

dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's
possible the the resultant mbuf portion of the dp_packet contains
random data. For some mbuf fields, specifically those related to
multi-segment mbufs and/or offload features, random values may cause
unexpected behaviour, should the dp_packet's contents be later copied
to a DPDK mbuf. It is critical therefore, that these fields should be
initialized to 0.

This patch ensures that the following mbuf fields are initialized to
appropriate values on creation of a new dp_packet:
   - ol_flags=0
   - nb_segs=1
   - tx_offload=0
   - packet_type=0
   - next=NULL

Adapted from an idea by Michael Qiu <qiudayu@chinac.com>:
https://patchwork.ozlabs.org/patch/777570/

Co-authored-by: Tiago Lam <tiago.lam@intel.com>

Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
 lib/dp-packet.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Eelco Chaudron June 18, 2018, 11:23 a.m. UTC | #1 
On 11 Jun 2018, at 18:21, Tiago Lam wrote:

> From: Mark Kavanagh <mark.b.kavanagh@intel.com>
>
> dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's
> possible the the resultant mbuf portion of the dp_packet contains
> random data. For some mbuf fields, specifically those related to
> multi-segment mbufs and/or offload features, random values may cause
> unexpected behaviour, should the dp_packet's contents be later copied
> to a DPDK mbuf. It is critical therefore, that these fields should be
> initialized to 0.
>
> This patch ensures that the following mbuf fields are initialized to
> appropriate values on creation of a new dp_packet:
>    - ol_flags=0
>    - nb_segs=1
>    - tx_offload=0
>    - packet_type=0
>    - next=NULL
>
> Adapted from an idea by Michael Qiu <qiudayu@chinac.com>:
> https://patchwork.ozlabs.org/patch/777570/
>
> Co-authored-by: Tiago Lam <tiago.lam@intel.com>
>
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> ---
>  lib/dp-packet.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 596cfe6..82e45ad 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -626,13 +626,15 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet 
> *p OVS_UNUSED)
>
>  /* This initialization is needed for packets that do not come
>   * from DPDK interfaces, when vswitchd is built with --with-dpdk.
> - * The DPDK rte library will still otherwise manage the mbuf.
> - * We only need to initialize the mbuf ol_flags. */
> + * The DPDK rte library will still otherwise manage the mbuf. */
>  static inline void
>  dp_packet_mbuf_init(struct dp_packet *p OVS_UNUSED)
>  {
>  #ifdef DPDK_NETDEV
> -    p->mbuf.ol_flags = 0;
> +    struct rte_mbuf *mbuf = &(p->mbuf);
> +    mbuf->ol_flags = mbuf->tx_offload = mbuf->packet_type = 0;
> +    mbuf->nb_segs = 1;
> +    mbuf->next = NULL;

Are we sure this is all we need? Asking as rte_pktmbuf_reset() cleans of 
some more field, so want to make sure we hit all the corner cases. 
Unfortunately, we can not use it here due to the sanity check.

>  #endif
>  }
>
> -- 
> 2.7.4
Lam, Tiago June 22, 2018, 7:03 p.m. UTC | #2 
On 18/06/2018 12:23, Eelco Chaudron wrote:
> 
> 
> On 11 Jun 2018, at 18:21, Tiago Lam wrote:
> 
>> From: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>
>> dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's
>> possible the the resultant mbuf portion of the dp_packet contains
>> random data. For some mbuf fields, specifically those related to
>> multi-segment mbufs and/or offload features, random values may cause
>> unexpected behaviour, should the dp_packet's contents be later copied
>> to a DPDK mbuf. It is critical therefore, that these fields should be
>> initialized to 0.
>>
>> This patch ensures that the following mbuf fields are initialized to
>> appropriate values on creation of a new dp_packet:
>>    - ol_flags=0
>>    - nb_segs=1
>>    - tx_offload=0
>>    - packet_type=0
>>    - next=NULL
>>
>> Adapted from an idea by Michael Qiu <qiudayu@chinac.com>:
>> https://patchwork.ozlabs.org/patch/777570/
>>
>> Co-authored-by: Tiago Lam <tiago.lam@intel.com>
>>
>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
>> ---
>>  lib/dp-packet.h | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index 596cfe6..82e45ad 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -626,13 +626,15 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet 
>> *p OVS_UNUSED)
>>
>>  /* This initialization is needed for packets that do not come
>>   * from DPDK interfaces, when vswitchd is built with --with-dpdk.
>> - * The DPDK rte library will still otherwise manage the mbuf.
>> - * We only need to initialize the mbuf ol_flags. */
>> + * The DPDK rte library will still otherwise manage the mbuf. */
>>  static inline void
>>  dp_packet_mbuf_init(struct dp_packet *p OVS_UNUSED)
>>  {
>>  #ifdef DPDK_NETDEV
>> -    p->mbuf.ol_flags = 0;
>> +    struct rte_mbuf *mbuf = &(p->mbuf);
>> +    mbuf->ol_flags = mbuf->tx_offload = mbuf->packet_type = 0;
>> +    mbuf->nb_segs = 1;
>> +    mbuf->next = NULL;
> 
> Are we sure this is all we need? Asking as rte_pktmbuf_reset() cleans of 
> some more field, so want to make sure we hit all the corner cases. 
> Unfortunately, we can not use it here due to the sanity check.
> 

In this case it doesn't seem to me that we need to replicate what DPDK
would do. Eventually, before sending this packet out (because this is a
DPBUF_MALLOC dp_packet), the values will be copied to an mbuf properly
initialised by DPDK, and we just need to make sure we are not copying
random values as it was happening before with `ol_falgs` and
`tx_offload`. For the use of DPDK's mbuf manipulation functions, such as
rte_pktmbuf_tailroom(), these flags are enough.

On a similar note, I don't think the comment above, "The DPDK rte
library will still otherwise manage the mbuf.", is relevant anymore
since DPDK won't manage this packet at all. I'll remove it for next
iteration.

Tiago.
Eelco Chaudron June 26, 2018, 8:51 a.m. UTC | #3 
On 22 Jun 2018, at 21:03, Lam, Tiago wrote:

> On 18/06/2018 12:23, Eelco Chaudron wrote:
>>
>>
>> On 11 Jun 2018, at 18:21, Tiago Lam wrote:
>>
>>> From: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>>
>>> dp_packets are created using xmalloc(); in the case of OvS-DPDK, 
>>> it's
>>> possible the the resultant mbuf portion of the dp_packet contains
>>> random data. For some mbuf fields, specifically those related to
>>> multi-segment mbufs and/or offload features, random values may cause
>>> unexpected behaviour, should the dp_packet's contents be later 
>>> copied
>>> to a DPDK mbuf. It is critical therefore, that these fields should 
>>> be
>>> initialized to 0.
>>>
>>> This patch ensures that the following mbuf fields are initialized to
>>> appropriate values on creation of a new dp_packet:
>>>    - ol_flags=0
>>>    - nb_segs=1
>>>    - tx_offload=0
>>>    - packet_type=0
>>>    - next=NULL
>>>
>>> Adapted from an idea by Michael Qiu <qiudayu@chinac.com>:
>>> https://patchwork.ozlabs.org/patch/777570/
>>>
>>> Co-authored-by: Tiago Lam <tiago.lam@intel.com>
>>>
>>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
>>> ---
>>>  lib/dp-packet.h | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>>> index 596cfe6..82e45ad 100644
>>> --- a/lib/dp-packet.h
>>> +++ b/lib/dp-packet.h
>>> @@ -626,13 +626,15 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet
>>> *p OVS_UNUSED)
>>>
>>>  /* This initialization is needed for packets that do not come
>>>   * from DPDK interfaces, when vswitchd is built with --with-dpdk.
>>> - * The DPDK rte library will still otherwise manage the mbuf.
>>> - * We only need to initialize the mbuf ol_flags. */
>>> + * The DPDK rte library will still otherwise manage the mbuf. */
>>>  static inline void
>>>  dp_packet_mbuf_init(struct dp_packet *p OVS_UNUSED)
>>>  {
>>>  #ifdef DPDK_NETDEV
>>> -    p->mbuf.ol_flags = 0;
>>> +    struct rte_mbuf *mbuf = &(p->mbuf);
>>> +    mbuf->ol_flags = mbuf->tx_offload = mbuf->packet_type = 0;
>>> +    mbuf->nb_segs = 1;
>>> +    mbuf->next = NULL;
>>
>> Are we sure this is all we need? Asking as rte_pktmbuf_reset() cleans 
>> of
>> some more field, so want to make sure we hit all the corner cases.
>> Unfortunately, we can not use it here due to the sanity check.
>>
>
> In this case it doesn't seem to me that we need to replicate what DPDK
> would do. Eventually, before sending this packet out (because this is 
> a
> DPBUF_MALLOC dp_packet), the values will be copied to an mbuf properly
> initialised by DPDK, and we just need to make sure we are not copying
> random values as it was happening before with `ol_falgs` and
> `tx_offload`. For the use of DPDK's mbuf manipulation functions, such 
> as
> rte_pktmbuf_tailroom(), these flags are enough.

Thanks for the clarification. Guess it would also mess up the fields 
just setup by dp_packet_use__() which I missed during review.

> On a similar note, I don't think the comment above, "The DPDK rte
> library will still otherwise manage the mbuf.", is relevant anymore
> since DPDK won't manage this packet at all. I'll remove it for next
> iteration.
>
> Tiago.
diff mbox series

Patch

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 596cfe6..82e45ad 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -626,13 +626,15 @@  dp_packet_mbuf_rss_flag_reset(struct dp_packet *p OVS_UNUSED)
 
 /* This initialization is needed for packets that do not come
  * from DPDK interfaces, when vswitchd is built with --with-dpdk.
- * The DPDK rte library will still otherwise manage the mbuf.
- * We only need to initialize the mbuf ol_flags. */
+ * The DPDK rte library will still otherwise manage the mbuf. */
 static inline void
 dp_packet_mbuf_init(struct dp_packet *p OVS_UNUSED)
 {
 #ifdef DPDK_NETDEV
-    p->mbuf.ol_flags = 0;
+    struct rte_mbuf *mbuf = &(p->mbuf);
+    mbuf->ol_flags = mbuf->tx_offload = mbuf->packet_type = 0;
+    mbuf->nb_segs = 1;
+    mbuf->next = NULL;
 #endif
 }