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 <[email protected]> > > > --- > > > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
