netdev - Re: [PATCH net-next 1/2] macvtap: Let TUNSETOFFLOAD actually controll offload features.

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: <1371656769.3252.320.camel@edumazet-glaptop>
Date:	Wed, 19 Jun 2013 08:46:09 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	vyasevic@...hat.com
Cc:	netdev@...r.kernel.org, davem@...emloft.net, mst@...hat.com,
	jasowang@...hat.com
Subject: Re: [PATCH net-next 1/2] macvtap: Let TUNSETOFFLOAD actually
 controll offload features.

On Wed, 2013-06-19 at 11:26 -0400, Vlad Yasevich wrote:

> I think I do since vlan pointer may change even when I am holding
> rtnl.  rtnl is needed to change features.  rcu is needed to get
> the vlan pointer.
> 
> > (A BH handler will not change q->vlan )
> 
> No, but the _bh rcu calls seem to be used when dereferencing q->vlan.
> I am not sure the reason for that...

You mix the reader/fast path, properly using RCU,
and the writer path, using macvtap_lock ( a spinlock ).

That's clear sign you missed something.

> 
> >
> > BTW, it looks like ->vlan is protected by macvtap_lock
> 
> Right.  This is why I use rcu to get vlan.  rtnl is needed to avoid
> asserts in the feature change code.

The management should be allowed to sleep, and rcu_read_lock_bh()
disallows that.

Maybe some driver callback will really sleep and crash after your patch.

vi +69 drivers/net/macvtap.c

/*
 * RCU usage:
 * The macvtap_queue and the macvlan_dev are loosely coupled, the
 * pointers from one to the other can only be read while rcu_read_lock
 * or macvtap_lock is held.

Your patch does not respect the rules of this driver.

macvtap_lock is always acquired from process context, without any need
for _bh variant.

Quite frankly, I would switch this driver to use a mutex for
macvtap_lock. 

And simply remove it, as RTNL is most probably already owned.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html