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

[Simon Horman]

On Mon, Aug 23, 2010 at 05:57:07PM -0400, Jesse Gross wrote:
> On Mon, Aug 23, 2010 at 12:01 AM, Simon Horman <horms at verge.net.au> wrote:
> >> > @@ -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.
> 
> Yes, this is definitely the correct thing to do in the future.  If you
> think that we can get one now so that we can start using it, I'm fine
> with that.  However, it seems like it would be a little weird to have
> the flag in there before the code.

I'll see what I can do.