lkml.org 
[lkml]   [2013]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [ 42/79] vlan: fix a race in egress prio management
    From
    Date
    On Sat, 2013-07-27 at 10:38 -0700, Eric Dumazet wrote:
    > On Sat, 2013-07-27 at 17:55 +0100, Ben Hutchings wrote:
    > > On Fri, 2013-07-26 at 13:47 -0700, Greg Kroah-Hartman wrote:
    > > > 3.10-stable review patch. If anyone has any objections, please let me know.
    > > >
    > > > ------------------
    > > >
    > > > From: Eric Dumazet <edumazet@google.com>
    > > >
    > > > [ Upstream commit 3e3aac497513c669e1c62c71e1d552ea85c1d974 ]
    > > >
    > > > egress_priority_map[] hash table updates are protected by rtnl,
    > > > and we never remove elements until device is dismantled.
    > > >
    > > > We have to make sure that before inserting an new element in hash table,
    > > > all its fields are committed to memory or else another cpu could
    > > > find corrupt values and crash.
    > > >
    > > > Signed-off-by: Eric Dumazet <edumazet@google.com>
    > > > Cc: Patrick McHardy <kaber@trash.net>
    > > > Signed-off-by: David S. Miller <davem@davemloft.net>
    > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    > > > ---
    > > > net/8021q/vlan_dev.c | 7 +++++++
    > > > 1 file changed, 7 insertions(+)
    > > >
    > > > --- a/net/8021q/vlan_dev.c
    > > > +++ b/net/8021q/vlan_dev.c
    > > > @@ -73,6 +73,8 @@ vlan_dev_get_egress_qos_mask(struct net_
    > > > {
    > > > struct vlan_priority_tci_mapping *mp;
    > > >
    > > > + smp_rmb(); /* coupled with smp_wmb() in vlan_dev_set_egress_priority() */
    > > > +
    > > > mp = vlan_dev_priv(dev)->egress_priority_map[(skb->priority & 0xF)];
    > >
    > > This barrier is the wrong type in the wrong place; there should be a
    > > smp_read_barrier_depends() *after* reading the pointer from
    > > egress_priority_map, and after reading each next pointer.
    > >
    >
    > Not sure its a problem anyway, as we insert new items at the beginning
    > of the chain, and we never delete any item. What you describe would make
    > sense if we were deleting an item.
    >
    > Once a reader gets a pointer, the whole chain is stable.

    For every sane architecture, data-dependency is an automatic read
    barrier. But Alpha is weird: it can prefetch mp->foo using a speculated
    value for mp before it sees mp. If the speculation was correct it won't
    get the correct value of mp->foo. Hence smp_read_barrier_depends().

    > The important part of the patch was the smp_wmb(), to make sure np->next
    > was committed before "vlan->egress_priority_map[skb_prio & 0xF] = np;"

    Sure. But the smp_rmb() you added doesn't synchronise with it (a
    barrier is always between operations, but where is the 'before'
    operation here?).

    Ben.

    --
    Ben Hutchings
    Once a job is fouled up, anything done to improve it makes it worse.
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2013-07-27 20:21    [W:4.179 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site