Skip to content

Commit

Permalink
macvtap: rework object lifetime rules
Browse files Browse the repository at this point in the history 
This reworks the change done by the previous patch
in a more complete way.

The original macvtap code has a number of problems
resulting from the use of RCU for protecting the
access to struct macvtap_queue from open files.

This includes
- need for GFP_ATOMIC allocations for skbs
- potential deadlocks when copy_*_user sleeps
- inability to work with vhost-net

Changing the lifetime of macvtap_queue to always
depend on the open file solves all these. The
RCU reference simply moves one step down to
the reference on the macvlan_dev, which we
only need for nonblocking operations.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Sridhar Samudrala <sri@us.ibm.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Arnd Bergmann authored and David S. Miller committed Feb 18, 2010
1 parent 37ee3d5 commit 02df55d
Showing 1 changed file with 91 additions and 92 deletions.
183 changes: 91 additions & 92 deletions drivers/net/macvtap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,30 +60,19 @@ static struct cdev macvtap_cdev;

/*
* RCU usage:
* The macvtap_queue is referenced both from the chardev struct file
* and from the struct macvlan_dev using rcu_read_lock.
* 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.
*
* We never actually update the contents of a macvtap_queue atomically
* with RCU but it is used for race-free destruction of a queue when
* either the file or the macvlan_dev goes away. Pointers back to
* the dev and the file are implicitly valid as long as the queue
* exists.
* Both the file and the macvlan_dev hold a reference on the macvtap_queue
* through sock_hold(&q->sk). When the macvlan_dev goes away first,
* q->vlan becomes inaccessible. When the files gets closed,
* macvtap_get_queue() fails.
*
* The callbacks from macvlan are always done with rcu_read_lock held
* already. For calls from file_operations, we use the rcu_read_lock_bh
* to get a reference count on the socket and the device.
*
* When destroying a queue, we remove the pointers from the file and
* from the dev and then synchronize_rcu to make sure no thread is
* still using the queue. There may still be references to the struct
* sock inside of the queue from outbound SKBs, but these never
* reference back to the file or the dev. The data structure is freed
* through __sk_free when both our references and any pending SKBs
* are gone.
*
* macvtap_lock is only used to prevent multiple concurrent open()
* calls to assign a new vlan->tap pointer. It could be moved into
* the macvlan_dev itself but is extremely rarely used.
* There may still be references to the struct sock inside of the
* queue from outbound SKBs, but these never reference back to the
* file or the dev. The data structure is freed through __sk_free
* when both our references and any pending SKBs are gone.
*/
static DEFINE_SPINLOCK(macvtap_lock);

Expand All @@ -101,40 +90,38 @@ static int macvtap_set_queue(struct net_device *dev, struct file *file,
goto out;

err = 0;
q->vlan = vlan;
rcu_assign_pointer(q->vlan, vlan);
rcu_assign_pointer(vlan->tap, q);
sock_hold(&q->sk);

q->file = file;
rcu_assign_pointer(file->private_data, q);
file->private_data = q;

out:
spin_unlock(&macvtap_lock);
return err;
}

