[ovs-dev] [RFC PATCH v2 02/10] openvswitch.h: add vport to ovs_action_push_tnl.

Stokes, Ian ian.stokes at intel.com
Wed Nov 1 16:14:23 UTC 2017


> 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 at 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


More information about the dev mailing list