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

Simon Horman horms at verge.net.au
Tue Aug 24 06:47:29 UTC 2010


On Tue, Aug 24, 2010 at 10:01:45AM +0900, Simon Horman wrote:
> 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.

I was partially successful.

#define IFF_OVS_DATAPATH	0x10000
has been added to net-next-2.6 and should appear in 2.6.37.

However, priv_flags was only 16 bits wide and as all 16 bits were
already in use I had to increase it to 32 bits. That change
also won't appear until 2.6.37. So we can't use IFF_OVS_DATAPATH=0x10000
until then (even if we have a local copy of the #define).

I think that the result is that we can use IFF_OVS_DATAPATH=0x10000
in the code we submit for merging, or change to use it
straight after a merge. But we still need to rely on comparing
dev->rx_handler for 2.6.36.

http://marc.info/?l=linux-netdev&m=128262559615801&w=2




More information about the dev mailing list