[ovs-dev] [patch v2 5/7] datapath: use rx_handler_data pointer

[Simon Horman]

On Fri, Aug 20, 2010 at 03:43:03PM -0400, Jesse Gross wrote:
> On Wed, Aug 18, 2010 at 11:36 PM, Simon Horman <horms at verge.net.au> wrote:
> > @@ -146,15 +146,14 @@ static int netdev_attach(struct vport *v
> >        struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
> >        int err;
> >
> > -       rcu_assign_pointer(netdev_vport->dev->br_port,
> > -                          (struct net_bridge_port *)vport);
> >        err = netdev_rx_handler_register(netdev_vport->dev, netdev_frame_hook,
> > -                                        NULL);
> > +                                        (struct net_bridge_port *)vport);
> 
> We don't need to cast to struct net_bridge_port here - the argument is
> a void *.  We don't want to lie about using the bridge's data
> structures when we don't have to.

Agreed, I have removed the cast. (What I was thinking?!)

> > @@ -300,7 +299,11 @@ static int netdev_send(struct vport *vpo
> >  /* Returns null if this device is not attached to a datapath. */
> >  struct vport *netdev_get_vport(struct net_device *dev)
> >  {
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,36)
> > +       return (struct vport *)rcu_dereference(dev->rx_handler_data);
> > +#else
> >        return (struct vport *)rcu_dereference(dev->br_port);
> > +#endif
> >  }
> 
> There's a potential memory corruption issue here.  There are two
> callers of this function: the one you just added in the new
> netdev_frame_hook(), which is OK and one in dp_notify.c.  The second
> caller assumes that if this function returns a non-NULL value the
> device is actually attached to Open vSwitch.  This was true before
> because we enforced exclusion with the bridge module so we were the
> only one who would write to dev->br_port.
> 
> However, now it is possible that a device could be attached to the
> bridge, which would populate dev->rx_handler_data, we would assume
> that it is actually a struct vport *, and bad things would happen.
> 
> The right way to do this would be to check for IFF_OVS_DATAPATH.
> However, seeing as it is currently defined to IFF_BRIDGE_PORT that
> won't help us much.  Probably the best thing to do is check the packet
> handler pointer and fix this up after the merge.

Sure I will do that for now.

	/* Returns null if this device is not attached to a datapath. */
	struct vport *netdev_get_vport(struct net_device *dev)
	{
	#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,36)
		/* XXX: The bridge code may have registered the data.
		 * So check that the handler pointer is the datapath's.
		 * Once the merge is done and IFF_OVS_DATAPATH stops
		 * being the same value as IFF_BRIDGE_PORT the check can
		 * simply be netdev_vport->dev->priv_flags & IFF_OVS_DATAPATH.
		 */
		if (rcu_dereference(dev->rx_handler) != netdev_frame_hook)
			return NULL;
		return (struct vport *)rcu_dereference(dev->rx_handler_data);
	#else
		return (struct vport *)rcu_dereference(dev->br_port);
	#endif
	}

But another approach might be to get a value for IFF_OVS_DATAPATH
from the Linux netdev people and start using it.

> > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,36)
> >  /*
> >  * Open vSwitch cannot safely coexist with the Linux bridge module on any
> >  * released version of Linux, because there is only a single bridge hook
> 
> Do you mind updating the comment here to say that it only pertains to
> kernels < 2.6.36?

Done.

/*
 * In kernels earlier than 2.6.36, Open vSwitch cannot safely coexist with
 * the Linux bridge module on any released version of Linux, because there
 * is only a single bridge hook function and only a single br_port member
 * in struct net_device.
 *
 ...