Message ID | 1503679232-11135-5-git-send-email-ian.stokes@intel.com |
---|---|
State | Changes Requested |
Headers | show |
On Fri, Aug 25, 2017 at 05:40:26PM +0100, Ian Stokes wrote: > This patch adds a field to the flow struct to represent the ESP security > parameter index of a packet. > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > --- > include/openvswitch/flow.h | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h > index a658a58..d986929 100644 > --- a/include/openvswitch/flow.h > +++ b/include/openvswitch/flow.h > @@ -156,6 +156,11 @@ struct flow { > ovs_be32 igmp_group_ip4; /* IGMP group IPv4 address. > * Keep last for BUILD_ASSERT_DECL below. */ > ovs_be32 pad3; /* Pad to 64 bits. */ > + > + /* SPI for ESP (64-bit aligned) */ > + /* XXX TO DO: move this to the l3 layer */ > + ovs_be32 spi; > + ovs_be32 pad4; > }; I'd like to see this moved to the L3 layer (as your comment says).
On Tue, Oct 31, 2017 at 02:46:15PM -0700, Ben Pfaff wrote: > On Fri, Aug 25, 2017 at 05:40:26PM +0100, Ian Stokes wrote: > > This patch adds a field to the flow struct to represent the ESP security > > parameter index of a packet. > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > > --- > > include/openvswitch/flow.h | 5 +++++ > > 1 files changed, 5 insertions(+), 0 deletions(-) > > > > diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h > > index a658a58..d986929 100644 > > --- a/include/openvswitch/flow.h > > +++ b/include/openvswitch/flow.h > > @@ -156,6 +156,11 @@ struct flow { > > ovs_be32 igmp_group_ip4; /* IGMP group IPv4 address. > > * Keep last for BUILD_ASSERT_DECL below. */ > > ovs_be32 pad3; /* Pad to 64 bits. */ > > + > > + /* SPI for ESP (64-bit aligned) */ > > + /* XXX TO DO: move this to the l3 layer */ > > + ovs_be32 spi; > > + ovs_be32 pad4; > > }; > > I'd like to see this moved to the L3 layer (as your comment says). Actually, I think I take that back. Isn't this logically part of L4? (If so, then 'spi' can replace 'pad3' instead of being a doubleword of its own.)
> On Fri, Aug 25, 2017 at 05:40:26PM +0100, Ian Stokes wrote: > > This patch adds a field to the flow struct to represent the ESP > > security parameter index of a packet. > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > > --- > > include/openvswitch/flow.h | 5 +++++ > > 1 files changed, 5 insertions(+), 0 deletions(-) > > > > diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h > > index a658a58..d986929 100644 > > --- a/include/openvswitch/flow.h > > +++ b/include/openvswitch/flow.h > > @@ -156,6 +156,11 @@ struct flow { > > ovs_be32 igmp_group_ip4; /* IGMP group IPv4 address. > > * Keep last for BUILD_ASSERT_DECL > below. */ > > ovs_be32 pad3; /* Pad to 64 bits. */ > > + > > + /* SPI for ESP (64-bit aligned) */ > > + /* XXX TO DO: move this to the l3 layer */ > > + ovs_be32 spi; > > + ovs_be32 pad4; > > }; > > I'd like to see this moved to the L3 layer (as your comment says). Sure will do for the v3 RFC. This was laziness on my part :) (Want to float the idea before moving it to the correct area and re-tesing to ensure padding wasn't disturbed for unit tests etc).
> On Tue, Oct 31, 2017 at 02:46:15PM -0700, Ben Pfaff wrote: > > On Fri, Aug 25, 2017 at 05:40:26PM +0100, Ian Stokes wrote: > > > This patch adds a field to the flow struct to represent the ESP > > > security parameter index of a packet. > > > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > > > --- > > > include/openvswitch/flow.h | 5 +++++ > > > 1 files changed, 5 insertions(+), 0 deletions(-) > > > > > > diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h > > > index a658a58..d986929 100644 > > > --- a/include/openvswitch/flow.h > > > +++ b/include/openvswitch/flow.h > > > @@ -156,6 +156,11 @@ struct flow { > > > ovs_be32 igmp_group_ip4; /* IGMP group IPv4 address. > > > * Keep last for BUILD_ASSERT_DECL > below. */ > > > ovs_be32 pad3; /* Pad to 64 bits. */ > > > + > > > + /* SPI for ESP (64-bit aligned) */ > > > + /* XXX TO DO: move this to the l3 layer */ > > > + ovs_be32 spi; > > > + ovs_be32 pad4; > > > }; > > > > I'd like to see this moved to the L3 layer (as your comment says). > > Actually, I think I take that back. Isn't this logically part of L4? > (If so, then 'spi' can replace 'pad3' instead of being a doubleword of its > own.) No you're right, technically speaking IPsec is an L3 protocol. I may have caused confusion here and in the EMC extract patch I submitted as part of this, in the EMC extract ESP is treated along with other L4 protocols, the reason there was from a code execution point of view I felt it wasnt a typical case and I didn't want to introduce a redundant check for the EMC in each call looking for ESP at layer 3 as chances are it won't be an ESP header.
diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h index a658a58..d986929 100644 --- a/include/openvswitch/flow.h +++ b/include/openvswitch/flow.h @@ -156,6 +156,11 @@ struct flow { ovs_be32 igmp_group_ip4; /* IGMP group IPv4 address. * Keep last for BUILD_ASSERT_DECL below. */ ovs_be32 pad3; /* Pad to 64 bits. */ + + /* SPI for ESP (64-bit aligned) */ + /* XXX TO DO: move this to the l3 layer */ + ovs_be32 spi; + ovs_be32 pad4; }; BUILD_ASSERT_DECL(sizeof(struct flow) % sizeof(uint64_t) == 0); BUILD_ASSERT_DECL(sizeof(struct flow_tnl) % sizeof(uint64_t) == 0);
This patch adds a field to the flow struct to represent the ESP security parameter index of a packet. Signed-off-by: Ian Stokes <ian.stokes@intel.com> --- include/openvswitch/flow.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)