Re: [ovs-dev] [RFC PATCH v2 1/2] miniflow_extract: Properly handle small IP packets.

2014-11-10 Thread Jarno Rajahalme

On Nov 6, 2014, at 3:58 PM, Ben Pfaff  wrote:

> On Wed, Nov 05, 2014 at 10:37:18AM -0800, Jarno Rajahalme wrote:
>> Ethernet frames may contain padding after the IP payload.  When
>> parsing IP packets, check the IP total size (IPv4) or IP payload size
>> (IPv6) to detect the size of l2 padding.  The l2 padding size is
>> stored in the ofpbuf to prevent ofpbuf_pull from entering the padding,
>> as well as to allow ofpbuf_l4_size() to return the size of the IP
>> payload without the l2 padding.
>> 
>> This helps avoiding parsing truncated transport headers, for example.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Ben Pfaff 

Thanks for the review, Ben!

Pushed to master,

  Jarno

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [RFC PATCH v2 1/2] miniflow_extract: Properly handle small IP packets.

2014-11-06 Thread Ben Pfaff
On Wed, Nov 05, 2014 at 10:37:18AM -0800, Jarno Rajahalme wrote:
> Ethernet frames may contain padding after the IP payload.  When
> parsing IP packets, check the IP total size (IPv4) or IP payload size
> (IPv6) to detect the size of l2 padding.  The l2 padding size is
> stored in the ofpbuf to prevent ofpbuf_pull from entering the padding,
> as well as to allow ofpbuf_l4_size() to return the size of the IP
> payload without the l2 padding.
> 
> This helps avoiding parsing truncated transport headers, for example.
> 
> Signed-off-by: Jarno Rajahalme 

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [RFC PATCH v2 1/2] miniflow_extract: Properly handle small IP packets.

2014-11-06 Thread Ben Pfaff
On Thu, Nov 06, 2014 at 03:40:32PM -0800, Jarno Rajahalme wrote:
> 
> On Nov 6, 2014, at 3:05 PM, Ben Pfaff  wrote:
> 
> > On Wed, Nov 05, 2014 at 10:37:18AM -0800, Jarno Rajahalme wrote:
> >> Ethernet frames may contain padding after the IP payload.  When
> >> parsing IP packets, check the IP total size (IPv4) or IP payload size
> >> (IPv6) to detect the size of l2 padding.  The l2 padding size is
> >> stored in the ofpbuf to prevent ofpbuf_pull from entering the padding,
> >> as well as to allow ofpbuf_l4_size() to return the size of the IP
> >> payload without the l2 padding.
> >> 
> >> This helps avoiding parsing truncated transport headers, for example.
> >> 
> >> Signed-off-by: Jarno Rajahalme 
> > 
> > Do you think we could just truncate the packet?
> 
> That was indeed my first implementation. I discussed this with Andy and
> Pravin yesterday after noticing that ~100 test cases need an update due
> to how the change affects stats, as userspace stats are attributed after
> parsing.
> 
> For a layer 3 device counting the padding in does not make any sense.
> For a layer 2 device, however, we concluded think the right thing to do
> is to count all the bits that were transmitted.  OVS is a L2 device, so it
> feels logical to not truncate the packet.
> 
> Truncating would also require re-padding with DPDK, which would be
> a performance issue.
> 
> I haven’t yet looked wether the kernel datapath action processing code
> needs a corresponding change.

That's a good analysis.  Thanks, I'll review the details then.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [RFC PATCH v2 1/2] miniflow_extract: Properly handle small IP packets.

2014-11-06 Thread Jarno Rajahalme

On Nov 6, 2014, at 3:05 PM, Ben Pfaff  wrote:

> On Wed, Nov 05, 2014 at 10:37:18AM -0800, Jarno Rajahalme wrote:
>> Ethernet frames may contain padding after the IP payload.  When
>> parsing IP packets, check the IP total size (IPv4) or IP payload size
>> (IPv6) to detect the size of l2 padding.  The l2 padding size is
>> stored in the ofpbuf to prevent ofpbuf_pull from entering the padding,
>> as well as to allow ofpbuf_l4_size() to return the size of the IP
>> payload without the l2 padding.
>> 
>> This helps avoiding parsing truncated transport headers, for example.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Do you think we could just truncate the packet?

That was indeed my first implementation. I discussed this with Andy and
Pravin yesterday after noticing that ~100 test cases need an update due
to how the change affects stats, as userspace stats are attributed after
parsing.

For a layer 3 device counting the padding in does not make any sense.
For a layer 2 device, however, we concluded think the right thing to do
is to count all the bits that were transmitted.  OVS is a L2 device, so it
feels logical to not truncate the packet.

Truncating would also require re-padding with DPDK, which would be
a performance issue.

I haven’t yet looked wether the kernel datapath action processing code
needs a corresponding change.

 Jarno

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [RFC PATCH v2 1/2] miniflow_extract: Properly handle small IP packets.

2014-11-06 Thread Ben Pfaff
On Wed, Nov 05, 2014 at 10:37:18AM -0800, Jarno Rajahalme wrote:
> Ethernet frames may contain padding after the IP payload.  When
> parsing IP packets, check the IP total size (IPv4) or IP payload size
> (IPv6) to detect the size of l2 padding.  The l2 padding size is
> stored in the ofpbuf to prevent ofpbuf_pull from entering the padding,
> as well as to allow ofpbuf_l4_size() to return the size of the IP
> payload without the l2 padding.
> 
> This helps avoiding parsing truncated transport headers, for example.
> 
> Signed-off-by: Jarno Rajahalme 

Do you think we could just truncate the packet?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev