linux-kernel - Re: [ 42/79] vlan: fix a race in egress prio management

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1374946729.3669.5.camel@edumazet-glaptop>
Date:	Sat, 27 Jul 2013 10:38:49 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Ben Hutchings <ben@...adent.org.uk>
Cc:	Eric Dumazet <edumazet@...gle.com>, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org, Patrick McHardy <kaber@...sh.net>,
	"David S. Miller" <davem@...emloft.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [ 42/79] vlan: fix a race in egress prio management

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@...gle.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@...gle.com>
> > Cc: Patrick McHardy <kaber@...sh.net>
> > Signed-off-by: David S. Miller <davem@...emloft.net>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.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.

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;"



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/