[2/2] example: ipsec: initialise packet length and tailroom correctly

Message ID 1452080019-25124-2-git-send-email-stuart.haslam@linaro.org
State New
Headers show

Commit Message

Stuart Haslam Jan. 6, 2016, 11:33 a.m.
The example scripts run_esp_out, run_ah_out and run_both_out are failing
due to test packet meta data being initialised incorrectly.

Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org>
---
This is a minimal change to fix the bug, but I'm not convinced the way
packets are created here is valid. It calls odp_packet_alloc() with a
length of 0, then starts writing directly to the odp_packet_data()
pointer. This works with linux-generic because allocating a packet of
length 0 gets a packet with the data length initialised to the length
specified in odp_pool_create(), but this behaviour isn't defined.

 example/ipsec/odp_ipsec_stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bill Fischofer Jan. 6, 2016, 10:10 p.m. | #1
On Wed, Jan 6, 2016 at 5:33 AM, Stuart Haslam <stuart.haslam@linaro.org>
wrote:

> The example scripts run_esp_out, run_ah_out and run_both_out are failing

> due to test packet meta data being initialised incorrectly.

>

> Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org>

> ---

> This is a minimal change to fix the bug, but I'm not convinced the way

> packets are created here is valid. It calls odp_packet_alloc() with a

> length of 0, then starts writing directly to the odp_packet_data()

> pointer. This works with linux-generic because allocating a packet of

> length 0 gets a packet with the data length initialised to the length

> specified in odp_pool_create(), but this behaviour isn't defined.

>


That is erroneous behavior.  If you allocate a packet of length 0 then
odp_packet_len() will report 0 so the code would be writing beyond the end
of the packet.  Such behavior is, by definition, undefined.  If you alloc a
packet with length 0 then you need to first call odp_packet_push_tail() to
sweep out the number of bytes you want to be able to write.  Of course, the
RC for this must be checked since the push may or may not be successful.


>

>  example/ipsec/odp_ipsec_stream.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/example/ipsec/odp_ipsec_stream.c

> b/example/ipsec/odp_ipsec_stream.c

> index f750e18..4d908e4 100644

> --- a/example/ipsec/odp_ipsec_stream.c

> +++ b/example/ipsec/odp_ipsec_stream.c

> @@ -356,7 +356,7 @@ odp_packet_t create_ipv4_packet(stream_db_entry_t

> *stream,

>         }

>

>         /* Correct set packet length offsets */

> -       odp_packet_push_tail(pkt, data - base);

> +       odp_packet_pull_tail(pkt, odp_packet_len(pkt) - (data - base));

>         odp_packet_l2_offset_set(pkt, (uint8_t *)eth - base);

>         odp_packet_l3_offset_set(pkt, (uint8_t *)ip - base);

>         odp_packet_l4_offset_set(pkt, ((uint8_t *)ip - base) +

> sizeof(*ip));

> --

> 2.1.1

>

> _______________________________________________

> lng-odp mailing list

> lng-odp@lists.linaro.org

> https://lists.linaro.org/mailman/listinfo/lng-odp

>
Stuart Haslam Jan. 7, 2016, 2:01 p.m. | #2
On Wed, Jan 06, 2016 at 04:10:01PM -0600, Bill Fischofer wrote:
> On Wed, Jan 6, 2016 at 5:33 AM, Stuart Haslam <stuart.haslam@linaro.org>
> wrote:
> 
> > The example scripts run_esp_out, run_ah_out and run_both_out are failing
> > due to test packet meta data being initialised incorrectly.
> >
> > Signed-off-by: Stuart Haslam <stuart.haslam@linaro.org>
> > ---
> > This is a minimal change to fix the bug, but I'm not convinced the way
> > packets are created here is valid. It calls odp_packet_alloc() with a
> > length of 0, then starts writing directly to the odp_packet_data()
> > pointer. This works with linux-generic because allocating a packet of
> > length 0 gets a packet with the data length initialised to the length
> > specified in odp_pool_create(), but this behaviour isn't defined.
> >
> 
> That is erroneous behavior.  If you allocate a packet of length 0 then
> odp_packet_len() will report 0 so the code would be writing beyond the end
> of the packet.  Such behavior is, by definition, undefined.  If you alloc a
> packet with length 0 then you need to first call odp_packet_push_tail() to
> sweep out the number of bytes you want to be able to write.  Of course, the
> RC for this must be checked since the push may or may not be successful.

That certainly sounds more sensible than what currently happens.

I'll resend with an additional patch fixing odp_packet_alloc(), then
modify create_ipv4_packet() to incrementally push out the tail before
writing to the packet.

Patch

diff --git a/example/ipsec/odp_ipsec_stream.c b/example/ipsec/odp_ipsec_stream.c
index f750e18..4d908e4 100644
--- a/example/ipsec/odp_ipsec_stream.c
+++ b/example/ipsec/odp_ipsec_stream.c
@@ -356,7 +356,7 @@  odp_packet_t create_ipv4_packet(stream_db_entry_t *stream,
 	}
 
 	/* Correct set packet length offsets */
-	odp_packet_push_tail(pkt, data - base);
+	odp_packet_pull_tail(pkt, odp_packet_len(pkt) - (data - base));
 	odp_packet_l2_offset_set(pkt, (uint8_t *)eth - base);
 	odp_packet_l3_offset_set(pkt, (uint8_t *)ip - base);
 	odp_packet_l4_offset_set(pkt, ((uint8_t *)ip - base) + sizeof(*ip));