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

[Jesse Gross]

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.

> @@ -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.

> +#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?