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

Jesse Gross jesse at nicira.com
Mon Aug 23 21:57:07 UTC 2010


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.




More information about the dev mailing list