| Message ID | 1503679232-11135-3-git-send-email-ian.stokes@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Regards _Sugesh > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Ian Stokes > Sent: Friday, August 25, 2017 5:40 PM > To: dev@openvswitch.org > Subject: [ovs-dev] [RFC PATCH v2 02/10] openvswitch.h: add vport to > ovs_action_push_tnl. > > Upon callback for building/pushing a packet header when encapsulating for > tunneling a reference to the vport in question is required to access > associated devices such as cryptodevs. This patch adds this pointer and will > enable future work with cryptodevs that are associated with a vport. > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > --- > datapath/linux/compat/include/linux/openvswitch.h | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/datapath/linux/compat/include/linux/openvswitch.h > b/datapath/linux/compat/include/linux/openvswitch.h > index bc6c94b..afa7faf 100644 > --- a/datapath/linux/compat/include/linux/openvswitch.h > +++ b/datapath/linux/compat/include/linux/openvswitch.h > @@ -723,6 +723,8 @@ struct ovs_action_hash { > * @tnl_port: To identify tunnel port to pass header info. > * @out_port: Physical port to send encapsulated packet. > * @header_len: Length of the header to be pushed. > + * @dev: Pointer to vport so that the cryptodev parameters associated > + with the > + * vport can be accessed at the callback function. > * @tnl_type: This is only required to format this header. Otherwise > * ODP layer can not parse %header. > * @header: Partial header for the tunnel. Tunnel push action can use @@ - > 732,6 +734,7 @@ struct ovs_action_push_tnl { > odp_port_t tnl_port; > odp_port_t out_port; > uint32_t header_len; [Sugesh] Should we handle the vport down in the datapath? From the control path its just need to mention only about the Ipsec tunnel. When its handled in the datapath, it will translate into vdev implicitly. It looks to me this shouldn't here. May be will get good picture when I see the following patches. > + struct netdev_vport *dev; > uint32_t tnl_type; /* For logging. */ > uint32_t header[TNL_PUSH_HEADER_SIZE / 4]; }; > -- > 1.7.0.7 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Aug 25, 2017 at 05:40:24PM +0100, Ian Stokes wrote: > Upon callback for building/pushing a packet header when encapsulating > for tunneling a reference to the vport in question is required to access > associated devices such as cryptodevs. This patch adds this pointer and > will enable future work with cryptodevs that are associated with a > vport. > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > --- > datapath/linux/compat/include/linux/openvswitch.h | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h > index bc6c94b..afa7faf 100644 > --- a/datapath/linux/compat/include/linux/openvswitch.h > +++ b/datapath/linux/compat/include/linux/openvswitch.h > @@ -723,6 +723,8 @@ struct ovs_action_hash { > * @tnl_port: To identify tunnel port to pass header info. > * @out_port: Physical port to send encapsulated packet. > * @header_len: Length of the header to be pushed. > + * @dev: Pointer to vport so that the cryptodev parameters associated with the > + * vport can be accessed at the callback function. > * @tnl_type: This is only required to format this header. Otherwise > * ODP layer can not parse %header. > * @header: Partial header for the tunnel. Tunnel push action can use > @@ -732,6 +734,7 @@ struct ovs_action_push_tnl { > odp_port_t tnl_port; > odp_port_t out_port; > uint32_t header_len; > + struct netdev_vport *dev; > uint32_t tnl_type; /* For logging. */ > uint32_t header[TNL_PUSH_HEADER_SIZE / 4]; > }; Maybe this is safe for some reason, but I worry that there's the possibility of a use-after-free error. Is 'dev' supposed to hold a reference to the netdev (with netdev_ref())? If so, it would be good to document that in the comment.
> On Fri, Aug 25, 2017 at 05:40:24PM +0100, Ian Stokes wrote: > > Upon callback for building/pushing a packet header when encapsulating > > for tunneling a reference to the vport in question is required to > > access associated devices such as cryptodevs. This patch adds this > > pointer and will enable future work with cryptodevs that are > > associated with a vport. > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > > --- > > datapath/linux/compat/include/linux/openvswitch.h | 3 +++ > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > diff --git a/datapath/linux/compat/include/linux/openvswitch.h > > b/datapath/linux/compat/include/linux/openvswitch.h > > index bc6c94b..afa7faf 100644 > > --- a/datapath/linux/compat/include/linux/openvswitch.h > > +++ b/datapath/linux/compat/include/linux/openvswitch.h > > @@ -723,6 +723,8 @@ struct ovs_action_hash { > > * @tnl_port: To identify tunnel port to pass header info. > > * @out_port: Physical port to send encapsulated packet. > > * @header_len: Length of the header to be pushed. > > + * @dev: Pointer to vport so that the cryptodev parameters associated > > + with the > > + * vport can be accessed at the callback function. > > * @tnl_type: This is only required to format this header. Otherwise > > * ODP layer can not parse %header. > > * @header: Partial header for the tunnel. Tunnel push action can use > > @@ -732,6 +734,7 @@ struct ovs_action_push_tnl { > > odp_port_t tnl_port; > > odp_port_t out_port; > > uint32_t header_len; > > + struct netdev_vport *dev; > > uint32_t tnl_type; /* For logging. */ > > uint32_t header[TNL_PUSH_HEADER_SIZE / 4]; }; > > Maybe this is safe for some reason, but I worry that there's the > possibility of a use-after-free error. Is 'dev' supposed to hold a > reference to the netdev (with netdev_ref())? If so, it would be good to > document that in the comment. Sure, the purpose of dev here is to allow access to the ipsec options such as security association info (cipher/authentication type, keys etc.) associated with the vport to be referenced during the build_header phase of processing. For example this info would be required to know the length of the initialization vector for the packet header. I'm open to a better way of this info being shared to the build header stage for the vport but the only reference passed was a pointer to ovs_action_push_tnl struct I just want to get an opinion on using it this way. This was a quick way to enable this for RFC purposes, so there could be a use-after-free error (I didn't hit it in testing myself) but I'll look at it again. Thanks Ian
On Wed, Nov 01, 2017 at 04:14:23PM +0000, Stokes, Ian wrote: > > On Fri, Aug 25, 2017 at 05:40:24PM +0100, Ian Stokes wrote: > > > Upon callback for building/pushing a packet header when encapsulating > > > for tunneling a reference to the vport in question is required to > > > access associated devices such as cryptodevs. This patch adds this > > > pointer and will enable future work with cryptodevs that are > > > associated with a vport. > > > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > > > --- > > > datapath/linux/compat/include/linux/openvswitch.h | 3 +++ > > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > > > diff --git a/datapath/linux/compat/include/linux/openvswitch.h > > > b/datapath/linux/compat/include/linux/openvswitch.h > > > index bc6c94b..afa7faf 100644 > > > --- a/datapath/linux/compat/include/linux/openvswitch.h > > > +++ b/datapath/linux/compat/include/linux/openvswitch.h > > > @@ -723,6 +723,8 @@ struct ovs_action_hash { > > > * @tnl_port: To identify tunnel port to pass header info. > > > * @out_port: Physical port to send encapsulated packet. > > > * @header_len: Length of the header to be pushed. > > > + * @dev: Pointer to vport so that the cryptodev parameters associated > > > + with the > > > + * vport can be accessed at the callback function. > > > * @tnl_type: This is only required to format this header. Otherwise > > > * ODP layer can not parse %header. > > > * @header: Partial header for the tunnel. Tunnel push action can use > > > @@ -732,6 +734,7 @@ struct ovs_action_push_tnl { > > > odp_port_t tnl_port; > > > odp_port_t out_port; > > > uint32_t header_len; > > > + struct netdev_vport *dev; > > > uint32_t tnl_type; /* For logging. */ > > > uint32_t header[TNL_PUSH_HEADER_SIZE / 4]; }; > > > > Maybe this is safe for some reason, but I worry that there's the > > possibility of a use-after-free error. Is 'dev' supposed to hold a > > reference to the netdev (with netdev_ref())? If so, it would be good to > > document that in the comment. > > Sure, the purpose of dev here is to allow access to the ipsec options such as security association info (cipher/authentication type, keys etc.) associated with the vport to be referenced during the build_header phase of processing. For example this info would be required to know the length of the initialization vector for the packet header. I'm open to a better way of this info being shared to the build header stage for the vport but the only reference passed was a pointer to ovs_action_push_tnl struct I just want to get an opinion on using it this way. > > This was a quick way to enable this for RFC purposes, so there could be a use-after-free error (I didn't hit it in testing myself) but I'll look at it again. OK. We need to carefully review this in a later version of the patch set, then. It would be good to call that out in a comment or the commit message.
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h index bc6c94b..afa7faf 100644 --- a/datapath/linux/compat/include/linux/openvswitch.h +++ b/datapath/linux/compat/include/linux/openvswitch.h @@ -723,6 +723,8 @@ struct ovs_action_hash { * @tnl_port: To identify tunnel port to pass header info. * @out_port: Physical port to send encapsulated packet. * @header_len: Length of the header to be pushed. + * @dev: Pointer to vport so that the cryptodev parameters associated with the + * vport can be accessed at the callback function. * @tnl_type: This is only required to format this header. Otherwise * ODP layer can not parse %header. * @header: Partial header for the tunnel. Tunnel push action can use @@ -732,6 +734,7 @@ struct ovs_action_push_tnl { odp_port_t tnl_port; odp_port_t out_port; uint32_t header_len; + struct netdev_vport *dev; uint32_t tnl_type; /* For logging. */ uint32_t header[TNL_PUSH_HEADER_SIZE / 4]; };
Upon callback for building/pushing a packet header when encapsulating for tunneling a reference to the vport in question is required to access associated devices such as cryptodevs. This patch adds this pointer and will enable future work with cryptodevs that are associated with a vport. Signed-off-by: Ian Stokes <ian.stokes@intel.com> --- datapath/linux/compat/include/linux/openvswitch.h | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)