/*
* We must destroy each queue exactly once, when either
* the netdev or the file go away.
* The file owning the queue got closed, give up both
* the reference that the files holds as well as the
* one from the macvlan_dev if that still exists.
*
* Using the spinlock makes sure that we don't get
* to the queue again after destroying it.
*
* synchronize_rcu serializes with the packet flow
* that uses rcu_read_lock.
*/
static void macvtap_del_queue(struct macvtap_queue **qp)
static void macvtap_put_queue(struct macvtap_queue *q)
{
struct macvtap_queue *q;
struct macvlan_dev *vlan;

spin_lock(&macvtap_lock);
q = rcu_dereference(*qp);
if (!q) {
spin_unlock(&macvtap_lock);
return;
vlan = rcu_dereference(q->vlan);
if (vlan) {
rcu_assign_pointer(vlan->tap, NULL);
rcu_assign_pointer(q->vlan, NULL);
sock_put(&q->sk);
}

rcu_assign_pointer(q->vlan->tap, NULL);
rcu_assign_pointer(q->file->private_data, NULL);
spin_unlock(&macvtap_lock);

synchronize_rcu();
Expand All @@ -152,29 +139,29 @@ static struct macvtap_queue *macvtap_get_queue(struct net_device *dev,
return rcu_dereference(vlan->tap);
}

/*
* The net_device is going away, give up the reference
* that it holds on the queue (all the queues one day)
* and safely set the pointer from the queues to NULL.
*/
static void macvtap_del_queues(struct net_device *dev)
{
struct macvlan_dev *vlan = netdev_priv(dev);
macvtap_del_queue(&vlan->tap);
}

static inline struct macvtap_queue *macvtap_file_get_queue(struct file *file)
{
struct macvtap_queue *q;
rcu_read_lock_bh();
q = rcu_dereference(file->private_data);
if (q) {
sock_hold(&q->sk);
dev_hold(q->vlan->dev);

spin_lock(&macvtap_lock);
q = rcu_dereference(vlan->tap);
if (!q) {
spin_unlock(&macvtap_lock);
return;
}
rcu_read_unlock_bh();
return q;
}

static inline void macvtap_file_put_queue(struct macvtap_queue *q)
{
rcu_assign_pointer(vlan->tap, NULL);
rcu_assign_pointer(q->vlan, NULL);
spin_unlock(&macvtap_lock);

synchronize_rcu();
sock_put(&q->sk);
dev_put(q->vlan->dev);
}

/*
Expand Down Expand Up @@ -284,7 +271,6 @@ static int macvtap_open(struct inode *inode, struct file *file)
q->sock.type = SOCK_RAW;
q->sock.state = SS_CONNECTED;
sock_init_data(&q->sock, &q->sk);
q->sk.sk_allocation = GFP_ATOMIC; /* for now */
q->sk.sk_write_space = macvtap_sock_write_space;

err = macvtap_set_queue(dev, file, q);
Expand All @@ -300,13 +286,14 @@ static int macvtap_open(struct inode *inode, struct file *file)

static int macvtap_release(struct inode *inode, struct file *file)
{
macvtap_del_queue((struct macvtap_queue **)&file->private_data);
struct macvtap_queue *q = file->private_data;
macvtap_put_queue(q);
return 0;
}

static unsigned int macvtap_poll(struct file *file, poll_table * wait)
{
struct macvtap_queue *q = macvtap_file_get_queue(file);
struct macvtap_queue *q = file->private_data;
unsigned int mask = POLLERR;

if (!q)
Expand All @@ -323,7 +310,6 @@ static unsigned int macvtap_poll(struct file *file, poll_table * wait)
sock_writeable(&q->sk)))
mask |= POLLOUT | POLLWRNORM;

macvtap_file_put_queue(q);
out:
return mask;
}
Expand All @@ -334,49 +320,56 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,
int noblock)
{
struct sk_buff *skb;
struct macvlan_dev *vlan;
size_t len = count;
int err;

if (unlikely(len < ETH_HLEN))
return -EINVAL;

skb = sock_alloc_send_skb(&q->sk, NET_IP_ALIGN + len, noblock, &err);

if (!skb) {
macvlan_count_rx(q->vlan, 0, false, false);
return err;
}
if (!skb)
goto err;

skb_reserve(skb, NET_IP_ALIGN);
skb_put(skb, count);

if (skb_copy_datagram_from_iovec(skb, 0, iv, 0, len)) {
macvlan_count_rx(q->vlan, 0, false, false);
kfree_skb(skb);
return -EFAULT;
}
err = skb_copy_datagram_from_iovec(skb, 0, iv, 0, len);
if (err)
goto err;

skb_set_network_header(skb, ETH_HLEN);

macvlan_start_xmit(skb, q->vlan->dev);
rcu_read_lock_bh();
vlan = rcu_dereference(q->vlan);
if (vlan)
macvlan_start_xmit(skb, vlan->dev);
else
kfree_skb(skb);
rcu_read_unlock_bh();

return count;

err:
rcu_read_lock_bh();
vlan = rcu_dereference(q->vlan);
if (vlan)
macvlan_count_rx(q->vlan, 0, false, false);
rcu_read_unlock_bh();

kfree_skb(skb);

return err;
}

static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv,
unsigned long count, loff_t pos)
{
struct file *file = iocb->ki_filp;
ssize_t result = -ENOLINK;
struct macvtap_queue *q = macvtap_file_get_queue(file);

if (!q)
goto out;
struct macvtap_queue *q = file->private_data;

result = macvtap_get_user(q, iv, iov_length(iv, count),
file->f_flags & O_NONBLOCK);
macvtap_file_put_queue(q);
out:
return result;
}

Expand All @@ -385,14 +378,17 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,
const struct sk_buff *skb,
const struct iovec *iv, int len)
{
struct macvlan_dev *vlan = q->vlan;
struct macvlan_dev *vlan;
int ret;

len = min_t(int, skb->len, len);

ret = skb_copy_datagram_const_iovec(skb, 0, iv, 0, len);

rcu_read_lock_bh();
vlan = rcu_dereference(q->vlan);
macvlan_count_rx(vlan, len, ret == 0, 0);
rcu_read_unlock_bh();

return ret ? ret : len;
}
Expand All @@ -401,14 +397,16 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,
unsigned long count, loff_t pos)
{
struct file *file = iocb->ki_filp;
struct macvtap_queue *q = macvtap_file_get_queue(file);
struct macvtap_queue *q = file->private_data;

DECLARE_WAITQUEUE(wait, current);
struct sk_buff *skb;
ssize_t len, ret = 0;

if (!q)
return -ENOLINK;
if (!q) {
ret = -ENOLINK;
goto out;
}

len = iov_length(iv, count);
if (len < 0) {
Expand Down Expand Up @@ -444,7 +442,6 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,
remove_wait_queue(q->sk.sk_sleep, &wait);

out:
macvtap_file_put_queue(q);
return ret;
}

Expand All @@ -454,12 +451,13 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,
static long macvtap_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
struct macvtap_queue *q;
struct macvtap_queue *q = file->private_data;
struct macvlan_dev *vlan;
void __user *argp = (void __user *)arg;
struct ifreq __user *ifr = argp;
unsigned int __user *up = argp;
unsigned int u;
char devname[IFNAMSIZ];
int ret;

switch (cmd) {
case TUNSETIFF:
Expand All @@ -471,16 +469,21 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
return 0;

case TUNGETIFF:
q = macvtap_file_get_queue(file);
if (!q)
rcu_read_lock_bh();
vlan = rcu_dereference(q->vlan);
if (vlan)
dev_hold(vlan->dev);
rcu_read_unlock_bh();

if (!vlan)
return -ENOLINK;
memcpy(devname, q->vlan->dev->name, sizeof(devname));
macvtap_file_put_queue(q);

ret = 0;
if (copy_to_user(&ifr->ifr_name, q->vlan->dev->name, IFNAMSIZ) ||
put_user((TUN_TAP_DEV | TUN_NO_PI), &ifr->ifr_flags))
return -EFAULT;
return 0;
ret = -EFAULT;
dev_put(vlan->dev);
return ret;

case TUNGETFEATURES:
if (put_user((IFF_TAP | IFF_NO_PI), up))
Expand All @@ -491,11 +494,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
if (get_user(u, up))
return -EFAULT;

q = macvtap_file_get_queue(file);
if (!q)
return -ENOLINK;
q->sk.sk_sndbuf = u;
macvtap_file_put_queue(q);
return 0;

case TUNSETOFFLOAD:
Expand Down

0 comments on commit 02df55d

Please sign in to comment.