macvtap driver
Subsystems (5)
v3 cover macvtap driver
This is the third version of the macvtap device driver, following another major restructuring and a lot of bug fixes:
* Change macvtap to be based around a struct sock
* macvtap: fix initialization
* return 0 to netlink
* don't use rcu for q->file and q->vlan pointers
* macvtap: checkpatch.pl fixes
* macvtap: fix tun IFF flags
* Use a struct socket to make tx flow control work
* disable BH processing during transmit
* only add an ethernet header for receive not forward
* allocate the SKB using GFP_NOWAIT since we're in rcu_read_lock
* use atomic allocation for socket
* fix blocking on send
* do not destroy netdev twice in error path
There are still known problems, but unless there are fundamental concerns, I'd like this to go into net-next as an experimental driver, fixing up the remaining problems by 2.6.34-rc1.
Arnd
PATCH v3 1/3 net: maintain namespace isolation between vlan and real device
In the vlan and macvlan drivers, the start_xmit function forwards data to the dev_queue_xmit function for another device, which may potentially belong to a different namespace.
To make sure that classification stays within a single namespace, this resets the potentially critical fields.
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/macvlan.c | 2 +- include/linux/netdevice.h | 9 +++++++++ net/8021q/vlan_dev.c | 2 +- net/core/dev.c | 35 +++++++++++++++++++++++++++++++---- 4 files changed, 42 insertions(+), 6 deletions(-)
drivers/net/macvlan.c +1 -1
@@ -269,7 +269,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev) } xmit_world: - skb->dev = vlan->lowerdev; +skb_set_dev (skb, vlan->lowerdev);return dev_queue_xmit (skb); }
include/linux/netdevice.h +9 -0
@@ -1004,6 +1004,15 @@ static inline bool netdev_uses_dsa_tags(struct net_device *dev)return 0 ; } +#ifndef CONFIG_NET_NS +static inline void skb_set_dev (struct sk_buff *skb,struct net_device *dev) +{ + skb->dev = dev; +} +#else /* CONFIG_NET_NS */ +void skb_set_dev (struct sk_buff *skb,struct net_device *dev); +#endif +static inline bool netdev_uses_trailer_tags (struct net_device *dev) {#ifdef CONFIG_NET_DSA_TAG_TRAILER
net/8021q/vlan_dev.c +1 -1
@@ -322,7 +322,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb, } - skb->dev =vlan_dev_info (dev)->real_dev; +skb_set_dev (skb,vlan_dev_info (dev)->real_dev); len = skb->len; ret =dev_queue_xmit (skb);
net/core/dev.c +31 -4
@@ -1448,13 +1448,10 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)if (skb->len > (dev->mtu + dev->hard_header_len))return NET_RX_DROP ; -skb_dst_drop (skb); +skb_set_dev (skb, dev); skb->tstamp.tv64 =0 ; skb->pkt_type =PACKET_HOST ; skb->protocol =eth_type_trans (skb, dev); - skb->mark =0 ; -secpath_reset (skb); -nf_reset (skb);return netif_rx (skb); }EXPORT_SYMBOL_GPL (dev_forward_skb);
@@ -1614,6 +1611,36 @@ static bool dev_can_checksum(struct net_device *dev, struct sk_buff *skb)return false; } +/** +* skb_dev_set -- assign a buffer to a new device +* @skb: buffer for the new device +* @dev: network device +* +* If an skb is owned by a device already, we have to reset +* all data private to the namespace a device belongs to +* before assigning it a new device. +*/ +#ifdef CONFIG_NET_NS +void skb_set_dev (struct sk_buff *skb,struct net_device *dev) +{ +skb_dst_drop (skb); +if (skb->dev && !net_eq (dev_net (skb->dev),dev_net (dev))) { +secpath_reset (skb); +nf_reset (skb); +skb_init_secmark (skb); + skb->mark =0 ; + skb->priority =0 ; + skb->nf_trace =0 ; + skb->ipvs_property =0 ; +#ifdef CONFIG_NET_SCHED + skb->tc_index =0 ; +#endif + } + skb->dev = dev; +} +EXPORT_SYMBOL (skb_set_dev); +#endif /* CONFIG_NET_NS */ +/* * Invalidate hardware checksum when packet is to be mangled, and * complete checksum manually on outgoing path.
Signature · 1 line
1.6.3.3
David Miller
maintainerRe: [PATCH 1/3] net: maintain namespace isolation between vlan and real device
From: Arnd Bergmann <[email protected]>
Date: Wed, 27 Jan 2010 11:05:15 +0100
Quoted 1 line
+ *
skb_dev_set-- assign a buffer to a new device
My english is terrible, but I think this should be "assign a new device to a buffer".
If you agree, please fix this up when you resubmit patches #1 and #2 along with the fix you already plan to make to patch #3.
Thanks!
Re: [PATCH 1/3] net: maintain namespace isolation between vlan and real device
Quoted 7 lines · David Miller
From: Arnd Bergmann <[email protected]>
Date: Wed, 27 Jan 2010 11:05:15 +0100Quoted 1 line
+ *
skb_dev_set-- assign a buffer to a new deviceMy english is terrible, but I think this should be "assign a new device to a buffer".
Right, that seems clearer.
Quoted 2 lines
If you agree, please fix this up when you resubmit patches #1 and #2 along with the fix you already plan to make to patch #3.
Ok, will do.
Thanks,
Arnd
PATCH v3 2/3 net/macvlan: allow multiple driver backends
In reply to Arnd BergmannThis makes it possible to hook into the macvlan driver from another kernel module. In particular, the goal is to extend it with the macvtap backend that provides a tun/tap compatible interface directly on the macvlan device.
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/macvlan.c | 113 +++++++++++++++++++------------------------- include/linux/if_macvlan.h | 70 +++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 64 deletions(-)
drivers/net/macvlan.c +49 -64
@@ -39,31 +39,6 @@ struct macvlan_port {struct list_head vlans; }; -/** -* struct macvlan_rx_stats - MACVLAN percpu rx stats -* @rx_packets: number of received packets -* @rx_bytes: number of received bytes -* @multicast: number of received multicast packets -* @rx_errors: number of errors -*/ -struct macvlan_rx_stats { -unsigned long rx_packets; -unsigned long rx_bytes; -unsigned long multicast; -unsigned long rx_errors; -}; - -struct macvlan_dev { -struct net_device *dev; -struct list_head list; -struct hlist_node hlist; -struct macvlan_port *port; -struct net_device *lowerdev; -struct macvlan_rx_stats *rx_stats; -enum macvlan_mode mode; -}; - -static struct macvlan_dev *macvlan_hash_lookup (const struct macvlan_port *port,const unsigned char *addr) {
@@ -118,31 +93,17 @@ static int macvlan_addr_busy(const struct macvlan_port *port,return 0 ; } -static inline void macvlan_count_rx (const struct macvlan_dev *vlan, -unsigned int len,bool success, -bool multicast) -{ -struct macvlan_rx_stats *rx_stats; - - rx_stats =per_cpu_ptr (vlan->rx_stats,smp_processor_id ()); -if (likely (success)) { - rx_stats->rx_packets++;; - rx_stats->rx_bytes += len; -if (multicast) - rx_stats->multicast++; - }else { - rx_stats->rx_errors++; - } -} -static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev, +static int macvlan_broadcast_one(struct sk_buff *skb, + const struct macvlan_dev *vlan, const struct ethhdr *eth, bool local) { + struct net_device *dev = vlan->dev; if (!skb) return NET_RX_DROP; if (local) - return dev_forward_skb(dev, skb); + return vlan->forward(dev, skb); skb->dev = dev; if (!compare_ether_addr_64bits(eth->h_dest,
@@ -151,7 +112,7 @@ static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,else skb->pkt_type =PACKET_MULTICAST ; -return netif_receive_skb (skb); +return vlan->receive (skb); } static void macvlan_broadcast(struct sk_buff *skb,
@@ -175,7 +136,7 @@ static void macvlan_broadcast(struct sk_buff *skb,continue ; nskb =skb_clone (skb,GFP_ATOMIC ); - err =macvlan_broadcast_one (nskb, vlan->dev, eth, + err =macvlan_broadcast_one (nskb, vlan, eth, mode ==MACVLAN_MODE_BRIDGE );macvlan_count_rx (vlan, skb->len +ETH_HLEN , err ==NET_RX_SUCCESS ,1 );
@@ -238,7 +199,7 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb) skb->dev = dev; skb->pkt_type =PACKET_HOST ; -netif_receive_skb (skb); + vlan->receive (skb);return NULL ; }
@@ -260,7 +221,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev) dest =macvlan_hash_lookup (port, eth->h_dest);if (dest && dest->mode ==MACVLAN_MODE_BRIDGE ) {unsigned int length = skb->len +ETH_HLEN ; -int ret =dev_forward_skb (dest->dev, skb); +int ret = dest->forward (dest->dev, skb);macvlan_count_rx (dest, length, ret ==NET_RX_SUCCESS ,0 );
@@ -273,8 +234,8 @@ xmit_world:return dev_queue_xmit (skb); } -static netdev_tx_t macvlan_start_xmit (struct sk_buff *skb, -struct net_device *dev) +netdev_tx_t macvlan_start_xmit (struct sk_buff *skb, +struct net_device *dev) {int i =skb_get_queue_mapping (skb);struct netdev_queue *txq =netdev_get_tx_queue (dev, i);
@@ -290,6 +251,7 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,return ret; } +EXPORT_SYMBOL_GPL (macvlan_start_xmit); static int macvlan_hard_header(struct sk_buff *skb, struct net_device *dev, unsigned short type, const void *daddr,
@@ -623,8 +585,11 @@ static int macvlan_get_tx_queues(struct net *net,return 0 ; } -static int macvlan_newlink (struct net *src_net,struct net_device *dev, -struct nlattr *tb[],struct nlattr *data[]) +int macvlan_common_newlink (struct net *src_net,struct net_device *dev, +struct nlattr *tb[],struct nlattr *data[], +int (*receive)(struct sk_buff *skb), +int (*forward)(struct net_device *dev, +struct sk_buff *skb)) {struct macvlan_dev *vlan =netdev_priv (dev);struct macvlan_port *port;
@@ -664,6 +629,8 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev, vlan->lowerdev = lowerdev; vlan->dev = dev; vlan->port = port; + vlan->receive = receive; + vlan->forward = forward; vlan->mode =MACVLAN_MODE_VEPA ;if (data && data[IFLA_MACVLAN_MODE ])
@@ -677,8 +644,17 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,netif_stacked_transfer_operstate (lowerdev, dev);return 0 ; } +EXPORT_SYMBOL_GPL (macvlan_common_newlink); -static void macvlan_dellink (struct net_device *dev,struct list_head *head) +static int macvlan_newlink (struct net *src_net,struct net_device *dev, +struct nlattr *tb[],struct nlattr *data[]) +{ +return macvlan_common_newlink (src_net, dev, tb, data, + netif_receive_skb, + dev_forward_skb); +} + +void macvlan_dellink (struct net_device *dev,struct list_head *head) {struct macvlan_dev *vlan =netdev_priv (dev);struct macvlan_port *port = vlan->port;
@@ -689,6 +665,7 @@ static void macvlan_dellink(struct net_device *dev, struct list_head *head)if (list_empty (&port->vlans))macvlan_port_destroy (port->dev); } +EXPORT_SYMBOL_GPL (macvlan_dellink);static int macvlan_changelink (struct net_device *dev,struct nlattr *tb[],struct nlattr *data[])
@@ -720,19 +697,27 @@ static const struct nla_policy macvlan_policy[IFLA_MACVLAN_MAX + 1] = { [IFLA_MACVLAN_MODE] = { .type = NLA_U32 }, }; -static struct rtnl_link_ops macvlan_link_ops__read_mostly = { +int macvlan_link_register (struct rtnl_link_ops *ops) +{ +/* common fields */ + ops->priv_size =sizeof (struct macvlan_dev ); + ops->get_tx_queues = macvlan_get_tx_queues; + ops->setup = macvlan_setup; + ops->validate = macvlan_validate; + ops->maxtype =IFLA_MACVLAN_MAX ; + ops->policy = macvlan_policy; + ops->changelink = macvlan_changelink; + ops->get_size = macvlan_get_size; + ops->fill_info = macvlan_fill_info; + +return rtnl_link_register (ops); +}; +EXPORT_SYMBOL_GPL (macvlan_link_register); + +static struct rtnl_link_ops macvlan_link_ops = { .kind ="macvlan" , - .priv_size =sizeof (struct macvlan_dev ), - .get_tx_queues = macvlan_get_tx_queues, - .setup = macvlan_setup, - .validate = macvlan_validate, .newlink = macvlan_newlink, .dellink = macvlan_dellink, - .maxtype =IFLA_MACVLAN_MAX , - .policy = macvlan_policy, - .changelink = macvlan_changelink, - .get_size = macvlan_get_size, - .fill_info = macvlan_fill_info, }; static int macvlan_device_event(struct notifier_block *unused,
@@ -761,7 +746,7 @@ static int macvlan_device_event(struct notifier_block *unused,break ;case NETDEV_UNREGISTER :list_for_each_entry_safe (vlan, next, &port->vlans, list) -macvlan_dellink (vlan->dev,NULL ); + vlan->dev->rtnl_link_ops->dellink (vlan->dev,NULL );break ; }return NOTIFY_DONE ;
@@ -778,7 +763,7 @@ static int __init macvlan_init_module(void)register_netdevice_notifier (&macvlan_notifier_block); macvlan_handle_frame_hook = macvlan_handle_frame; - err =rtnl_link_register (&macvlan_link_ops); + err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ;
include/linux/if_macvlan.h +70 -0
76 lines not on this page · raw message
Signature · 1 line
1.6.3.3
PATCH v3 3/3 net: macvtap driver
In reply to Arnd BergmannIn order to use macvlan with qemu and other tools that require a tap file descriptor, the macvtap driver adds a small backend with a character device with the same interface as the tun driver, with a minimum set of features.
Macvtap interfaces are created in the same way as macvlan interfaces using ip link, but the netif is just used as a handle for configuration and accounting, while the data goes through the chardev. Each macvtap interface has its own character device, simplifying permission management significantly over the generic tun/tap driver.
Cc Patrick McHardy <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc David S. Miller" <[email protected]>
Cc "Michael S. Tsirkin" <[email protected]>
Cc Herbert Xu <[email protected]>
Cc Or Gerlitz <[email protected]>
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/Kconfig | 12 + drivers/net/Makefile | 1 + drivers/net/macvtap.c | 572 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/if_macvlan.h | 1 + 4 files changed, 586 insertions(+), 0 deletions(-) create mode 100644 drivers/net/macvtap.c
drivers/net/Kconfig +12 -0
@@ -90,6 +90,18 @@ config MACVLAN To compile this driver as a module, choose M here: the module will be called macvlan. +config MACVTAP + tristate "MAC-VLAN based tap driver (EXPERIMENTAL)" + depends on MACVLAN + help + This adds a specialized tap character device driver that is based + on the MAC-VLAN network interface, called macvtap. A macvtap device + can be added in the same way as a macvlan device, using 'type + macvlan', and then be accessed through the tap user space interface. + + To compile this driver as a module, choose M here: the module + will be called macvtap. + config EQUALIZER tristate "EQL (serial line load balancing) support" ---help---
drivers/net/Makefile +1 -0
@@ -169,6 +169,7 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND) += xen-netfront.o obj-$(CONFIG_DUMMY) += dummy.o obj-$(CONFIG_IFB) += ifb.o obj-$(CONFIG_MACVLAN) += macvlan.o +obj-$(CONFIG_MACVTAP) += macvtap.o obj-$(CONFIG_DE600) += de600.o obj-$(CONFIG_DE620) += de620.o obj-$(CONFIG_LANCE) += lance.o
drivers/net/macvtap.c new file +572 -0
572 lines not on this page · raw message
include/linux/if_macvlan.h +1 -0
@@ -34,6 +34,7 @@ struct macvlan_dev {enum macvlan_mode mode;int (*receive)(struct sk_buff *skb);int (*forward)(struct net_device *dev,struct sk_buff *skb); +struct macvtap_queue *tap; }; static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
Signature · 1 line
1.6.3.3
Michael S. Tsirkin
maintainerRe: [PATCH 3/3] net: macvtap driver
Quoted 231 lines · Arnd Bergmann
In order to use macvlan with qemu and other tools that require a tap file descriptor, the macvtap driver adds a small backend with a character device with the same interface as the tun driver, with a minimum set of features.
Macvtap interfaces are created in the same way as macvlan interfaces using ip link, but the netif is just used as a handle for configuration and accounting, while the data goes through the chardev. Each macvtap interface has its own character device, simplifying permission management significantly over the generic tun/tap driver.
Cc Patrick McHardy <[email protected]>Cc Stephen Hemminger <[email protected]>Cc David S. Miller" <[email protected]>Cc "Michael S. Tsirkin" <[email protected]>Cc Herbert Xu <[email protected]>Cc Or Gerlitz <[email protected]>Signed-off-by Arnd Bergmann <[email protected]>--- drivers/net/Kconfig | 12 + drivers/net/Makefile | 1 + drivers/net/macvtap.c | 572 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/if_macvlan.h | 1 + 4 files changed, 586 insertions(+), 0 deletions(-) create mode 100644 drivers/net/macvtap.cdrivers/net/Kconfig +12
-0@@ -90,6 +90,18 @@ config MACVLAN To compile this driver as a module, choose M here: the module will be called macvlan. +config MACVTAP + tristate "MAC-VLAN based tap driver (EXPERIMENTAL)" + depends on MACVLAN + help + This adds a specialized tap character device driver that is based + on the MAC-VLAN network interface, called macvtap. A macvtap device + can be added in the same way as a macvlan device, using 'type + macvlan', and then be accessed through the tap user space interface. + + To compile this driver as a module, choose M here: the module + will be called macvtap. + config EQUALIZER tristate "EQL (serial line load balancing) support" ---help---drivers/net/Makefile +1
-0@@ -169,6 +169,7 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND) += xen-netfront.o obj-$(CONFIG_DUMMY) += dummy.o obj-$(CONFIG_IFB) += ifb.o obj-$(CONFIG_MACVLAN) += macvlan.o +obj-$(CONFIG_MACVTAP) += macvtap.o obj-$(CONFIG_DE600) += de600.o obj-$(CONFIG_DE620) += de620.o obj-$(CONFIG_LANCE) += lance.odrivers/net/macvtap.c new file +160
-0@@ -0,0 +1,572 @@ +#include <linux/etherdevice.h> +#include <linux/if_macvlan.h> +#include <linux/interrupt.h> +#include <linux/nsproxy.h> +#include <linux/compat.h> +#include <linux/if_tun.h> +#include <linux/module.h> +#include <linux/skbuff.h> +#include <linux/cache.h> +#include <linux/sched.h> +#include <linux/types.h> +#include <linux/init.h> +#include <linux/wait.h> +#include <linux/cdev.h> +#include <linux/fs.h> + +#include <net/net_namespace.h> +#include <net/rtnetlink.h> +#include <net/sock.h> + +/* +* A macvtap queue is the central object of this driver, it connects +* an open character device to a macvlan interface. There can be +* multiple queues on one interface, which map back to queues +* implemented in hardware on the underlying device. +* +* macvtap_proto is used to allocate queues through the sock allocation +* mechanism. +* +* TODO: multiqueue support is currently not implemented, even though +* macvtap is basically prepared for that. We will need to add this +* here as well as in virtio-net and qemu to get line rate on 10gbit +* adapters from a guest. +*/ +struct macvtap_queue { +struct sock sk; +struct socket sock; +struct macvlan_dev *vlan; +struct file *file; +}; + +static struct proto macvtap_proto = { + .name ="macvtap" , + .owner =THIS_MODULE , + .obj_size =sizeof (struct macvtap_queue ), +}; + +/* +* Minor number matches netdev->ifindex, so need a potentially +* large value. This also makes it possible to split the +* tap functionality out again in the future by offering it +* from other drivers besides macvtap. As long as every device +* only has one tap, the interface numbers assure that the +* device nodes are unique. +*/ +static unsigned int macvtap_major; +#define MACVTAP_NUM_DEVS 65536 +static struct class *macvtap_class; +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. +* +* 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. +* +* The callbacks from macvlan are always done with rcu_read_lock held +* already, while in the file_operations, we get it ourselves. +* +* 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. +*/ +static DEFINE_SPINLOCK (macvtap_lock); + +/* +* Choose the next free queue, for now there is only one +*/ +static int macvtap_set_queue (struct net_device *dev,struct file *file, +struct macvtap_queue *q) +{ +struct macvlan_dev *vlan =netdev_priv (dev); +int err = -EBUSY ; + +spin_lock (&macvtap_lock); +if (rcu_dereference (vlan->tap)) + goto out; + + err =0 ; + q->vlan = vlan; +rcu_assign_pointer (vlan->tap, q); + + q->file = file; +rcu_assign_pointer (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. +* +* 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) +{ +struct macvtap_queue *q; + +spin_lock (&macvtap_lock); + q =rcu_dereference (*qp); +if (!q) { +spin_unlock (&macvtap_lock); +return ; + } + +rcu_assign_pointer (q->vlan->tap,NULL ); +rcu_assign_pointer (q->file->private_data,NULL ); +spin_unlock (&macvtap_lock); + +synchronize_rcu (); +sock_put (&q->sk); +} + +/* +* Since we only support one queue, just dereference the pointer. +*/ +static struct macvtap_queue *macvtap_get_queue (struct net_device *dev, +struct sk_buff *skb) +{ +struct macvlan_dev *vlan =netdev_priv (dev); + +return rcu_dereference (vlan->tap); +} + +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)
drivers/net/macvtap.c line 170
Quoted 10 lines
drivers/net/macvtap.c new file +10
-0+{ +rcu_read_lock_bh (); +return rcu_dereference (file->private_data); +} + +static inline void macvtap_file_put_queue (void ) +{ +rcu_read_unlock_bh (); +} +
I find such wrappers around rcu obscure this,
already sufficiently complex, pattern.
Might be just me.
Quoted 78 lines
drivers/net/macvtap.c new file +78
-0+/* +* Forward happens for data that gets sent from one macvlan +* endpoint to another one in bridge mode. We just take +* the skb and put it into the receive queue. +*/ +static int macvtap_forward (struct net_device *dev,struct sk_buff *skb) +{ +struct macvtap_queue *q =macvtap_get_queue (dev, skb); +if (!q) +return -ENOLINK ; + +skb_queue_tail (&q->sk.sk_receive_queue, skb); +wake_up (q->sk.sk_sleep); +return 0 ; +} + +/* +* Receive is for data from the external interface (lowerdev), +* in case of macvtap, we can treat that the same way as +* forward, which macvlan cannot. +*/ +static int macvtap_receive (struct sk_buff *skb) +{ +skb_push (skb,ETH_HLEN ); +return macvtap_forward (skb->dev, skb); +} + +static int macvtap_newlink (struct net *src_net, +struct net_device *dev, +struct nlattr *tb[], +struct nlattr *data[]) +{ +struct device *classdev; +dev_t devt; +int err; + + err =macvlan_common_newlink (src_net, dev, tb, data, + macvtap_receive, macvtap_forward); +if (err) + goto out; + + devt =MKDEV (MAJOR (macvtap_major), dev->ifindex); + + classdev =device_create (macvtap_class, &dev->dev, devt, + dev,"tap%d" , dev->ifindex); +if (IS_ERR (classdev)) { + err =PTR_ERR (classdev); +macvtap_del_queues (dev); + } + +out: +return err; +} + +static void macvtap_dellink (struct net_device *dev, +struct list_head *head) +{ +device_destroy (macvtap_class, +MKDEV (MAJOR (macvtap_major), dev->ifindex)); + +macvtap_del_queues (dev); +macvlan_dellink (dev, head); +} + +static struct rtnl_link_ops macvtap_link_ops__read_mostly = { + .kind ="macvtap" , + .newlink = macvtap_newlink, + .dellink = macvtap_dellink, +}; + + +static void macvtap_sock_write_space (struct sock *sk) +{ +if (!sock_writeable (sk) || + !test_and_clear_bit (SOCK_ASYNC_NOSPACE , &sk->sk_socket->flags)) +return ; + +if (sk->sk_sleep &&waitqueue_active (sk->sk_sleep))
drivers/net/macvtap.c line 258
Quoted 10 lines
drivers/net/macvtap.c new file +10
-0+wake_up_interruptible_sync (sk->sk_sleep); +} + +static int macvtap_open (struct inode *inode,struct file *file) +{ +struct net *net = current->nsproxy->net_ns; +struct net_device *dev =dev_get_by_index (net,iminor (inode)); +struct macvtap_queue *q; +int err; +
This seems to keep reference to device as long as character device is open, which, if I understand correctly, will start printing error messages to kernel log about once a second if you try to remove the device.
I suspect the best way to fix this issue would be to use some kind of notifier so that macvtap can disconnect on device removal.
Quoted 85 lines
drivers/net/macvtap.c new file +85
-0+ err = -ENODEV ; +if (!dev) + goto out; + +/* check if this is a macvtap device */ + err = -EINVAL ; +if (dev->rtnl_link_ops != &macvtap_link_ops) + goto out; + + err = -ENOMEM ; + q = (struct macvtap_queue *)sk_alloc (net,AF_UNSPEC ,GFP_KERNEL , + &macvtap_proto); +if (!q) + goto out; + +init_waitqueue_head (&q->sock.wait); + 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); +if (err) +sock_put (&q->sk); + +out: +return err; +} + +static int macvtap_release (struct inode *inode,struct file *file) +{ +macvtap_del_queue ((struct macvtap_queue **)&file->private_data); +return 0 ; +} + +static unsigned int macvtap_poll (struct file *file,poll_table * wait) +{ +struct macvtap_queue *q =macvtap_file_get_queue (file); +unsigned int mask =POLLERR ; + +if (!q) + goto out; + + mask =0 ; +poll_wait (file, &q->sock.wait, wait); + +if (!skb_queue_empty (&q->sk.sk_receive_queue)) + mask |=POLLIN |POLLRDNORM ; + +if (sock_writeable (&q->sk) || + (!test_and_set_bit (SOCK_ASYNC_NOSPACE , &q->sock.flags) && +sock_writeable (&q->sk))) + mask |=POLLOUT |POLLWRNORM ; + +out: +macvtap_file_put_queue (); +return mask; +} + +/* Get packet from user space buffer */ +static ssize_t macvtap_get_user (struct macvtap_queue *q, +const struct iovec *iv,size_t count, +int noblock) +{ +struct sk_buff *skb; +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; + } + +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);
drivers/net/macvtap.c line 353
Quoted 10 lines
drivers/net/macvtap.c new file +10
-0+return -EFAULT ; + } + +skb_set_network_header (skb,ETH_HLEN ); + +macvlan_start_xmit (skb, q->vlan->dev); + +return count; +} +
I am surprised there's no GSO support. Would it be a good idea to share code with tun driver? That already has GSO ...
Also, network header pointer seems off for vlan packets?
Quoted 77 lines
drivers/net/macvtap.c new file +77
-0+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; + + result =macvtap_get_user (q, iv,iov_length (iv, count), + file->f_flags &O_NONBLOCK ); +out: +macvtap_file_put_queue (); +return result; +} + +/* Put packet to the user space buffer */ +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; +int ret; + + len =min_t (int, skb->len, len); + + ret =skb_copy_datagram_const_iovec (skb,0 , iv,0 , len); + +macvlan_count_rx (vlan, len, ret ==0 ,0 ); + +return ret ? ret : len; +} + +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); + +DECLARE_WAITQUEUE (wait, current); +struct sk_buff *skb; +ssize_t len, ret =0 ; + +if (!q) { + ret = -ENOLINK ; + goto out; + } + + len =iov_length (iv, count); +if (len <0 ) { + ret = -EINVAL ; + goto out; + } + +add_wait_queue (q->sk.sk_sleep, &wait); +while (len) { + current->state =TASK_INTERRUPTIBLE ; + +/* Read frames from the queue */ + skb =skb_dequeue (&q->sk.sk_receive_queue); +if (!skb) { +if (file->f_flags &O_NONBLOCK ) { + ret = -EAGAIN ; +break ; + } +if (signal_pending (current)) { + ret = -ERESTARTSYS ; +break ; + } +/* Nothing to read, let's sleep */ +schedule (); +continue ; + } + ret =macvtap_put_user (q, skb, iv, len); +kfree_skb (skb); +break ;
drivers/net/macvtap.c line 440
Quoted 10 lines
drivers/net/macvtap.c new file +10
-0+ } + + current->state =TASK_RUNNING ; +remove_wait_queue (q->sk.sk_sleep, &wait); + +out: +macvtap_file_put_queue (); +return ret; +} +
Same GSO comment here.
drivers/net/macvtap.c line 446
Quoted 6 lines
drivers/net/macvtap.c new file +6
-0+/* +* provide compatibility with generic tun/tap interface +*/ +static long macvtap_ioctl (struct file *file,unsigned int cmd, +unsigned long arg) +{
All of these seem to be stubs, and tun has many more that you didn't stub out. So, why do you bother to support any ioctls at all?
Quoted 140 lines
drivers/net/macvtap.c new file +126
-0+struct macvtap_queue *q; +void __user *argp = (void __user *)arg; +struct ifreq __user *ifr = argp; +unsigned int __user *up = argp; +unsigned int u; +int err; + +switch (cmd) { +case TUNSETIFF : +/* ignore the name, just look at flags */ +if (get_user (u, &ifr->ifr_flags)) +return -EFAULT ; +if (u != (IFF_TAP |IFF_NO_PI )) +return -EINVAL ; +return 0 ; + +case TUNGETIFF : + q =macvtap_file_get_queue (file); +if (!q) +return -ENOLINK ; + err =0 ; +if (copy_to_user (&ifr->ifr_name, q->vlan->dev->name,IFNAMSIZ ) || +put_user ((TUN_TAP_DEV |TUN_NO_PI ), &ifr->ifr_flags)) + err = -EFAULT ; +macvtap_file_put_queue (); +return err; + +case TUNGETFEATURES : +if (put_user ((IFF_TAP |IFF_NO_PI ), up)) +return -EFAULT ; +return 0 ; + +case TUNSETSNDBUF : +/* ignore */ +return 0 ; + +case TUNSETOFFLOAD : +/* let the user check for future flags */ +if (arg & ~(TUN_F_CSUM |TUN_F_TSO4 |TUN_F_TSO6 | +TUN_F_TSO_ECN |TUN_F_UFO )) +return -EINVAL ; + +/* TODO: add support for these, so far we don't +support any offload */ +if (arg & (TUN_F_CSUM |TUN_F_TSO4 |TUN_F_TSO6 | +TUN_F_TSO_ECN |TUN_F_UFO )) +return -EINVAL ; + +return 0 ; + +default : +return -EINVAL ; + } +} + +#ifdef CONFIG_COMPAT +static long macvtap_compat_ioctl (struct file *file,unsigned int cmd, +unsigned long arg) +{ +return macvtap_ioctl (file, cmd, (unsigned long )compat_ptr (arg)); +} +#endif + +static const struct file_operations macvtap_fops = { + .owner =THIS_MODULE , + .open = macvtap_open, + .release = macvtap_release, + .aio_read = macvtap_aio_read, + .aio_write = macvtap_aio_write, + .poll = macvtap_poll, + .llseek = no_llseek, + .unlocked_ioctl = macvtap_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = macvtap_compat_ioctl, +#endif +}; + +static int macvtap_init (void ) +{ +int err; + + err =alloc_chrdev_region (&macvtap_major,0 , +MACVTAP_NUM_DEVS ,"macvtap" ); +if (err) + goto out1; + +cdev_init (&macvtap_cdev, &macvtap_fops); + err =cdev_add (&macvtap_cdev, macvtap_major,MACVTAP_NUM_DEVS ); +if (err) + goto out2; + + macvtap_class =class_create (THIS_MODULE ,"macvtap" ); +if (IS_ERR (macvtap_class)) { + err =PTR_ERR (macvtap_class); + goto out3; + } + + err =macvlan_link_register (&macvtap_link_ops); +if (err) + goto out4; + +return 0 ; + +out4: +class_unregister (macvtap_class); +out3: +cdev_del (&macvtap_cdev); +out2: +unregister_chrdev_region (macvtap_major,MACVTAP_NUM_DEVS ); +out1: +return err; +} +module_init (macvtap_init); + +static void macvtap_exit (void ) +{ +rtnl_link_unregister (&macvtap_link_ops); +class_unregister (macvtap_class); +cdev_del (&macvtap_cdev); +unregister_chrdev_region (macvtap_major,MACVTAP_NUM_DEVS ); +} +module_exit (macvtap_exit); + +MODULE_ALIAS_RTNL_LINK ("macvtap" ); +MODULE_AUTHOR ("Arnd Bergmann <[email protected]>" ); +MODULE_LICENSE ("GPL" );include/linux/if_macvlan.h +1
-0@@ -34,6 +34,7 @@ struct macvlan_dev {enum macvlan_mode mode;int (*receive)(struct sk_buff *skb);int (*forward)(struct net_device *dev,struct sk_buff *skb); +struct macvtap_queue *tap; }; static inline void macvlan_count_rx(const struct macvlan_dev *vlan,Signature · 1 line
1.6.3.3
Re: [PATCH 3/3] net: macvtap driver
Quoted 16 lines · Michael S. Tsirkin
drivers/net/macvtap.c line 170
Quoted 11 lines · Arnd Bergmann
drivers/net/macvtap.c new file +11
-0+static inline struct macvtap_queue *macvtap_file_get_queue (struct file *file) +{ +rcu_read_lock_bh (); +return rcu_dereference (file->private_data); +} + +static inline void macvtap_file_put_queue (void ) +{ +rcu_read_unlock_bh (); +} +I find such wrappers around rcu obscure this,
already sufficiently complex, pattern.
Might be just me.
Obviously I find them useful here, but if more people feel the same as you, I'll just open-code them.
Quoted 13 lines
drivers/net/macvtap.c line 258
Quoted 7 lines
drivers/net/macvtap.c new file +7
-0+static int macvtap_open (struct inode *inode,struct file *file) +{ +struct net *net = current->nsproxy->net_ns; +struct net_device *dev =dev_get_by_index (net,iminor (inode)); +struct macvtap_queue *q; +int err; +This seems to keep reference to device as long as character device is open, which, if I understand correctly, will start printing error messages to kernel log about once a second if you try to remove the device.
Quoted 2 lines
I suspect the best way to fix this issue would be to use some kind of notifier so that macvtap can disconnect on device removal.
I think I'm just missing the put in the open function, the code already handles the netif and the file disappearing independently.
Thanks for spotting this one, I'll fix that in the next post.
Quoted 11 lines
Quoted 11 lines
drivers/net/macvtap.c new file +11
-0+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 ; + } + +skb_set_network_header (skb,ETH_HLEN ); +
Quoted 8 lines
drivers/net/macvtap.c line 353
Quoted 5 lines
drivers/net/macvtap.c new file +5
-0+macvlan_start_xmit (skb, q->vlan->dev); + +return count; +} +I am surprised there's no GSO support. Would it be a good idea to share code with tun driver? That already has GSO ...
The driver still only does the minimum feature set to get things going. GSO is an obvious extension, but I wanted the code to be as simple as possible to find all the basic bugs before we do anything fancy.
Quoted 1 line
Also, network header pointer seems off for vlan packets?
That may well be, I haven't tried vlan. What do you think it should do then?
Quoted 9 lines
drivers/net/macvtap.c line 446
Quoted 6 lines
drivers/net/macvtap.c new file +6
-0+/* +* provide compatibility with generic tun/tap interface +*/ +static long macvtap_ioctl (struct file *file,unsigned int cmd, +unsigned long arg) +{All of these seem to be stubs, and tun has many more that you didn't stub out. So, why do you bother to support any ioctls at all?
Again, minimum features to get things going. qemu fails to open the device if these ioctls are not implemented, but any of the more advanced features are left out.
Thansk for the review,
Arnd
Michael S. Tsirkin
maintainerRe: [PATCH 3/3] net: macvtap driver
Quoted 62 lines · Arnd Bergmann
Quoted 16 lines · Michael S. Tsirkin
drivers/net/macvtap.c line 170
Quoted 11 lines · Arnd Bergmann
drivers/net/macvtap.c new file +11
-0+static inline struct macvtap_queue *macvtap_file_get_queue (struct file *file) +{ +rcu_read_lock_bh (); +return rcu_dereference (file->private_data); +} + +static inline void macvtap_file_put_queue (void ) +{ +rcu_read_unlock_bh (); +} +I find such wrappers around rcu obscure this,
already sufficiently complex, pattern.
Might be just me.Obviously I find them useful here, but if more people feel the same as you, I'll just open-code them.
Quoted 13 lines
drivers/net/macvtap.c line 258
Quoted 7 lines
drivers/net/macvtap.c new file +7
-0+static int macvtap_open (struct inode *inode,struct file *file) +{ +struct net *net = current->nsproxy->net_ns; +struct net_device *dev =dev_get_by_index (net,iminor (inode)); +struct macvtap_queue *q; +int err; +This seems to keep reference to device as long as character device is open, which, if I understand correctly, will start printing error messages to kernel log about once a second if you try to remove the device.
Quoted 2 lines
I suspect the best way to fix this issue would be to use some kind of notifier so that macvtap can disconnect on device removal.
I think I'm just missing the put in the open function, the code already handles the netif and the file disappearing independently.
Thanks for spotting this one, I'll fix that in the next post.
Quoted 11 lines
Quoted 11 lines
drivers/net/macvtap.c new file +11
-0+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 ; + } + +skb_set_network_header (skb,ETH_HLEN ); +Quoted 8 lines
drivers/net/macvtap.c line 353
Quoted 5 lines
drivers/net/macvtap.c new file +5
-0+macvlan_start_xmit (skb, q->vlan->dev); + +return count; +} +I am surprised there's no GSO support. Would it be a good idea to share code with tun driver? That already has GSO ...
Quoted 8 lines
The driver still only does the minimum feature set to get things going. GSO is an obvious extension, but I wanted the code to be as simple as possible to find all the basic bugs before we do anything fancy.
Quoted 1 line
Also, network header pointer seems off for vlan packets?
That may well be, I haven't tried vlan. What do you think it should do then?
Look at eth_type for a more complete packet parsing.
Quoted 10 lines
Quoted 9 lines
drivers/net/macvtap.c line 446
Quoted 6 lines
drivers/net/macvtap.c new file +6
-0+/* +* provide compatibility with generic tun/tap interface +*/ +static long macvtap_ioctl (struct file *file,unsigned int cmd, +unsigned long arg) +{All of these seem to be stubs, and tun has many more that you didn't stub out. So, why do you bother to support any ioctls at all?
Quoted 3 lines
Again, minimum features to get things going. qemu fails to open the device if these ioctls are not implemented, but any of the more advanced features are left out.
This is strange, could be application bug. E.g. send buf size is relatively new and apps should handle failure gracefully. IMO, returning success and ignoring the value is not a good idea. How about we just fix qemu? What about other apps?
Quoted 3 lines
Thansk for the review,
Arnd
Re: [PATCH 3/3] net: macvtap driver
Quoted 4 lines · Michael S. Tsirkin
Quoted 2 lines
That may well be, I haven't tried vlan. What do you think it should do then?
Look at
eth_typefor a more complete packet parsing.
ok. I initially called that but it crashed because the skb was not initialized properly at that point. I'll have a look.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
drivers/net/macvtap.c line 446
Quoted 6 lines
drivers/net/macvtap.c new file +6
-0+/* +* provide compatibility with generic tun/tap interface +*/ +static long macvtap_ioctl (struct file *file,unsigned int cmd, +unsigned long arg) +{All of these seem to be stubs, and tun has many more that you didn't stub out. So, why do you bother to support any ioctls at all?
Quoted 8 lines
Quoted 3 lines
Again, minimum features to get things going. qemu fails to open the device if these ioctls are not implemented, but any of the more advanced features are left out.
This is strange, could be application bug. E.g. send buf size is relatively new and apps should handle failure gracefully. IMO, returning success and ignoring the value is not a good idea. How about we just fix qemu? What about other apps?
Ok, I'll go through the ioctls again and make sure they behave correctly they way you said. I haven't tried against against anything but qemu and cat.
Arnd
Quoted 4 lines · Arnd Bergmann
There are still known problems, but unless there are fundamental concerns, I'd like this to go into net-next as an experimental driver, fixing up the remaining problems by 2.6.34-rc1.
I should have been more specific here. The one really annoying problem is a reference counting problem I introduced in one of the last changes that prevents you from destroying a device after it has been used.
Unfortunately, I'm still traveling after LCA, and haven't had a chance to look into this before sending out the patches as I had originally planned. I've also seen crashes that are not fully reproducible, any bug reports on those are appreciated.
Arnd
v4 cover macvtap driver
In reply to Arnd BergmannThis is the fourth version of the macvtap driver,
based on the comments I got for the last version
I got a few days ago. Very few changes:
* release netdev in chardev open function so
we can destroy it properly.
* Implement TUNSETSNDBUF
* fix sleeping call in rcu_read_lock
* Fix comment in namespace isolation patch
* Fix small context difference to make it apply
to net-next
I can't really test here while travelling, so please give it a go if you're interested in this driver.
Arnd
PATCH v3 1/3 net: maintain namespace isolation between vlan and real device
In the vlan and macvlan drivers, the start_xmit function forwards data to the dev_queue_xmit function for another device, which may potentially belong to a different namespace.
To make sure that classification stays within a single namespace, this resets the potentially critical fields.
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/macvlan.c | 2 +- include/linux/netdevice.h | 9 +++++++++ net/8021q/vlan_dev.c | 2 +- net/core/dev.c | 35 +++++++++++++++++++++++++++++++---- 4 files changed, 42 insertions(+), 6 deletions(-)
drivers/net/macvlan.c +1 -1
@@ -269,7 +269,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev) } xmit_world: - skb->dev = vlan->lowerdev; +skb_set_dev (skb, vlan->lowerdev);return dev_queue_xmit (skb); }
include/linux/netdevice.h +9 -0
@@ -1004,6 +1004,15 @@ static inline bool netdev_uses_dsa_tags(struct net_device *dev)return 0 ; } +#ifndef CONFIG_NET_NS +static inline void skb_set_dev (struct sk_buff *skb,struct net_device *dev) +{ + skb->dev = dev; +} +#else /* CONFIG_NET_NS */ +void skb_set_dev (struct sk_buff *skb,struct net_device *dev); +#endif +static inline bool netdev_uses_trailer_tags (struct net_device *dev) {#ifdef CONFIG_NET_DSA_TAG_TRAILER
net/8021q/vlan_dev.c +1 -1
@@ -322,7 +322,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb, } - skb->dev =vlan_dev_info (dev)->real_dev; +skb_set_dev (skb,vlan_dev_info (dev)->real_dev); len = skb->len; ret =dev_queue_xmit (skb);
net/core/dev.c +31 -4
@@ -1448,13 +1448,10 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)if (skb->len > (dev->mtu + dev->hard_header_len))return NET_RX_DROP ; -skb_dst_drop (skb); +skb_set_dev (skb, dev); skb->tstamp.tv64 =0 ; skb->pkt_type =PACKET_HOST ; skb->protocol =eth_type_trans (skb, dev); - skb->mark =0 ; -secpath_reset (skb); -nf_reset (skb);return netif_rx (skb); }EXPORT_SYMBOL_GPL (dev_forward_skb);
@@ -1614,6 +1611,36 @@ static bool dev_can_checksum(struct net_device *dev, struct sk_buff *skb)return false; } +/** +* skb_dev_set -- assign a new device to a buffer +* @skb: buffer for the new device +* @dev: network device +* +* If an skb is owned by a device already, we have to reset +* all data private to the namespace a device belongs to +* before assigning it a new device. +*/ +#ifdef CONFIG_NET_NS +void skb_set_dev (struct sk_buff *skb,struct net_device *dev) +{ +skb_dst_drop (skb); +if (skb->dev && !net_eq (dev_net (skb->dev),dev_net (dev))) { +secpath_reset (skb); +nf_reset (skb); +skb_init_secmark (skb); + skb->mark =0 ; + skb->priority =0 ; + skb->nf_trace =0 ; + skb->ipvs_property =0 ; +#ifdef CONFIG_NET_SCHED + skb->tc_index =0 ; +#endif + } + skb->dev = dev; +} +EXPORT_SYMBOL (skb_set_dev); +#endif /* CONFIG_NET_NS */ +/* * Invalidate hardware checksum when packet is to be mangled, and * complete checksum manually on outgoing path.
Signature · 1 line
1.6.3.3
PATCH v3 1/3 net: maintain namespace isolation between vlan and real device
In reply to Arnd BergmannIn the vlan and macvlan drivers, the start_xmit function forwards data to the dev_queue_xmit function for another device, which may potentially belong to a different namespace.
To make sure that classification stays within a single namespace, this resets the potentially critical fields.
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/macvlan.c | 2 +- include/linux/netdevice.h | 9 +++++++++ net/8021q/vlan_dev.c | 2 +- net/core/dev.c | 35 +++++++++++++++++++++++++++++++---- 4 files changed, 42 insertions(+), 6 deletions(-)
drivers/net/macvlan.c +1 -1
@@ -269,7 +269,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev) } xmit_world: - skb->dev = vlan->lowerdev; +skb_set_dev (skb, vlan->lowerdev);return dev_queue_xmit (skb); }
include/linux/netdevice.h +9 -0
@@ -1004,6 +1004,15 @@ static inline bool netdev_uses_dsa_tags(struct net_device *dev)return 0 ; } +#ifndef CONFIG_NET_NS +static inline void skb_set_dev (struct sk_buff *skb,struct net_device *dev) +{ + skb->dev = dev; +} +#else /* CONFIG_NET_NS */ +void skb_set_dev (struct sk_buff *skb,struct net_device *dev); +#endif +static inline bool netdev_uses_trailer_tags (struct net_device *dev) {#ifdef CONFIG_NET_DSA_TAG_TRAILER
net/8021q/vlan_dev.c +1 -1
@@ -322,7 +322,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb, } - skb->dev =vlan_dev_info (dev)->real_dev; +skb_set_dev (skb,vlan_dev_info (dev)->real_dev); len = skb->len; ret =dev_queue_xmit (skb);
net/core/dev.c +31 -4
@@ -1448,13 +1448,10 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)if (skb->len > (dev->mtu + dev->hard_header_len))return NET_RX_DROP ; -skb_dst_drop (skb); +skb_set_dev (skb, dev); skb->tstamp.tv64 =0 ; skb->pkt_type =PACKET_HOST ; skb->protocol =eth_type_trans (skb, dev); - skb->mark =0 ; -secpath_reset (skb); -nf_reset (skb);return netif_rx (skb); }EXPORT_SYMBOL_GPL (dev_forward_skb);
@@ -1614,6 +1611,36 @@ static bool dev_can_checksum(struct net_device *dev, struct sk_buff *skb)return false; } +/** +* skb_dev_set -- assign a new device to a buffer +* @skb: buffer for the new device +* @dev: network device +* +* If an skb is owned by a device already, we have to reset +* all data private to the namespace a device belongs to +* before assigning it a new device. +*/ +#ifdef CONFIG_NET_NS +void skb_set_dev (struct sk_buff *skb,struct net_device *dev) +{ +skb_dst_drop (skb); +if (skb->dev && !net_eq (dev_net (skb->dev),dev_net (dev))) { +secpath_reset (skb); +nf_reset (skb); +skb_init_secmark (skb); + skb->mark =0 ; + skb->priority =0 ; + skb->nf_trace =0 ; + skb->ipvs_property =0 ; +#ifdef CONFIG_NET_SCHED + skb->tc_index =0 ; +#endif + } + skb->dev = dev; +} +EXPORT_SYMBOL (skb_set_dev); +#endif /* CONFIG_NET_NS */ +/* * Invalidate hardware checksum when packet is to be mangled, and * complete checksum manually on outgoing path.
Signature · 1 line
1.6.3.3
PATCH v3 2/3 macvlan: allow multiple driver backends
In reply to Arnd BergmannThis makes it possible to hook into the macvlan driver from another kernel module. In particular, the goal is to extend it with the macvtap backend that provides a tun/tap compatible interface directly on the macvlan device.
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/macvlan.c | 113 +++++++++++++++++++------------------------- include/linux/if_macvlan.h | 70 +++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 64 deletions(-)
drivers/net/macvlan.c +49 -64
@@ -39,31 +39,6 @@ struct macvlan_port {struct list_head vlans; }; -/** -* struct macvlan_rx_stats - MACVLAN percpu rx stats -* @rx_packets: number of received packets -* @rx_bytes: number of received bytes -* @multicast: number of received multicast packets -* @rx_errors: number of errors -*/ -struct macvlan_rx_stats { -unsigned long rx_packets; -unsigned long rx_bytes; -unsigned long multicast; -unsigned long rx_errors; -}; - -struct macvlan_dev { -struct net_device *dev; -struct list_head list; -struct hlist_node hlist; -struct macvlan_port *port; -struct net_device *lowerdev; -struct macvlan_rx_stats *rx_stats; -enum macvlan_mode mode; -}; - -static struct macvlan_dev *macvlan_hash_lookup (const struct macvlan_port *port,const unsigned char *addr) {
@@ -118,31 +93,17 @@ static int macvlan_addr_busy(const struct macvlan_port *port,return 0 ; } -static inline void macvlan_count_rx (const struct macvlan_dev *vlan, -unsigned int len,bool success, -bool multicast) -{ -struct macvlan_rx_stats *rx_stats; - - rx_stats =per_cpu_ptr (vlan->rx_stats,smp_processor_id ()); -if (likely (success)) { - rx_stats->rx_packets++;; - rx_stats->rx_bytes += len; -if (multicast) - rx_stats->multicast++; - }else { - rx_stats->rx_errors++; - } -} -static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev, +static int macvlan_broadcast_one(struct sk_buff *skb, + const struct macvlan_dev *vlan, const struct ethhdr *eth, bool local) { + struct net_device *dev = vlan->dev; if (!skb) return NET_RX_DROP; if (local) - return dev_forward_skb(dev, skb); + return vlan->forward(dev, skb); skb->dev = dev; if (!compare_ether_addr_64bits(eth->h_dest,
@@ -151,7 +112,7 @@ static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,else skb->pkt_type =PACKET_MULTICAST ; -return netif_rx (skb); +return vlan->receive (skb); } static void macvlan_broadcast(struct sk_buff *skb,
@@ -175,7 +136,7 @@ static void macvlan_broadcast(struct sk_buff *skb,continue ; nskb =skb_clone (skb,GFP_ATOMIC ); - err =macvlan_broadcast_one (nskb, vlan->dev, eth, + err =macvlan_broadcast_one (nskb, vlan, eth, mode ==MACVLAN_MODE_BRIDGE );macvlan_count_rx (vlan, skb->len +ETH_HLEN , err ==NET_RX_SUCCESS ,1 );
@@ -238,7 +199,7 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb) skb->dev = dev; skb->pkt_type =PACKET_HOST ; -netif_rx (skb); + vlan->receive (skb);return NULL ; }
@@ -260,7 +221,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev) dest =macvlan_hash_lookup (port, eth->h_dest);if (dest && dest->mode ==MACVLAN_MODE_BRIDGE ) {unsigned int length = skb->len +ETH_HLEN ; -int ret =dev_forward_skb (dest->dev, skb); +int ret = dest->forward (dest->dev, skb);macvlan_count_rx (dest, length, ret ==NET_RX_SUCCESS ,0 );
@@ -273,8 +234,8 @@ xmit_world:return dev_queue_xmit (skb); } -static netdev_tx_t macvlan_start_xmit (struct sk_buff *skb, -struct net_device *dev) +netdev_tx_t macvlan_start_xmit (struct sk_buff *skb, +struct net_device *dev) {int i =skb_get_queue_mapping (skb);struct netdev_queue *txq =netdev_get_tx_queue (dev, i);
@@ -290,6 +251,7 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,return ret; } +EXPORT_SYMBOL_GPL (macvlan_start_xmit); static int macvlan_hard_header(struct sk_buff *skb, struct net_device *dev, unsigned short type, const void *daddr,
@@ -623,8 +585,11 @@ static int macvlan_get_tx_queues(struct net *net,return 0 ; } -static int macvlan_newlink (struct net *src_net,struct net_device *dev, -struct nlattr *tb[],struct nlattr *data[]) +int macvlan_common_newlink (struct net *src_net,struct net_device *dev, +struct nlattr *tb[],struct nlattr *data[], +int (*receive)(struct sk_buff *skb), +int (*forward)(struct net_device *dev, +struct sk_buff *skb)) {struct macvlan_dev *vlan =netdev_priv (dev);struct macvlan_port *port;
@@ -664,6 +629,8 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev, vlan->lowerdev = lowerdev; vlan->dev = dev; vlan->port = port; + vlan->receive = receive; + vlan->forward = forward; vlan->mode =MACVLAN_MODE_VEPA ;if (data && data[IFLA_MACVLAN_MODE ])
@@ -677,8 +644,17 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,netif_stacked_transfer_operstate (lowerdev, dev);return 0 ; } +EXPORT_SYMBOL_GPL (macvlan_common_newlink); -static void macvlan_dellink (struct net_device *dev,struct list_head *head) +static int macvlan_newlink (struct net *src_net,struct net_device *dev, +struct nlattr *tb[],struct nlattr *data[]) +{ +return macvlan_common_newlink (src_net, dev, tb, data, + netif_rx, + dev_forward_skb); +} + +void macvlan_dellink (struct net_device *dev,struct list_head *head) {struct macvlan_dev *vlan =netdev_priv (dev);struct macvlan_port *port = vlan->port;
@@ -689,6 +665,7 @@ static void macvlan_dellink(struct net_device *dev, struct list_head *head)if (list_empty (&port->vlans))macvlan_port_destroy (port->dev); } +EXPORT_SYMBOL_GPL (macvlan_dellink);static int macvlan_changelink (struct net_device *dev,struct nlattr *tb[],struct nlattr *data[])
@@ -720,19 +697,27 @@ static const struct nla_policy macvlan_policy[IFLA_MACVLAN_MAX + 1] = { [IFLA_MACVLAN_MODE] = { .type = NLA_U32 }, }; -static struct rtnl_link_ops macvlan_link_ops__read_mostly = { +int macvlan_link_register (struct rtnl_link_ops *ops) +{ +/* common fields */ + ops->priv_size =sizeof (struct macvlan_dev ); + ops->get_tx_queues = macvlan_get_tx_queues; + ops->setup = macvlan_setup; + ops->validate = macvlan_validate; + ops->maxtype =IFLA_MACVLAN_MAX ; + ops->policy = macvlan_policy; + ops->changelink = macvlan_changelink; + ops->get_size = macvlan_get_size; + ops->fill_info = macvlan_fill_info; + +return rtnl_link_register (ops); +}; +EXPORT_SYMBOL_GPL (macvlan_link_register); + +static struct rtnl_link_ops macvlan_link_ops = { .kind ="macvlan" , - .priv_size =sizeof (struct macvlan_dev ), - .get_tx_queues = macvlan_get_tx_queues, - .setup = macvlan_setup, - .validate = macvlan_validate, .newlink = macvlan_newlink, .dellink = macvlan_dellink, - .maxtype =IFLA_MACVLAN_MAX , - .policy = macvlan_policy, - .changelink = macvlan_changelink, - .get_size = macvlan_get_size, - .fill_info = macvlan_fill_info, }; static int macvlan_device_event(struct notifier_block *unused,
@@ -761,7 +746,7 @@ static int macvlan_device_event(struct notifier_block *unused,break ;case NETDEV_UNREGISTER :list_for_each_entry_safe (vlan, next, &port->vlans, list) -macvlan_dellink (vlan->dev,NULL ); + vlan->dev->rtnl_link_ops->dellink (vlan->dev,NULL );break ; }return NOTIFY_DONE ;
@@ -778,7 +763,7 @@ static int __init macvlan_init_module(void)register_netdevice_notifier (&macvlan_notifier_block); macvlan_handle_frame_hook = macvlan_handle_frame; - err =rtnl_link_register (&macvlan_link_ops); + err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ;
include/linux/if_macvlan.h +70 -0
76 lines not on this page · raw message
Signature · 1 line
1.6.3.3
PATCH v3 2/3 macvlan: allow multiple driver backends
In reply to Arnd BergmannThis makes it possible to hook into the macvlan driver from another kernel module. In particular, the goal is to extend it with the macvtap backend that provides a tun/tap compatible interface directly on the macvlan device.
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/macvlan.c | 113 +++++++++++++++++++------------------------- include/linux/if_macvlan.h | 70 +++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 64 deletions(-)
drivers/net/macvlan.c +49 -64
@@ -39,31 +39,6 @@ struct macvlan_port {struct list_head vlans; }; -/** -* struct macvlan_rx_stats - MACVLAN percpu rx stats -* @rx_packets: number of received packets -* @rx_bytes: number of received bytes -* @multicast: number of received multicast packets -* @rx_errors: number of errors -*/ -struct macvlan_rx_stats { -unsigned long rx_packets; -unsigned long rx_bytes; -unsigned long multicast; -unsigned long rx_errors; -}; - -struct macvlan_dev { -struct net_device *dev; -struct list_head list; -struct hlist_node hlist; -struct macvlan_port *port; -struct net_device *lowerdev; -struct macvlan_rx_stats *rx_stats; -enum macvlan_mode mode; -}; - -static struct macvlan_dev *macvlan_hash_lookup (const struct macvlan_port *port,const unsigned char *addr) {
@@ -118,31 +93,17 @@ static int macvlan_addr_busy(const struct macvlan_port *port,return 0 ; } -static inline void macvlan_count_rx (const struct macvlan_dev *vlan, -unsigned int len,bool success, -bool multicast) -{ -struct macvlan_rx_stats *rx_stats; - - rx_stats =per_cpu_ptr (vlan->rx_stats,smp_processor_id ()); -if (likely (success)) { - rx_stats->rx_packets++;; - rx_stats->rx_bytes += len; -if (multicast) - rx_stats->multicast++; - }else { - rx_stats->rx_errors++; - } -} -static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev, +static int macvlan_broadcast_one(struct sk_buff *skb, + const struct macvlan_dev *vlan, const struct ethhdr *eth, bool local) { + struct net_device *dev = vlan->dev; if (!skb) return NET_RX_DROP; if (local) - return dev_forward_skb(dev, skb); + return vlan->forward(dev, skb); skb->dev = dev; if (!compare_ether_addr_64bits(eth->h_dest,
@@ -151,7 +112,7 @@ static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,else skb->pkt_type =PACKET_MULTICAST ; -return netif_rx (skb); +return vlan->receive (skb); } static void macvlan_broadcast(struct sk_buff *skb,
@@ -175,7 +136,7 @@ static void macvlan_broadcast(struct sk_buff *skb,continue ; nskb =skb_clone (skb,GFP_ATOMIC ); - err =macvlan_broadcast_one (nskb, vlan->dev, eth, + err =macvlan_broadcast_one (nskb, vlan, eth, mode ==MACVLAN_MODE_BRIDGE );macvlan_count_rx (vlan, skb->len +ETH_HLEN , err ==NET_RX_SUCCESS ,1 );
@@ -238,7 +199,7 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb) skb->dev = dev; skb->pkt_type =PACKET_HOST ; -netif_rx (skb); + vlan->receive (skb);return NULL ; }
@@ -260,7 +221,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev) dest =macvlan_hash_lookup (port, eth->h_dest);if (dest && dest->mode ==MACVLAN_MODE_BRIDGE ) {unsigned int length = skb->len +ETH_HLEN ; -int ret =dev_forward_skb (dest->dev, skb); +int ret = dest->forward (dest->dev, skb);macvlan_count_rx (dest, length, ret ==NET_RX_SUCCESS ,0 );
@@ -273,8 +234,8 @@ xmit_world:return dev_queue_xmit (skb); } -static netdev_tx_t macvlan_start_xmit (struct sk_buff *skb, -struct net_device *dev) +netdev_tx_t macvlan_start_xmit (struct sk_buff *skb, +struct net_device *dev) {int i =skb_get_queue_mapping (skb);struct netdev_queue *txq =netdev_get_tx_queue (dev, i);
@@ -290,6 +251,7 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,return ret; } +EXPORT_SYMBOL_GPL (macvlan_start_xmit); static int macvlan_hard_header(struct sk_buff *skb, struct net_device *dev, unsigned short type, const void *daddr,
@@ -623,8 +585,11 @@ static int macvlan_get_tx_queues(struct net *net,return 0 ; } -static int macvlan_newlink (struct net *src_net,struct net_device *dev, -struct nlattr *tb[],struct nlattr *data[]) +int macvlan_common_newlink (struct net *src_net,struct net_device *dev, +struct nlattr *tb[],struct nlattr *data[], +int (*receive)(struct sk_buff *skb), +int (*forward)(struct net_device *dev, +struct sk_buff *skb)) {struct macvlan_dev *vlan =netdev_priv (dev);struct macvlan_port *port;
@@ -664,6 +629,8 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev, vlan->lowerdev = lowerdev; vlan->dev = dev; vlan->port = port; + vlan->receive = receive; + vlan->forward = forward; vlan->mode =MACVLAN_MODE_VEPA ;if (data && data[IFLA_MACVLAN_MODE ])
@@ -677,8 +644,17 @@ static int macvlan_newlink(struct net *src_net, struct net_device *dev,netif_stacked_transfer_operstate (lowerdev, dev);return 0 ; } +EXPORT_SYMBOL_GPL (macvlan_common_newlink); -static void macvlan_dellink (struct net_device *dev,struct list_head *head) +static int macvlan_newlink (struct net *src_net,struct net_device *dev, +struct nlattr *tb[],struct nlattr *data[]) +{ +return macvlan_common_newlink (src_net, dev, tb, data, + netif_rx, + dev_forward_skb); +} + +void macvlan_dellink (struct net_device *dev,struct list_head *head) {struct macvlan_dev *vlan =netdev_priv (dev);struct macvlan_port *port = vlan->port;
@@ -689,6 +665,7 @@ static void macvlan_dellink(struct net_device *dev, struct list_head *head)if (list_empty (&port->vlans))macvlan_port_destroy (port->dev); } +EXPORT_SYMBOL_GPL (macvlan_dellink);static int macvlan_changelink (struct net_device *dev,struct nlattr *tb[],struct nlattr *data[])
@@ -720,19 +697,27 @@ static const struct nla_policy macvlan_policy[IFLA_MACVLAN_MAX + 1] = { [IFLA_MACVLAN_MODE] = { .type = NLA_U32 }, }; -static struct rtnl_link_ops macvlan_link_ops__read_mostly = { +int macvlan_link_register (struct rtnl_link_ops *ops) +{ +/* common fields */ + ops->priv_size =sizeof (struct macvlan_dev ); + ops->get_tx_queues = macvlan_get_tx_queues; + ops->setup = macvlan_setup; + ops->validate = macvlan_validate; + ops->maxtype =IFLA_MACVLAN_MAX ; + ops->policy = macvlan_policy; + ops->changelink = macvlan_changelink; + ops->get_size = macvlan_get_size; + ops->fill_info = macvlan_fill_info; + +return rtnl_link_register (ops); +}; +EXPORT_SYMBOL_GPL (macvlan_link_register); + +static struct rtnl_link_ops macvlan_link_ops = { .kind ="macvlan" , - .priv_size =sizeof (struct macvlan_dev ), - .get_tx_queues = macvlan_get_tx_queues, - .setup = macvlan_setup, - .validate = macvlan_validate, .newlink = macvlan_newlink, .dellink = macvlan_dellink, - .maxtype =IFLA_MACVLAN_MAX , - .policy = macvlan_policy, - .changelink = macvlan_changelink, - .get_size = macvlan_get_size, - .fill_info = macvlan_fill_info, }; static int macvlan_device_event(struct notifier_block *unused,
@@ -761,7 +746,7 @@ static int macvlan_device_event(struct notifier_block *unused,break ;case NETDEV_UNREGISTER :list_for_each_entry_safe (vlan, next, &port->vlans, list) -macvlan_dellink (vlan->dev,NULL ); + vlan->dev->rtnl_link_ops->dellink (vlan->dev,NULL );break ; }return NOTIFY_DONE ;
@@ -778,7 +763,7 @@ static int __init macvlan_init_module(void)register_netdevice_notifier (&macvlan_notifier_block); macvlan_handle_frame_hook = macvlan_handle_frame; - err =rtnl_link_register (&macvlan_link_ops); + err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ;
include/linux/if_macvlan.h +70 -0
76 lines not on this page · raw message
Signature · 1 line
1.6.3.3
PATCH v3 3/3 net: macvtap driver
In reply to Arnd BergmannIn order to use macvlan with qemu and other tools that require a tap file descriptor, the macvtap driver adds a small backend with a character device with the same interface as the tun driver, with a minimum set of features.
Macvtap interfaces are created in the same way as macvlan interfaces using ip link, but the netif is just used as a handle for configuration and accounting, while the data goes through the chardev. Each macvtap interface has its own character device, simplifying permission management significantly over the generic tun/tap driver.
Cc Patrick McHardy <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc David S. Miller" <[email protected]>
Cc "Michael S. Tsirkin" <[email protected]>
Cc Herbert Xu <[email protected]>
Cc Or Gerlitz <[email protected]>
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/Kconfig | 12 + drivers/net/Makefile | 1 + drivers/net/macvtap.c | 581 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/if_macvlan.h | 1 + 4 files changed, 595 insertions(+), 0 deletions(-) create mode 100644 drivers/net/macvtap.c
drivers/net/Kconfig +12 -0
@@ -90,6 +90,18 @@ config MACVLAN To compile this driver as a module, choose M here: the module will be called macvlan. +config MACVTAP + tristate "MAC-VLAN based tap driver (EXPERIMENTAL)" + depends on MACVLAN + help + This adds a specialized tap character device driver that is based + on the MAC-VLAN network interface, called macvtap. A macvtap device + can be added in the same way as a macvlan device, using 'type + macvlan', and then be accessed through the tap user space interface. + + To compile this driver as a module, choose M here: the module + will be called macvtap. + config EQUALIZER tristate "EQL (serial line load balancing) support" ---help---
drivers/net/Makefile +1 -0
@@ -169,6 +169,7 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND) += xen-netfront.o obj-$(CONFIG_DUMMY) += dummy.o obj-$(CONFIG_IFB) += ifb.o obj-$(CONFIG_MACVLAN) += macvlan.o +obj-$(CONFIG_MACVTAP) += macvtap.o obj-$(CONFIG_DE600) += de600.o obj-$(CONFIG_DE620) += de620.o obj-$(CONFIG_LANCE) += lance.o
drivers/net/macvtap.c new file +581 -0
581 lines not on this page · raw message
include/linux/if_macvlan.h +1 -0
@@ -34,6 +34,7 @@ struct macvlan_dev {enum macvlan_mode mode;int (*receive)(struct sk_buff *skb);int (*forward)(struct net_device *dev,struct sk_buff *skb); +struct macvtap_queue *tap; }; static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
Signature · 1 line
1.6.3.3
PATCH v3 3/3 net: macvtap driver
In reply to Arnd BergmannIn order to use macvlan with qemu and other tools that require a tap file descriptor, the macvtap driver adds a small backend with a character device with the same interface as the tun driver, with a minimum set of features.
Macvtap interfaces are created in the same way as macvlan interfaces using ip link, but the netif is just used as a handle for configuration and accounting, while the data goes through the chardev. Each macvtap interface has its own character device, simplifying permission management significantly over the generic tun/tap driver.
Cc Patrick McHardy <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc David S. Miller" <[email protected]>
Cc "Michael S. Tsirkin" <[email protected]>
Cc Herbert Xu <[email protected]>
Cc Or Gerlitz <[email protected]>
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/Kconfig | 12 + drivers/net/Makefile | 1 + drivers/net/macvtap.c | 581 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/if_macvlan.h | 1 + 4 files changed, 595 insertions(+), 0 deletions(-) create mode 100644 drivers/net/macvtap.c
drivers/net/Kconfig +12 -0
@@ -90,6 +90,18 @@ config MACVLAN To compile this driver as a module, choose M here: the module will be called macvlan. +config MACVTAP + tristate "MAC-VLAN based tap driver (EXPERIMENTAL)" + depends on MACVLAN + help + This adds a specialized tap character device driver that is based + on the MAC-VLAN network interface, called macvtap. A macvtap device + can be added in the same way as a macvlan device, using 'type + macvlan', and then be accessed through the tap user space interface. + + To compile this driver as a module, choose M here: the module + will be called macvtap. + config EQUALIZER tristate "EQL (serial line load balancing) support" ---help---
drivers/net/Makefile +1 -0
@@ -169,6 +169,7 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND) += xen-netfront.o obj-$(CONFIG_DUMMY) += dummy.o obj-$(CONFIG_IFB) += ifb.o obj-$(CONFIG_MACVLAN) += macvlan.o +obj-$(CONFIG_MACVTAP) += macvtap.o obj-$(CONFIG_DE600) += de600.o obj-$(CONFIG_DE620) += de620.o obj-$(CONFIG_LANCE) += lance.o
drivers/net/macvtap.c new file +581 -0
581 lines not on this page · raw message
include/linux/if_macvlan.h +1 -0
@@ -34,6 +34,7 @@ struct macvlan_dev {enum macvlan_mode mode;int (*receive)(struct sk_buff *skb);int (*forward)(struct net_device *dev,struct sk_buff *skb); +struct macvtap_queue *tap; }; static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
Signature · 1 line
1.6.3.3
David Miller
maintainerFrom: Arnd Bergmann <[email protected]>
Date: Sat, 30 Jan 2010 23:22:15 +0100
Quoted 12 lines
This is the fourth version of the macvtap driver,
based on the comments I got for the last version
I got a few days ago. Very few changes:* release netdev in chardev open function so
we can destroy it properly.
* Implement TUNSETSNDBUF
* fix sleeping call inrcu_read_lock
* Fix comment in namespace isolation patch
* Fix small context difference to make it apply
to net-next
Quoted 2 lines
I can't really test here while travelling, so please give it a go if you're interested in this driver.
All applied to net-next-2.6, thanks!
Quoted 17 lines
From: Arnd Bergmann <[email protected]>
Date: Sat, 30 Jan 2010 23:22:15 +0100Quoted 14 lines
This is the fourth version of the macvtap driver,
based on the comments I got for the last version
I got a few days ago. Very few changes:* release netdev in chardev open function so
we can destroy it properly.
* Implement TUNSETSNDBUF
* fix sleeping call inrcu_read_lock
* Fix comment in namespace isolation patch
* Fix small context difference to make it apply
to net-nextI can't really test here while travelling, so please give it a go if you're interested in this driver.
I'm seeing complaints from might_sleep():
Feb 8 16:21:06 ti102 kernel: BUG: sleeping function called from invalid context at include/linux/kernel.h:155 Feb 8 16:21:06 ti102 kernel: in_atomic(): 1, irqs_disabled(): 0, pid: 2881, name: qemu-kvm Feb 8 16:21:06 ti102 kernel: Pid: 2881, comm: qemu-kvm Not tainted 2.6.29.6.Ar-224527.2009eswierk8 #1 Feb 8 16:21:06 ti102 kernel: Call Trace: Feb 8 16:21:06 ti102 kernel: [<c0119250>] __might_sleep+0xdc/0xe3 Feb 8 16:21:06 ti102 kernel: [<c0210f7c>] copy_to_user+0x36/0x106 Feb 8 16:21:06 ti102 kernel: [<c02af568>] memcpy_toiovec+0x2c/0x50 Feb 8 16:21:06 ti102 kernel: [<c02afbb3>] skb_copy_datagram_iovec+0x47/0x184 Feb 8 16:21:06 ti102 kernel: [<c034bd07>] ? _spin_unlock_irqrestore+0x17/0x2c Feb 8 16:21:06 ti102 kernel: [<f829a776>] macvtap_aio_read+0x102/0x158 [macvtap] Feb 8 16:21:06 ti102 kernel: [<c011eaf7>] ? default_wake_function+0x0/0xd Feb 8 16:21:06 ti102 kernel: [<c016c75f>] do_sync_read+0xab/0xe9 Feb 8 16:21:06 ti102 kernel: [<c0133933>] ? autoremove_wake_function+0x0/0x33 Feb 8 16:21:06 ti102 kernel: [<c019211f>] ? eventfd_read+0x121/0x156 Feb 8 16:21:06 ti102 kernel: [<c011eaf7>] ? default_wake_function+0x0/0xd Feb 8 16:21:06 ti102 kernel: [<c016d101>] vfs_read+0xb5/0x129 Feb 8 16:21:06 ti102 kernel: [<c016d20e>] sys_read+0x3b/0x60 Feb 8 16:21:06 ti102 kernel: [<c0102e71>] sysenter_do_call+0x12/0x25
Feb 8 16:21:08 ti102 kernel: BUG: sleeping function called from invalid context at include/linux/kernel.h:155 Feb 8 16:21:08 ti102 kernel: in_atomic(): 1, irqs_disabled(): 0, pid: 2882, name: qemu-kvm Feb 8 16:21:08 ti102 kernel: Pid: 2882, comm: qemu-kvm Not tainted 2.6.29.6.Ar-224527.2009eswierk8 #1 Feb 8 16:21:08 ti102 kernel: Call Trace: Feb 8 16:21:08 ti102 kernel: [<c0119250>] __might_sleep+0xdc/0xe3 Feb 8 16:21:08 ti102 kernel: [<c0210e5f>] copy_from_user+0x34/0x11b Feb 8 16:21:08 ti102 kernel: [<c02af388>] memcpy_fromiovec+0x2c/0x50 Feb 8 16:21:08 ti102 kernel: [<c02afa30>] skb_copy_datagram_from_iovec+0x47/0x183 Feb 8 16:21:08 ti102 kernel: [<f829a618>] macvtap_aio_write+0xaa/0x106 [macvtap] Feb 8 16:21:08 ti102 kernel: [<c034a553>] ? mutex_unlock+0x8/0xa Feb 8 16:21:08 ti102 kernel: [<c016c58d>] do_sync_readv_writev+0xa1/0xdf Feb 8 16:21:08 ti102 kernel: [<c0133933>] ? autoremove_wake_function+0x0/0x33 Feb 8 16:21:08 ti102 kernel: [<c016c44f>] ? rw_copy_check_uvector+0x5b/0xc3 Feb 8 16:21:08 ti102 kernel: [<c016cbec>] do_readv_writev+0x82/0x165 Feb 8 16:21:08 ti102 kernel: [<f829a56e>] ? macvtap_aio_write+0x0/0x106 [macvtap] Feb 8 16:21:08 ti102 kernel: [<c0177461>] ? do_vfs_ioctl+0x4a3/0x4dc Feb 8 16:21:08 ti102 kernel: [<c0112d83>] ? read_hpet+0xf/0x13 Feb 8 16:21:08 ti102 kernel: [<c016cd1e>] vfs_writev+0x4f/0x6b Feb 8 16:21:08 ti102 kernel: [<c016cd75>] sys_writev+0x3b/0x60 Feb 8 16:21:08 ti102 kernel: [<c0102e71>] sysenter_do_call+0x12/0x25
I backported your patch to kernel 2.6.29.6, an i386 kernel with CONFIG_PREEMPT=y and CONFIG_CLASSIC_RCU=y. It's entirely possible that I screwed something up in the backport; I can post my modified patch if it would help.
--Ed
Quoted 34 lines · Ed Swierk
Quoted 17 lines
From: Arnd Bergmann <[email protected]>
Date: Sat, 30 Jan 2010 23:22:15 +0100Quoted 14 lines
This is the fourth version of the macvtap driver,
based on the comments I got for the last version
I got a few days ago. Very few changes:* release netdev in chardev open function so
we can destroy it properly.
* Implement TUNSETSNDBUF
* fix sleeping call inrcu_read_lock
* Fix comment in namespace isolation patch
* Fix small context difference to make it apply
to net-nextI can't really test here while travelling, so please give it a go if you're interested in this driver.
I'm seeing complaints from
might_sleep():Feb 8 16:21:06 ti102 kernel: BUG: sleeping function called from invalid context at include/linux/kernel.h:155 Feb 8 16:21:06 ti102 kernel: in_atomic(): 1, irqs_disabled(): 0, pid: 2881, name: qemu-kvm Feb 8 16:21:06 ti102 kernel: Pid: 2881, comm: qemu-kvm Not tainted 2.6.29.6.Ar-224527.2009eswierk8 #1 Feb 8 16:21:06 ti102 kernel: Call Trace: Feb 8 16:21:06 ti102 kernel: [<c0119250>] __might_sleep+0xdc/0xe3 Feb 8 16:21:06 ti102 kernel: [<c0210f7c>] copy_to_user+0x36/0x106 Feb 8 16:21:06 ti102 kernel: [<c02af568>] memcpy_toiovec+0x2c/0x50 Feb 8 16:21:06 ti102 kernel: [<c02afbb3>] skb_copy_datagram_iovec+0x47/0x184 Feb 8 16:21:06 ti102 kernel: [<c034bd07>] ? _spin_unlock_irqrestore+0x17/0x2c Feb 8 16:21:06 ti102 kernel: [<f829a776>] macvtap_aio_read+0x102/0x158 [macvtap]
Quoted 8 lines
Feb 8 16:21:06 ti102 kernel: [<c011eaf7>] ? default_wake_function+0x0/0xd Feb 8 16:21:06 ti102 kernel: [<c016c75f>] do_sync_read+0xab/0xe9 Feb 8 16:21:06 ti102 kernel: [<c0133933>] ? autoremove_wake_function+0x0/0x33 Feb 8 16:21:06 ti102 kernel: [<c019211f>] ? eventfd_read+0x121/0x156 Feb 8 16:21:06 ti102 kernel: [<c011eaf7>] ? default_wake_function+0x0/0xd Feb 8 16:21:06 ti102 kernel: [<c016d101>] vfs_read+0xb5/0x129 Feb 8 16:21:06 ti102 kernel: [<c016d20e>] sys_read+0x3b/0x60 Feb 8 16:21:06 ti102 kernel: [<c0102e71>] sysenter_do_call+0x12/0x25
I am also seeing this issue with net-next-2.6.
Basically macvtap_put_user() and macvtap_get_user() call copy_to/from_user from within a RCU read-side critical section.
The following patch fixes this issue by releasing the RCU read lock before calling these routines, but instead hold a reference to q->sk.
Signed-off-by Sridhar Samudrala <[email protected]>
drivers/net/macvtap.c +29 -18
@@ -320,7 +320,7 @@ out: }/* Get packet from user space buffer */ -static ssize_t macvtap_get_user (struct macvtap_queue *q, +static ssize_t macvtap_get_user (struct macvlan_dev *vlan,struct sock *sk,const struct iovec *iv,size_t count,int noblock) {
@@ -331,10 +331,10 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,if (unlikely (len <ETH_HLEN ))return -EINVAL ; - skb =sock_alloc_send_skb (&q->sk,NET_IP_ALIGN + len, noblock, &err); + skb =sock_alloc_send_skb (sk,NET_IP_ALIGN + len, noblock, &err);if (!skb) { -macvlan_count_rx (q->vlan,0 , false, false); +macvlan_count_rx (vlan,0 , false, false);return err; }
@@ -342,14 +342,14 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,skb_put (skb, count);if (skb_copy_datagram_from_iovec (skb,0 , iv,0 , len)) { -macvlan_count_rx (q->vlan,0 , false, false); +macvlan_count_rx (vlan,0 , false, false);kfree_skb (skb);return -EFAULT ; }skb_set_network_header (skb,ETH_HLEN ); -macvlan_start_xmit (skb, q->vlan->dev); +macvlan_start_xmit (skb, vlan->dev);return count; }
@@ -360,23 +360,29 @@ static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv,struct file *file = iocb->ki_filp;ssize_t result = -ENOLINK ;struct macvtap_queue *q =macvtap_file_get_queue (file); +struct macvlan_dev *vlan; +struct sock *sk;if (!q) goto out; - result =macvtap_get_user (q, iv,iov_length (iv, count), + vlan = q->vlan; + sk = &q->sk; +sock_hold (sk); +macvtap_file_put_queue (); + + result =macvtap_get_user (vlan, sk, iv,iov_length (iv, count), file->f_flags &O_NONBLOCK ); +sock_put (sk); out: -macvtap_file_put_queue ();return result; }/* Put packet to the user space buffer */ -static ssize_t macvtap_put_user (struct macvtap_queue *q, +static ssize_t macvtap_put_user (struct macvlan_dev *vlan,const struct sk_buff *skb,const struct iovec *iv,int len) { -struct macvlan_dev *vlan = q->vlan;int ret; len =min_t (int, skb->len, len);
@@ -393,15 +399,20 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv, {struct file *file = iocb->ki_filp;struct macvtap_queue *q =macvtap_file_get_queue (file); +struct macvlan_dev *vlan; +struct sock *sk;DECLARE_WAITQUEUE (wait, current);struct sk_buff *skb;ssize_t len, ret =0 ; -if (!q) { - ret = -ENOLINK ; - goto out; - } +if (!q) +return -ENOLINK ; + + vlan = q->vlan; + sk = &q->sk; +sock_hold (sk); +macvtap_file_put_queue (); len =iov_length (iv, count);if (len <0 ) {
@@ -409,12 +420,12 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv, goto out; } -add_wait_queue (q->sk.sk_sleep, &wait); +add_wait_queue (sk->sk_sleep, &wait);while (len) { current->state =TASK_INTERRUPTIBLE ;/* Read frames from the queue */ - skb =skb_dequeue (&q->sk.sk_receive_queue); + skb =skb_dequeue (&sk->sk_receive_queue);if (!skb) {if (file->f_flags &O_NONBLOCK ) { ret = -EAGAIN ;
@@ -428,16 +439,16 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,schedule ();continue ; } - ret =macvtap_put_user (q, skb, iv, len); + ret =macvtap_put_user (vlan, skb, iv, len);kfree_skb (skb);break ; } current->state =TASK_RUNNING ; -remove_wait_queue (q->sk.sk_sleep, &wait); +remove_wait_queue (sk->sk_sleep, &wait); out: -macvtap_file_put_queue (); +sock_put (sk);return ret; }
Quoted 6 lines · Sridhar Samudrala
I am also seeing this issue with net-next-2.6.
Basicallymacvtap_put_user()andmacvtap_get_user()call copy_to/from_user from within a RCU read-side critical section.The following patch fixes this issue by releasing the RCU read lock before calling these routines, but instead hold a reference to
q->sk.
Thanks, I tried your patch and it fixes the problem.
However, it seems to cause another minor problem. macvlan_count_rx() is now getting called from macvtap_put_user() with preemption enabled, which causes smp_processor_id() to BUG:
Feb 8 20:31:38 ti102 kernel: BUG: using smp_processor_id() in preemptible [00000000] code: qemu-kvm/4546 Feb 8 20:31:38 ti102 kernel: caller is macvtap_aio_read+0x18c/0x221 [macvtap] Feb 8 20:31:38 ti102 kernel: Pid: 4546, comm: qemu-kvm Not tainted 2.6.29.6.Ar-224686.2009eswierk8.2 #1 Feb 8 20:31:38 ti102 kernel: Call Trace: Feb 8 20:31:38 ti102 kernel: [<c0349546>] ? printk+0xf/0x11 Feb 8 20:31:38 ti102 kernel: [<c02142c0>] debug_smp_processor_id +0xa4/0xb8 Feb 8 20:31:38 ti102 kernel: [<f8af581f>] macvtap_aio_read+0x18c/0x221 [macvtap] Feb 8 20:31:38 ti102 kernel: [<c011eaf7>] ? default_wake_function +0x0/0xd Feb 8 20:31:38 ti102 kernel: [<c016c75f>] do_sync_read+0xab/0xe9 Feb 8 20:31:38 ti102 kernel: [<c011933d>] ? update_curr+0x6c/0x147 Feb 8 20:31:38 ti102 kernel: [<c0133933>] ? autoremove_wake_function +0x0/0x33 Feb 8 20:31:38 ti102 kernel: [<c0349fd0>] ? schedule+0x7af/0x7e3 Feb 8 20:31:38 ti102 kernel: [<c016d101>] vfs_read+0xb5/0x129 Feb 8 20:31:38 ti102 kernel: [<c016d20e>] sys_read+0x3b/0x60 Feb 8 20:31:38 ti102 kernel: [<c0102e71>] sysenter_do_call+0x12/0x25
I fixed this problem with the change below. I'm not sure if replacing smp_processor_id() with get_cpu() is the right thing to do but it works for macvtap at least.
Signed-off-by Ed Swierk <[email protected]>
---
include/linux/if_macvlan.h +3 -1
@@ -42,8 +42,9 @@ static inline void macvlan_count_rx(cons bool multicast) {struct macvlan_rx_stats *rx_stats; +int cpu =get_cpu (); - rx_stats =per_cpu_ptr (vlan->rx_stats,smp_processor_id ()); + rx_stats =per_cpu_ptr (vlan->rx_stats, cpu);if (likely (success)) { rx_stats->rx_packets++;; rx_stats->rx_bytes += len;
@@ -52,6 +53,7 @@ static inline void macvlan_count_rx(cons }else { rx_stats->rx_errors++; } +put_cpu (); } extern int macvlan_common_newlink(struct net_device *dev,
Quoted 3 lines · Ed Swierk
I fixed this problem with the change below. I'm not sure if replacing
smp_processor_id()withget_cpu()is the right thing to do but it works for macvtap at least.
I think we also need to ensure the device doesn't go away, which was one of the reasons for the rcu_read_lock_bh() earlier.
Arnd
Quoted 2 lines · Arnd Bergmann
I think we also need to ensure the device doesn't go away, which was one of the reasons for the
rcu_read_lock_bh()earlier.
This may be veering far off into the weeds, but I'm wondering if you considered making macvtap devices behave more like tap devices. Specifically, the application would open /dev/net/macvtap and send it an ioctl with the name of the macvtap interface, the name of the lower interface to attach to, the MAC address, etc; this would cause the macvtap interface to spring into existence. The macvtap interface would go away when the application exits or closes the file.
The tricky part here would be noticing when the lower interface goes away, and (ideally) reattaching when an interface with the same name reappears.
I think the advantage of this approach is that it better fits the way applications like qemu and libvirt use tap interfaces. Unlike the current approach, however, this wouldn't allow creating a macvtap interface and keep it around independently of the application using it. Is it desirable to support this use case?
--Ed
Quoted 11 lines · On Thursday 11 February 2010 01:42:04 Ed Swierk wrote:
Quoted 2 lines · Arnd Bergmann
I think we also need to ensure the device doesn't go away, which was one of the reasons for the
rcu_read_lock_bh()earlier.This may be veering far off into the weeds, but I'm wondering if you considered making macvtap devices behave more like tap devices. Specifically, the application would open
/dev/net/macvtapand send it an ioctl with the name of the macvtap interface, the name of the lower interface to attach to, the MAC address, etc; this would cause the macvtap interface to spring into existence. The macvtap interface would go away when the application exits or closes the file.
No, I never considered this. In fact, this behavior of tun/tap is what makes that driver have really complex lifetime rules (more so than macvtap) and causes all sorts of problems if you want to manage unprivileged users accessing different outgoing interfaces.
Quoted 3 lines
The tricky part here would be noticing when the lower interface goes away, and (ideally) reattaching when an interface with the same name reappears.
The first part is not so hard, the second part I'd rather not do.
Quoted 5 lines
I think the advantage of this approach is that it better fits the way applications like qemu and libvirt use tap interfaces. Unlike the current approach, however, this wouldn't allow creating a macvtap interface and keep it around independently of the application using it. Is it desirable to support this use case?
I think it's very useful that you can set up static interfaces and give them to a user (or group) that are then able to use these interfaces without getting any network privileges beyond that.
Another reason for having one chardev per interface is to support multiple open files for the same interface. I want to use that as an easy way to support multi-queue NICs.
Arnd
Quoted 6 lines · Sridhar Samudrala
I am also seeing this issue with net-next-2.6.
Basicallymacvtap_put_user()andmacvtap_get_user()call copy_to/from_user from within a RCU read-side critical section.The following patch fixes this issue by releasing the RCU read lock before calling these routines, but instead hold a reference to
q->sk.
I've encountered some more problems, with various users of macvtap_file_get_queue() either calling or neglecting to call macvtap_file_put_queue() in error cases.
I modified your patch so that when macvtap_file_get_queue() returns 0, it also calls rcu_read_unlock_bh(), and modified the users appropriately.
This patch also incorporates my preemption fix for macvlan_count_rx().
Signed-off-by Ed Swierk <[email protected]>
---
Quoted 34 lines · Ed Swierk
Quoted 17 lines
From: Arnd Bergmann <[email protected]>
Date: Sat, 30 Jan 2010 23:22:15 +0100Quoted 14 lines
This is the fourth version of the macvtap driver,
based on the comments I got for the last version
I got a few days ago. Very few changes:* release netdev in chardev open function so
we can destroy it properly.
* Implement TUNSETSNDBUF
* fix sleeping call inrcu_read_lock
* Fix comment in namespace isolation patch
* Fix small context difference to make it apply
to net-nextI can't really test here while travelling, so please give it a go if you're interested in this driver.
I'm seeing complaints from
might_sleep():Feb 8 16:21:06 ti102 kernel: BUG: sleeping function called from invalid context at include/linux/kernel.h:155 Feb 8 16:21:06 ti102 kernel: in_atomic(): 1, irqs_disabled(): 0, pid: 2881, name: qemu-kvm Feb 8 16:21:06 ti102 kernel: Pid: 2881, comm: qemu-kvm Not tainted 2.6.29.6.Ar-224527.2009eswierk8 #1 Feb 8 16:21:06 ti102 kernel: Call Trace: Feb 8 16:21:06 ti102 kernel: [<c0119250>] __might_sleep+0xdc/0xe3 Feb 8 16:21:06 ti102 kernel: [<c0210f7c>] copy_to_user+0x36/0x106 Feb 8 16:21:06 ti102 kernel: [<c02af568>] memcpy_toiovec+0x2c/0x50 Feb 8 16:21:06 ti102 kernel: [<c02afbb3>] skb_copy_datagram_iovec+0x47/0x184 Feb 8 16:21:06 ti102 kernel: [<c034bd07>] ? _spin_unlock_irqrestore+0x17/0x2c Feb 8 16:21:06 ti102 kernel: [<f829a776>] macvtap_aio_read+0x102/0x158 [macvtap]
Quoted 8 lines
Feb 8 16:21:06 ti102 kernel: [<c011eaf7>] ? default_wake_function+0x0/0xd Feb 8 16:21:06 ti102 kernel: [<c016c75f>] do_sync_read+0xab/0xe9 Feb 8 16:21:06 ti102 kernel: [<c0133933>] ? autoremove_wake_function+0x0/0x33 Feb 8 16:21:06 ti102 kernel: [<c019211f>] ? eventfd_read+0x121/0x156 Feb 8 16:21:06 ti102 kernel: [<c011eaf7>] ? default_wake_function+0x0/0xd Feb 8 16:21:06 ti102 kernel: [<c016d101>] vfs_read+0xb5/0x129 Feb 8 16:21:06 ti102 kernel: [<c016d20e>] sys_read+0x3b/0x60 Feb 8 16:21:06 ti102 kernel: [<c0102e71>] sysenter_do_call+0x12/0x25
I am also seeing this issue with net-next-2.6.
Basically macvtap_put_user() and macvtap_get_user() call copy_to/from_user from within a RCU read-side critical section.
The following patch fixes this issue by releasing the RCU read lock before calling these routines, but instead hold a reference to q->sk.
Signed-off-by Sridhar Samudrala <[email protected]>
drivers/net/macvtap.c +36 -20
@@ -160,8 +160,12 @@ static void macvtap_del_queues(struct nestatic inline struct macvtap_queue *macvtap_file_get_queue (struct file *file) { +struct macvtap_queue *q;rcu_read_lock_bh (); -return rcu_dereference (file->private_data); + q =rcu_dereference (file->private_data); +if (!q) +rcu_read_unlock_bh (); +return q; }static inline void macvtap_file_put_queue (void )
@@ -313,13 +317,14 @@ static unsigned int macvtap_poll(struct sock_writeable(&q->sk))) mask |=POLLOUT |POLLWRNORM ; -out:macvtap_file_put_queue (); + +out:return mask; }/* Get packet from user space buffer */ -static ssize_t macvtap_get_user (struct macvtap_queue *q, +static ssize_t macvtap_get_user (struct macvlan_dev *vlan,struct sock *sk,struct iovec *iv,size_t count,int noblock) {
@@ -330,10 +335,10 @@ static ssize_t macvtap_get_user(struct mif (unlikely (len <ETH_HLEN ))return -EINVAL ; - skb =sock_alloc_send_skb (&q->sk,NET_IP_ALIGN + len, noblock, &err); + skb =sock_alloc_send_skb (sk,NET_IP_ALIGN + len, noblock, &err);if (!skb) { -macvlan_count_rx (q->vlan,0 , false, false); +macvlan_count_rx (vlan,0 , false, false);return err; }
@@ -341,14 +346,14 @@ static ssize_t macvtap_get_user(struct mskb_put (skb, count);if (skb_copy_datagram_from_iovec (skb,0 , iv, len)) { -macvlan_count_rx (q->vlan,0 , false, false); +macvlan_count_rx (vlan,0 , false, false);kfree_skb (skb);return -EFAULT ; }skb_set_network_header (skb,ETH_HLEN ); -macvlan_start_xmit (skb, q->vlan->dev); +macvlan_start_xmit (skb, vlan->dev);return count; }
@@ -359,23 +364,29 @@ static ssize_t macvtap_aio_write(structstruct file *file = iocb->ki_filp;ssize_t result = -ENOLINK ;struct macvtap_queue *q =macvtap_file_get_queue (file); +struct macvlan_dev *vlan; +struct sock *sk;if (!q) goto out; - result =macvtap_get_user (q, (struct iovec *) iv,iov_length (iv, count), + vlan = q->vlan; + sk = &q->sk; +sock_hold (sk); +macvtap_file_put_queue (); + + result =macvtap_get_user (vlan, sk, (struct iovec *) iv,iov_length (iv, count), file->f_flags &O_NONBLOCK ); +sock_put (sk); out: -macvtap_file_put_queue ();return result; }/* Put packet to the user space buffer */ -static ssize_t macvtap_put_user (struct macvtap_queue *q, +static ssize_t macvtap_put_user (struct macvlan_dev *vlan,struct sk_buff *skb,struct iovec *iv,int len) { -struct macvlan_dev *vlan = q->vlan;int ret; len =min_t (int, skb->len, len);
@@ -392,15 +403,20 @@ static ssize_t macvtap_aio_read(struct k {struct file *file = iocb->ki_filp;struct macvtap_queue *q =macvtap_file_get_queue (file); +struct macvlan_dev *vlan; +struct sock *sk;DECLARE_WAITQUEUE (wait, current);struct sk_buff *skb;ssize_t len, ret =0 ; -if (!q) { - ret = -ENOLINK ; - goto out; - } +if (!q) +return -ENOLINK ; + + vlan = q->vlan; + sk = &q->sk; +sock_hold (sk); +macvtap_file_put_queue (); len =iov_length (iv, count);if (len <0 ) {
@@ -408,12 +424,12 @@ static ssize_t macvtap_aio_read(struct k goto out; } -add_wait_queue (q->sk.sk_sleep, &wait); +add_wait_queue (sk->sk_sleep, &wait);while (len) { current->state =TASK_INTERRUPTIBLE ;/* Read frames from the queue */ - skb =skb_dequeue (&q->sk.sk_receive_queue); + skb =skb_dequeue (&sk->sk_receive_queue);if (!skb) {if (file->f_flags &O_NONBLOCK ) { ret = -EAGAIN ;
@@ -427,16 +443,16 @@ static ssize_t macvtap_aio_read(struct kschedule ();continue ; } - ret =macvtap_put_user (q, skb, (struct iovec *) iv, len); + ret =macvtap_put_user (vlan, skb, (struct iovec *) iv, len);kfree_skb (skb);break ; } current->state =TASK_RUNNING ; -remove_wait_queue (q->sk.sk_sleep, &wait); +remove_wait_queue (sk->sk_sleep, &wait); out: -macvtap_file_put_queue (); +sock_put (sk);return ret; }
include/linux/if_macvlan.h +3 -1
@@ -42,8 +42,9 @@ static inline void macvlan_count_rx(cons bool multicast) {struct macvlan_rx_stats *rx_stats; +int cpu =get_cpu (); - rx_stats =per_cpu_ptr (vlan->rx_stats,smp_processor_id ()); + rx_stats =per_cpu_ptr (vlan->rx_stats, cpu);if (likely (success)) { rx_stats->rx_packets++;; rx_stats->rx_bytes += len;
@@ -52,6 +53,7 @@ static inline void macvlan_count_rx(cons }else { rx_stats->rx_errors++; } +put_cpu (); } extern int macvlan_common_newlink(struct net_device *dev,
Quoted 11 lines · Ed Swierk
I've encountered some more problems, with various users of
macvtap_file_get_queue()either calling or neglecting to callmacvtap_file_put_queue()in error cases.I modified your patch so that when
macvtap_file_get_queue()returns 0, it also callsrcu_read_unlock_bh(), and modified the users appropriately.This patch also incorporates my preemption fix for
macvlan_count_rx().Signed-off-by Ed Swierk <[email protected]>
Good idea, I'll incorporate it into my next fix, need to think about it some more to make sure I catch all the corner cases of the lifetime rules.
Thanks,
Arnd
Quoted 8 lines · Sridhar Samudrala
I am also seeing this issue with net-next-2.6.
Basicallymacvtap_put_user()andmacvtap_get_user()call copy_to/from_user from within a RCU read-side critical section.The following patch fixes this issue by releasing the RCU read lock before calling these routines, but instead hold a reference to
q->sk.Signed-off-by Sridhar Samudrala <[email protected]>
Yes, we need something like this, but we also need to protect the device from going away. The concept right now is to use file_get_queue to protect both the macvtap_queue and the macvlan_dev from going away. The sock_hold will keep the macvtap_queue around, but as far as I can tell, a user could still destroy the macvlan_dev using netlink at the same time, which still breaks.
Quoted 26 lines
/* Get packet from user space buffer */
drivers/net/macvtap.c +4
-4-static ssize_t macvtap_get_user (struct macvtap_queue *q, +static ssize_t macvtap_get_user (struct macvlan_dev *vlan,struct sock *sk,const struct iovec *iv,size_t count,int noblock) {@@ -331,10 +331,10 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,if (unlikely (len <ETH_HLEN ))return -EINVAL ; - skb =sock_alloc_send_skb (&q->sk,NET_IP_ALIGN + len, noblock, &err); + skb =sock_alloc_send_skb (sk,NET_IP_ALIGN + len, noblock, &err);if (!skb) { -macvlan_count_rx (q->vlan,0 , false, false); +macvlan_count_rx (vlan,0 , false, false);return err; }@@ -342,14 +342,14 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,skb_put (skb, count);if (skb_copy_datagram_from_iovec (skb,0 , iv,0 , len)) { -macvlan_count_rx (q->vlan,0 , false, false); +macvlan_count_rx (vlan,0 , false, false);kfree_skb (skb);
drivers/net/macvtap.c static ssize_t macvtap_get_user(struct macvtap_queue *q, · line 355
Quoted 10 lines
drivers/net/macvtap.c +1
-1return -EFAULT ; }skb_set_network_header (skb,ETH_HLEN ); -macvlan_start_xmit (skb, q->vlan->dev); +macvlan_start_xmit (skb, vlan->dev);return count; }
What are these changes for? The lifetime of q is the same as &q->sk, so it won't change anything, right?
Moving the macvlan_count_rx and maxclan_start_xmit under the lock should be fine though, but we'd have to take it twice then for each transmit.
I'd hope that this could get simpler by adding zero-copy transmit, where we first get_user() the whole buffer and do the rest under rcu_read_lock_bh().
Quoted 12 lines
drivers/net/macvtap.c +2
-1@@ -393,15 +399,20 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv, {struct file *file = iocb->ki_filp;struct macvtap_queue *q =macvtap_file_get_queue (file); +struct macvlan_dev *vlan; +struct sock *sk;DECLARE_WAITQUEUE (wait, current);struct sk_buff *skb;ssize_t len, ret =0 ; -if (!q) {
drivers/net/macvtap.c static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv, · line 415
Quoted 10 lines
drivers/net/macvtap.c +7
-3- ret = -ENOLINK ; - goto out; - } +if (!q) +return -ENOLINK ; + + vlan = q->vlan; + sk = &q->sk; +sock_hold (sk); +macvtap_file_put_queue ();
Here, we probably need to prevent vlan from going away by dev_hold(), not just sock_hold(). Or is one implied by the other?
Arnd
Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · Sridhar Samudrala
I am also seeing this issue with net-next-2.6.
Basicallymacvtap_put_user()andmacvtap_get_user()call copy_to/from_user from within a RCU read-side critical section.The following patch fixes this issue by releasing the RCU read lock before calling these routines, but instead hold a reference to
q->sk.
Quoted 8 lines
Quoted 1 line
Signed-off-by Sridhar Samudrala <[email protected]>Yes, we need something like this, but we also need to protect the device from going away. The concept right now is to use
file_get_queueto protect both themacvtap_queueand themacvlan_devfrom going away. Thesock_holdwill keep themacvtap_queuearound, but as far as I can tell, a user could still destroy themacvlan_devusing netlink at the same time, which still breaks.
may be we should do a dev_hold() in macvtap_set_queue() and dev_put() in macvtap_del_queue() so that the underlying device cannot go away as long the macvtap fd is open.
Thanks
Sridhar
Quoted 17 lines · Sridhar Samudrala
Quoted 8 lines · Arnd Bergmann
Quoted 7 lines · Sridhar Samudrala
I am also seeing this issue with net-next-2.6.
Basicallymacvtap_put_user()andmacvtap_get_user()call copy_to/from_user from within a RCU read-side critical section.The following patch fixes this issue by releasing the RCU read lock before calling these routines, but instead hold a reference to
q->sk.Quoted 7 lines
Quoted 1 line
Signed-off-by Sridhar Samudrala <[email protected]>Yes, we need something like this, but we also need to protect the device from going away. The concept right now is to use
file_get_queueto protect both themacvtap_queueand themacvlan_devfrom going away. Thesock_holdwill keep themacvtap_queuearound, but as far as I can tell, a user could still destroy themacvlan_devusing netlink at the same time, which still breaks.
Quoted 3 lines
may be we should do a
dev_hold()inmacvtap_set_queue()anddev_put()inmacvtap_del_queue()so that the underlying device cannot go away as long the macvtap fd is open.
You either need some kind of loose binding (f.i. using the ifindex) or need to handle the case that the device goes away asynchronously by indicating an error to the socket and unbinding it.
But you can't make the lifetime of the device dependant on the socket.
PATCH 1/1 net/macvtap: fix reference counting
The RCU usage in the original code was broken because there are cases where we possibly sleep with rcu_read_lock held. As a fix, change the macvtap_file_get_queue to get a reference on the socket and the netdev instead of taking the full rcu_read_lock.
Also, change macvtap_file_get_queue failure case to not require a subsequent macvtap_file_put_queue, as pointed out by Ed Swierk.
Signed-off-by Arnd Bergmann <[email protected]>
Cc Ed Swierk <[email protected]>
Cc Sridhar Samudrala <[email protected]>
--- drivers/net/macvtap.c | 57 +++++++++++++++++++++++++++++++----------------- 1 files changed, 37 insertions(+), 20 deletions(-)
Sridhar, Ed: Does this look ok to you? I'm still working on restoring my test setup, but I'd like you to take a look at this version.
drivers/net/macvtap.c +37 -20
@@ -159,8 +159,12 @@ static void macvtap_del_queues(struct net_device *dev)static inline struct macvtap_queue *macvtap_file_get_queue (struct file *file) { +struct macvtap_queue *q;rcu_read_lock_bh (); -return rcu_dereference (file->private_data); + q =rcu_dereference (file->private_data); +if (!q) +rcu_read_unlock_bh (); +return q; }static inline void macvtap_file_put_queue (void )
@@ -314,13 +318,13 @@ static unsigned int macvtap_poll(struct file *file, poll_table * wait) sock_writeable(&q->sk))) mask |=POLLOUT |POLLWRNORM ; -out:macvtap_file_put_queue (); +out:return mask; }/* Get packet from user space buffer */ -static ssize_t macvtap_get_user (struct macvtap_queue *q, +static ssize_t macvtap_get_user (struct macvlan_dev *vlan,struct sock *sk,const struct iovec *iv,size_t count,int noblock) {
@@ -331,10 +335,10 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,if (unlikely (len <ETH_HLEN ))return -EINVAL ; - skb =sock_alloc_send_skb (&q->sk,NET_IP_ALIGN + len, noblock, &err); + skb =sock_alloc_send_skb (sk,NET_IP_ALIGN + len, noblock, &err);if (!skb) { -macvlan_count_rx (q->vlan,0 , false, false); +macvlan_count_rx (vlan,0 , false, false);return err; }
@@ -342,14 +346,14 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,skb_put (skb, count);if (skb_copy_datagram_from_iovec (skb,0 , iv,0 , len)) { -macvlan_count_rx (q->vlan,0 , false, false); +macvlan_count_rx (vlan,0 , false, false);kfree_skb (skb);return -EFAULT ; }skb_set_network_header (skb,ETH_HLEN ); -macvlan_start_xmit (skb, q->vlan->dev); +macvlan_start_xmit (skb, vlan->dev);return count; }
@@ -360,23 +364,29 @@ static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv,struct file *file = iocb->ki_filp;ssize_t result = -ENOLINK ;struct macvtap_queue *q =macvtap_file_get_queue (file); +struct macvlan_dev *vlan; +struct sock *sk;if (!q) goto out; - result =macvtap_get_user (q, iv,iov_length (iv, count), + vlan = q->vlan; + sk = &q->sk; +sock_hold (sk); +macvtap_file_put_queue (); + + result =macvtap_get_user (vlan, sk, iv,iov_length (iv, count), file->f_flags &O_NONBLOCK ); +sock_put (sk); out: -macvtap_file_put_queue ();return result; }/* Put packet to the user space buffer */ -static ssize_t macvtap_put_user (struct macvtap_queue *q, +static ssize_t macvtap_put_user (struct macvlan_dev *vlan,const struct sk_buff *skb,const struct iovec *iv,int len) { -struct macvlan_dev *vlan = q->vlan;int ret; len =min_t (int, skb->len, len);
@@ -393,15 +403,20 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv, {struct file *file = iocb->ki_filp;struct macvtap_queue *q =macvtap_file_get_queue (file); +struct macvlan_dev *vlan; +struct sock *sk;DECLARE_WAITQUEUE (wait, current);struct sk_buff *skb;ssize_t len, ret =0 ; -if (!q) { - ret = -ENOLINK ; - goto out; - } +if (!q) +return -ENOLINK ; + + vlan = q->vlan; + sk = &q->sk; +sock_hold (sk); +macvtap_file_put_queue (); len =iov_length (iv, count);if (len <0 ) {
@@ -409,12 +424,12 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv, goto out; } -add_wait_queue (q->sk.sk_sleep, &wait); +add_wait_queue (sk->sk_sleep, &wait);while (len) { current->state =TASK_INTERRUPTIBLE ;/* Read frames from the queue */ - skb =skb_dequeue (&q->sk.sk_receive_queue); + skb =skb_dequeue (&sk->sk_receive_queue);if (!skb) {if (file->f_flags &O_NONBLOCK ) { ret = -EAGAIN ;
@@ -428,16 +443,16 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,schedule ();continue ; } - ret =macvtap_put_user (q, skb, iv, len); + ret =macvtap_put_user (vlan, skb, iv, len);kfree_skb (skb);break ; } current->state =TASK_RUNNING ; -remove_wait_queue (q->sk.sk_sleep, &wait); +remove_wait_queue (sk->sk_sleep, &wait); out: -macvtap_file_put_queue (); +sock_put (sk);return ret; }
@@ -485,6 +500,8 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,return -EFAULT ; q =macvtap_file_get_queue (file); +if (!q) +return -ENOLINK ; q->sk.sk_sndbuf = u;macvtap_file_put_queue ();return 0 ;
Signature · 1 line
1.6.3.3
PATCH v2 1/1 net/macvtap: fix reference counting
The RCU usage in the original code was broken because there are cases where we possibly sleep with rcu_read_lock held. As a fix, change the macvtap_file_get_queue to get a reference on the socket and the netdev instead of taking the full rcu_read_lock.
Also, change macvtap_file_get_queue failure case to not require a subsequent macvtap_file_put_queue, as pointed out by Ed Swierk.
Signed-off-by Arnd Bergmann <[email protected]>
Cc Ed Swierk <[email protected]>
Cc Sridhar Samudrala <[email protected]>
--- drivers/net/macvtap.c | 35 ++++++++++++++++++++++------------- 1 files changed, 22 insertions(+), 13 deletions(-)
Please disregard v1 of this patch, I accidentally sent Sridhar's patch...
drivers/net/macvtap.c +22 -13
@@ -70,7 +70,8 @@ static struct cdev macvtap_cdev;* exists. * * The callbacks from macvlan are always done with rcu_read_lock held -* already, while in the file_operations, we get it ourselves. +* 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
@@ -159,13 +160,21 @@ static void macvtap_del_queues(struct net_device *dev)static inline struct macvtap_queue *macvtap_file_get_queue (struct file *file) { +struct macvtap_queue *q;rcu_read_lock_bh (); -return rcu_dereference (file->private_data); + q =rcu_dereference (file->private_data); +if (q) { +sock_hold (&q->sk); +dev_hold (q->vlan->dev); + } +rcu_read_unlock_bh (); +return q; } -static inline void macvtap_file_put_queue (void ) +static inline void macvtap_file_put_queue (struct macvtap_queue *q) { -rcu_read_unlock_bh (); +sock_put (&q->sk); +dev_put (q->vlan->dev); }/*
@@ -314,8 +323,8 @@ static unsigned int macvtap_poll(struct file *file, poll_table * wait) sock_writeable(&q->sk))) mask |=POLLOUT |POLLWRNORM ; +macvtap_file_put_queue (q); out: -macvtap_file_put_queue ();return mask; }
@@ -366,8 +375,8 @@ static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv, result =macvtap_get_user (q, iv,iov_length (iv, count), file->f_flags &O_NONBLOCK ); +macvtap_file_put_queue (q); out: -macvtap_file_put_queue ();return result; }
@@ -398,10 +407,8 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,struct sk_buff *skb;ssize_t len, ret =0 ; -if (!q) { - ret = -ENOLINK ; - goto out; - } +if (!q) +return -ENOLINK ; len =iov_length (iv, count);if (len <0 ) {
@@ -437,7 +444,7 @@ 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 (); +macvtap_file_put_queue (q);return ret; }
@@ -468,7 +475,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,if (!q)return -ENOLINK ;memcpy (devname, q->vlan->dev->name,sizeof (devname)); -macvtap_file_put_queue (); +macvtap_file_put_queue (q);if (copy_to_user (&ifr->ifr_name, q->vlan->dev->name,IFNAMSIZ ) ||put_user ((TUN_TAP_DEV |TUN_NO_PI ), &ifr->ifr_flags))
@@ -485,8 +492,10 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,return -EFAULT ; q =macvtap_file_get_queue (file); +if (!q) +return -ENOLINK ; q->sk.sk_sndbuf = u; -macvtap_file_put_queue (); +macvtap_file_put_queue (q);return 0 ;case TUNSETOFFLOAD :
Signature · 1 line
1.6.3.3
Re: [PATCH v2] net/macvtap: fix reference counting
Quoted 9 lines · Arnd Bergmann
The RCU usage in the original code was broken because there are cases where we possibly sleep with
rcu_read_lockheld. As a fix, change themacvtap_file_get_queueto get a reference on the socket and the netdev instead of taking the fullrcu_read_lock.Also, change
macvtap_file_get_queuefailure case to not require a subsequentmacvtap_file_put_queue, as pointed out by Ed Swierk.
Looks good.
Quoted 113 lines
Signed-off-by Arnd Bergmann <[email protected]>Cc Ed Swierk <[email protected]>Cc Sridhar Samudrala <[email protected]>--- drivers/net/macvtap.c | 35 ++++++++++++++++++++++------------- 1 files changed, 22 insertions(+), 13 deletions(-)Please disregard v1 of this patch, I accidentally sent Sridhar's patch...
drivers/net/macvtap.c +22
-13@@ -70,7 +70,8 @@ static struct cdev macvtap_cdev;* exists. * * The callbacks from macvlan are always done with rcu_read_lock held -* already, while in the file_operations, we get it ourselves. +* 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 @@ -159,13 +160,21 @@ static void macvtap_del_queues(struct net_device *dev)static inline struct macvtap_queue *macvtap_file_get_queue (struct file *file) { +struct macvtap_queue *q;rcu_read_lock_bh (); -return rcu_dereference (file->private_data); + q =rcu_dereference (file->private_data); +if (q) { +sock_hold (&q->sk); +dev_hold (q->vlan->dev); + } +rcu_read_unlock_bh (); +return q; } -static inline void macvtap_file_put_queue (void ) +static inline void macvtap_file_put_queue (struct macvtap_queue *q) { -rcu_read_unlock_bh (); +sock_put (&q->sk); +dev_put (q->vlan->dev); }/* @@ -314,8 +323,8 @@ static unsigned int macvtap_poll(struct file *file, poll_table * wait) sock_writeable(&q->sk))) mask |=POLLOUT |POLLWRNORM ; +macvtap_file_put_queue (q); out: -macvtap_file_put_queue ();return mask; }@@ -366,8 +375,8 @@ static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv, result =macvtap_get_user (q, iv,iov_length (iv, count), file->f_flags &O_NONBLOCK ); +macvtap_file_put_queue (q); out: -macvtap_file_put_queue ();return result; }@@ -398,10 +407,8 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,struct sk_buff *skb;ssize_t len, ret =0 ; -if (!q) { - ret = -ENOLINK ; - goto out; - } +if (!q) +return -ENOLINK ; len =iov_length (iv, count);if (len <0 ) {@@ -437,7 +444,7 @@ 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 (); +macvtap_file_put_queue (q);return ret; }@@ -468,7 +475,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,if (!q)return -ENOLINK ;memcpy (devname, q->vlan->dev->name,sizeof (devname)); -macvtap_file_put_queue (); +macvtap_file_put_queue (q);if (copy_to_user (&ifr->ifr_name, q->vlan->dev->name,IFNAMSIZ ) ||put_user ((TUN_TAP_DEV |TUN_NO_PI ), &ifr->ifr_flags))@@ -485,8 +492,10 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,return -EFAULT ; q =macvtap_file_get_queue (file); +if (!q) +return -ENOLINK ; q->sk.sk_sndbuf = u; -macvtap_file_put_queue (); +macvtap_file_put_queue (q);return 0 ;case TUNSETOFFLOAD :
Re: [PATCH v2] net/macvtap: fix reference counting
From: Sridhar Samudrala <[email protected]>
Date: Thu, 11 Feb 2010 13:09:37 -0800
Quoted 7 lines · Arnd Bergmann
Quoted 6 lines
The RCU usage in the original code was broken because there are cases where we possibly sleep with
rcu_read_lockheld. As a fix, change themacvtap_file_get_queueto get a reference on the socket and the netdev instead of taking the fullrcu_read_lock.
Quoted 7 lines
Quoted 3 lines
Also, change
macvtap_file_get_queuefailure case to not require a subsequentmacvtap_file_put_queue, as pointed out by Ed Swierk.Looks good.
Applied.
Re: [PATCH v2] net/macvtap: fix reference counting
Quoted 4 lines · David Miller
From: Sridhar Samudrala <[email protected]>
Quoted 1 line
Applied.
Thanks for applying this one, but unfortunately I had reworked the patch in a different way in the meantime and Sridhar has added another patch conflicting with my rework. I think I've got it all sorted out now, so I'll send a new series with the three patches I was intending to get merged, but rebased on this one and mostly reverting it.
Sridhar, please have a look at this series and send an Ack if you're fine with it.
Arnd
PATCH v3 1/3 macvtap: rework object lifetime rules
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 <[email protected]>
--- drivers/net/macvtap.c | 183 ++++++++++++++++++++++++------------------------- 1 files changed, 91 insertions(+), 92 deletions(-)
drivers/net/macvtap.c +91 -92
@@ -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);
@@ -101,11 +90,12 @@ 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);
@@ -113,28 +103,25 @@ out: }/* -* 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 ();
@@ -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); }/*
@@ -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);
@@ -300,13 +286,14 @@ out: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)
@@ -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; }
@@ -334,6 +320,7 @@ 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;
@@ -341,26 +328,37 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,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,
@@ -368,15 +366,10 @@ static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv, {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; }
@@ -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; }
@@ -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 ) {
@@ -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; }
@@ -454,12 +451,13 @@ out: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 :
@@ -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))
@@ -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 :
Signature · 1 line
1.6.3.3
Re: [PATCH 1/3] macvtap: rework object lifetime rules
Quoted 12 lines · Arnd Bergmann
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_queuefrom open files.This includes
- need forGFP_ATOMICallocations for skbs
- potential deadlocks when copy_*_user sleeps
- inability to work with vhost-net
Quoted 7 lines
Changing the lifetime of
macvtap_queueto always depend on the open file solves all these. The RCU reference simply moves one step down to the reference on themacvlan_dev, which we only need for nonblocking operations.Signed-off-by Arnd Bergmann <[email protected]>
Reviewed and tested.
Quoted 361 lines
--- drivers/net/macvtap.c | 183 ++++++++++++++++++++++++------------------------- 1 files changed, 91 insertions(+), 92 deletions(-)drivers/net/macvtap.c +91
-92@@ -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);@@ -101,11 +90,12 @@ 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);@@ -113,28 +103,25 @@ out: }/* -* 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 ();@@ -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); }/* @@ -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);@@ -300,13 +286,14 @@ out: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)@@ -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; }@@ -334,6 +320,7 @@ 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;@@ -341,26 +328,37 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,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,@@ -368,15 +366,10 @@ static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv, {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; }@@ -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; }@@ -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 ) {@@ -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; }@@ -454,12 +451,13 @@ out: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 :@@ -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))@@ -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 :
Re: [PATCH 1/3] macvtap: rework object lifetime rules
In reply to Arnd BergmannFrom: Arnd Bergmann <[email protected]>
Date: Thu, 18 Feb 2010 16:45:36 +0100
Quoted 12 lines
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_queuefrom open files.This includes
- need forGFP_ATOMICallocations for skbs
- potential deadlocks when copy_*_user sleeps
- inability to work with vhost-net
Quoted 7 lines
Changing the lifetime of
macvtap_queueto always depend on the open file solves all these. The RCU reference simply moves one step down to the reference on themacvlan_dev, which we only need for nonblocking operations.Signed-off-by Arnd Bergmann <[email protected]>
Applied.
PATCH v3 2/3 net/macvtap: add vhost support
In reply to Arnd BergmannThis adds support for passing a macvtap file descriptor into vhost-net, much like we already do for tun/tap.
Most of the new code is taken from the respective patch in the tun driver and may get consolidated in the future.
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/macvtap.c | 98 ++++++++++++++++++++++++++++++++++--------- drivers/vhost/Kconfig | 2 +- drivers/vhost/net.c | 8 +++- include/linux/if_macvlan.h | 13 ++++++ 4 files changed, 97 insertions(+), 24 deletions(-)
drivers/net/macvtap.c +77 -21
@@ -58,6 +58,8 @@ static unsigned int macvtap_major;static struct class *macvtap_class;static struct cdev macvtap_cdev; +static const struct proto_ops macvtap_socket_ops; +/* * RCU usage: * The macvtap_queue and the macvlan_dev are loosely coupled, the
@@ -176,7 +178,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)return -ENOLINK ;skb_queue_tail (&q->sk.sk_receive_queue, skb); -wake_up (q->sk.sk_sleep); +wake_up_interruptible_poll (q->sk.sk_sleep,POLLIN |POLLRDNORM |POLLRDBAND );return 0 ; }
@@ -242,7 +244,7 @@ static void macvtap_sock_write_space(struct sock *sk)return ;if (sk->sk_sleep &&waitqueue_active (sk->sk_sleep)) -wake_up_interruptible_sync (sk->sk_sleep); +wake_up_interruptible_poll (sk->sk_sleep,POLLOUT |POLLWRNORM |POLLWRBAND ); }static int macvtap_open (struct inode *inode,struct file *file)
@@ -270,6 +272,8 @@ static int macvtap_open(struct inode *inode, struct file *file)init_waitqueue_head (&q->sock.wait); q->sock.type =SOCK_RAW ; q->sock.state =SS_CONNECTED ; + q->sock.file = file; + q->sock.ops = &macvtap_socket_ops;sock_init_data (&q->sock, &q->sk); q->sk.sk_write_space = macvtap_sock_write_space;
@@ -387,32 +391,20 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,rcu_read_lock_bh (); vlan =rcu_dereference (q->vlan); -macvlan_count_rx (vlan, len, ret ==0 ,0 ); +if (vlan) +macvlan_count_rx (vlan, len, ret ==0 ,0 );rcu_read_unlock_bh ();return ret ? ret : len; } -static ssize_t macvtap_aio_read (struct kiocb *iocb,const struct iovec *iv, -unsigned long count,loff_t pos) +static ssize_t macvtap_do_read (struct macvtap_queue *q,struct kiocb *iocb, +const struct iovec *iv,unsigned long len, +int noblock) { -struct file *file = iocb->ki_filp; -struct macvtap_queue *q = file->private_data; -DECLARE_WAITQUEUE (wait, current);struct sk_buff *skb; -ssize_t len, ret =0 ; - -if (!q) { - ret = -ENOLINK ; - goto out; - } - - len =iov_length (iv, count); -if (len <0 ) { - ret = -EINVAL ; - goto out; - } +ssize_t ret =0 ;add_wait_queue (q->sk.sk_sleep, &wait);while (len) {
@@ -421,7 +413,7 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,/* Read frames from the queue */ skb =skb_dequeue (&q->sk.sk_receive_queue);if (!skb) { -if (file->f_flags &O_NONBLOCK ) { +if (noblock) { ret = -EAGAIN ;break ; }
@@ -440,7 +432,24 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv, current->state =TASK_RUNNING ;remove_wait_queue (q->sk.sk_sleep, &wait); +return ret; +} + +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 = file->private_data; +ssize_t len, ret =0 ; + len =iov_length (iv, count); +if (len <0 ) { + ret = -EINVAL ; + goto out; + } + + ret =macvtap_do_read (q, iocb, iv, len, file->f_flags &O_NONBLOCK ); + ret =min_t (ssize_t, ret, len);/* XXX copied from tun.c. Why? */ out:return ret; }
@@ -538,6 +547,53 @@ static const struct file_operations macvtap_fops = {#endif }; +static int macvtap_sendmsg (struct kiocb *iocb,struct socket *sock, +struct msghdr *m,size_t total_len) +{ + struct macvtap_queue *q = container_of(sock, struct macvtap_queue, sock); +return macvtap_get_user (q, m->msg_iov, total_len, + m->msg_flags &MSG_DONTWAIT ); +} + +static int macvtap_recvmsg (struct kiocb *iocb,struct socket *sock, +struct msghdr *m,size_t total_len, +int flags) +{ + struct macvtap_queue *q = container_of(sock, struct macvtap_queue, sock); +int ret; +if (flags & ~(MSG_DONTWAIT |MSG_TRUNC )) +return -EINVAL ; + ret =macvtap_do_read (q, iocb, m->msg_iov, total_len, + flags &MSG_DONTWAIT ); +if (ret > total_len) { + m->msg_flags |=MSG_TRUNC ; + ret = flags &MSG_TRUNC ? ret : total_len; + } +return ret; +} + +/* Ops structure to mimic raw sockets with tun */ +static const struct proto_ops macvtap_socket_ops = { + .sendmsg = macvtap_sendmsg, + .recvmsg = macvtap_recvmsg, +}; + +/* Get an underlying socket object from tun file. Returns error unless file is +* attached to a device. The returned object works like a packet socket, it +* can be used for sock_sendmsg/sock_recvmsg. The caller is responsible for +* holding a reference to the file for as long as the socket is in use. */ +struct socket *macvtap_get_socket (struct file *file) +{ +struct macvtap_queue *q; +if (file->f_op != &macvtap_fops) +return ERR_PTR (-EINVAL ); + q = file->private_data; +if (!q) +return ERR_PTR (-EBADFD ); +return &q->sock; +} +EXPORT_SYMBOL_GPL (macvtap_get_socket); +static int macvtap_init (void ) {int err;
drivers/vhost/Kconfig +1 -1
@@ -1,6 +1,6 @@ config VHOST_NET tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)" - depends on NET && EVENTFD && (TUN || !TUN) && EXPERIMENTAL + depends on NET && EVENTFD && (TUN || !TUN) && (MACVTAP || !MACVTAP) && EXPERIMENTAL ---help--- This kernel module can be loaded in host kernel to accelerate guest networking with virtio_net. Not to be confused with virtio_net
drivers/vhost/net.c +6 -2
@@ -22,6 +22,7 @@#include <linux/if_packet.h> #include <linux/if_arp.h> #include <linux/if_tun.h> +#include <linux/if_macvlan.h> #include <net/sock.h>
@@ -452,13 +453,16 @@ err:return ERR_PTR (r); } -static struct socket *get_tun_socket (int fd) +static struct socket *get_tap_socket (int fd) {struct file *file =fget (fd);struct socket *sock;if (!file)return ERR_PTR (-EBADF ); sock =tun_get_socket (file); +if (!IS_ERR (sock)) +return sock; + sock =macvtap_get_socket (file);if (IS_ERR (sock))fput (file);return sock;
@@ -473,7 +477,7 @@ static struct socket *get_socket(int fd) sock =get_raw_socket (fd);if (!IS_ERR (sock))return sock; - sock =get_tun_socket (fd); + sock =get_tap_socket (fd);if (!IS_ERR (sock))return sock;return ERR_PTR (-ENOTSOCK );
include/linux/if_macvlan.h +13 -0
@@ -7,6 +7,19 @@#include <linux/netlink.h> #include <net/netlink.h> +#if defined(CONFIG_MACVTAP ) || defined(CONFIG_MACVTAP_MODULE ) +struct socket *macvtap_get_socket (struct file *); +#else +#include <linux/err.h> +#include <linux/errno.h> +struct file ; +struct socket ; +static inline struct socket *macvtap_get_socket (struct file *f) +{ +return ERR_PTR (-EINVAL ); +} +#endif /* CONFIG_MACVTAP */ +struct macvlan_port ;struct macvtap_queue ;
Signature · 1 line
1.6.3.3
Re: [PATCH 2/3] net/macvtap: add vhost support
Quoted 7 lines · Arnd Bergmann
This adds support for passing a macvtap file descriptor into vhost-net, much like we already do for tun/tap.
Most of the new code is taken from the respective patch in the tun driver and may get consolidated in the future.
Signed-off-by Arnd Bergmann <[email protected]>
Reviewed and tested.
Quoted 249 lines
--- drivers/net/macvtap.c | 98 ++++++++++++++++++++++++++++++++++--------- drivers/vhost/Kconfig | 2 +- drivers/vhost/net.c | 8 +++- include/linux/if_macvlan.h | 13 ++++++ 4 files changed, 97 insertions(+), 24 deletions(-)drivers/net/macvtap.c +77
-21@@ -58,6 +58,8 @@ static unsigned int macvtap_major;static struct class *macvtap_class;static struct cdev macvtap_cdev; +static const struct proto_ops macvtap_socket_ops; +/* * RCU usage: * The macvtap_queue and the macvlan_dev are loosely coupled, the @@ -176,7 +178,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)return -ENOLINK ;skb_queue_tail (&q->sk.sk_receive_queue, skb); -wake_up (q->sk.sk_sleep); +wake_up_interruptible_poll (q->sk.sk_sleep,POLLIN |POLLRDNORM |POLLRDBAND );return 0 ; }@@ -242,7 +244,7 @@ static void macvtap_sock_write_space(struct sock *sk)return ;if (sk->sk_sleep &&waitqueue_active (sk->sk_sleep)) -wake_up_interruptible_sync (sk->sk_sleep); +wake_up_interruptible_poll (sk->sk_sleep,POLLOUT |POLLWRNORM |POLLWRBAND ); }static int macvtap_open (struct inode *inode,struct file *file)@@ -270,6 +272,8 @@ static int macvtap_open(struct inode *inode, struct file *file)init_waitqueue_head (&q->sock.wait); q->sock.type =SOCK_RAW ; q->sock.state =SS_CONNECTED ; + q->sock.file = file; + q->sock.ops = &macvtap_socket_ops;sock_init_data (&q->sock, &q->sk); q->sk.sk_write_space = macvtap_sock_write_space;@@ -387,32 +391,20 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,rcu_read_lock_bh (); vlan =rcu_dereference (q->vlan); -macvlan_count_rx (vlan, len, ret ==0 ,0 ); +if (vlan) +macvlan_count_rx (vlan, len, ret ==0 ,0 );rcu_read_unlock_bh ();return ret ? ret : len; } -static ssize_t macvtap_aio_read (struct kiocb *iocb,const struct iovec *iv, -unsigned long count,loff_t pos) +static ssize_t macvtap_do_read (struct macvtap_queue *q,struct kiocb *iocb, +const struct iovec *iv,unsigned long len, +int noblock) { -struct file *file = iocb->ki_filp; -struct macvtap_queue *q = file->private_data; -DECLARE_WAITQUEUE (wait, current);struct sk_buff *skb; -ssize_t len, ret =0 ; - -if (!q) { - ret = -ENOLINK ; - goto out; - } - - len =iov_length (iv, count); -if (len <0 ) { - ret = -EINVAL ; - goto out; - } +ssize_t ret =0 ;add_wait_queue (q->sk.sk_sleep, &wait);while (len) {@@ -421,7 +413,7 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,/* Read frames from the queue */ skb =skb_dequeue (&q->sk.sk_receive_queue);if (!skb) { -if (file->f_flags &O_NONBLOCK ) { +if (noblock) { ret = -EAGAIN ;break ; }@@ -440,7 +432,24 @@ static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv, current->state =TASK_RUNNING ;remove_wait_queue (q->sk.sk_sleep, &wait); +return ret; +} + +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 = file->private_data; +ssize_t len, ret =0 ; + len =iov_length (iv, count); +if (len <0 ) { + ret = -EINVAL ; + goto out; + } + + ret =macvtap_do_read (q, iocb, iv, len, file->f_flags &O_NONBLOCK ); + ret =min_t (ssize_t, ret, len);/* XXX copied from tun.c. Why? */ out:return ret; }@@ -538,6 +547,53 @@ static const struct file_operations macvtap_fops = {#endif }; +static int macvtap_sendmsg (struct kiocb *iocb,struct socket *sock, +struct msghdr *m,size_t total_len) +{ + struct macvtap_queue *q = container_of(sock, struct macvtap_queue, sock); +return macvtap_get_user (q, m->msg_iov, total_len, + m->msg_flags &MSG_DONTWAIT ); +} + +static int macvtap_recvmsg (struct kiocb *iocb,struct socket *sock, +struct msghdr *m,size_t total_len, +int flags) +{ + struct macvtap_queue *q = container_of(sock, struct macvtap_queue, sock); +int ret; +if (flags & ~(MSG_DONTWAIT |MSG_TRUNC )) +return -EINVAL ; + ret =macvtap_do_read (q, iocb, m->msg_iov, total_len, + flags &MSG_DONTWAIT ); +if (ret > total_len) { + m->msg_flags |=MSG_TRUNC ; + ret = flags &MSG_TRUNC ? ret : total_len; + } +return ret; +} + +/* Ops structure to mimic raw sockets with tun */ +static const struct proto_ops macvtap_socket_ops = { + .sendmsg = macvtap_sendmsg, + .recvmsg = macvtap_recvmsg, +}; + +/* Get an underlying socket object from tun file. Returns error unless file is +* attached to a device. The returned object works like a packet socket, it +* can be used for sock_sendmsg/sock_recvmsg. The caller is responsible for +* holding a reference to the file for as long as the socket is in use. */ +struct socket *macvtap_get_socket (struct file *file) +{ +struct macvtap_queue *q; +if (file->f_op != &macvtap_fops) +return ERR_PTR (-EINVAL ); + q = file->private_data; +if (!q) +return ERR_PTR (-EBADFD ); +return &q->sock; +} +EXPORT_SYMBOL_GPL (macvtap_get_socket); +static int macvtap_init (void ) {int err;drivers/vhost/Kconfig +1
-1@@ -1,6 +1,6 @@ config VHOST_NET tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)" - depends on NET && EVENTFD && (TUN || !TUN) && EXPERIMENTAL + depends on NET && EVENTFD && (TUN || !TUN) && (MACVTAP || !MACVTAP) && EXPERIMENTAL ---help--- This kernel module can be loaded in host kernel to accelerate guest networking with virtio_net. Not to be confused with virtio_netdrivers/vhost/net.c +6
-2@@ -22,6 +22,7 @@#include <linux/if_packet.h> #include <linux/if_arp.h> #include <linux/if_tun.h> +#include <linux/if_macvlan.h> #include <net/sock.h> @@ -452,13 +453,16 @@ err:return ERR_PTR (r); } -static struct socket *get_tun_socket (int fd) +static struct socket *get_tap_socket (int fd) {struct file *file =fget (fd);struct socket *sock;if (!file)return ERR_PTR (-EBADF ); sock =tun_get_socket (file); +if (!IS_ERR (sock)) +return sock; + sock =macvtap_get_socket (file);if (IS_ERR (sock))fput (file);return sock;@@ -473,7 +477,7 @@ static struct socket *get_socket(int fd) sock =get_raw_socket (fd);if (!IS_ERR (sock))return sock; - sock =get_tun_socket (fd); + sock =get_tap_socket (fd);if (!IS_ERR (sock))return sock;return ERR_PTR (-ENOTSOCK );include/linux/if_macvlan.h +13
-0@@ -7,6 +7,19 @@#include <linux/netlink.h> #include <net/netlink.h> +#if defined(CONFIG_MACVTAP ) || defined(CONFIG_MACVTAP_MODULE ) +struct socket *macvtap_get_socket (struct file *); +#else +#include <linux/err.h> +#include <linux/errno.h> +struct file ; +struct socket ; +static inline struct socket *macvtap_get_socket (struct file *f) +{ +return ERR_PTR (-EINVAL ); +} +#endif /* CONFIG_MACVTAP */ +struct macvlan_port ;struct macvtap_queue ;
Re: [PATCH 2/3] net/macvtap: add vhost support
In reply to Arnd BergmannFrom: Arnd Bergmann <[email protected]>
Date: Thu, 18 Feb 2010 16:46:50 +0100
Quoted 7 lines
This adds support for passing a macvtap file descriptor into vhost-net, much like we already do for tun/tap.
Most of the new code is taken from the respective patch in the tun driver and may get consolidated in the future.
Signed-off-by Arnd Bergmann <[email protected]>
Applied.
PATCH v3 3/3 macvtap: add GSO/csum offload support
In reply to Arnd BergmannAdded flags field to macvtap_queue to enable/disable processing of virtio_net_hdr via IFF_VNET_HDR. This flag is checked to prepend virtio_net_hdr in the receive path and process/skip virtio_net_hdr in the send path.
Original patch by Sridhar, further changes by Arnd.
Signed-off-by Sridhar Samudrala <[email protected]>
Signed-off-by Arnd Bergmann <[email protected]>
--- drivers/net/macvtap.c | 206 +++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 182 insertions(+), 24 deletions(-)
drivers/net/macvtap.c +182 -24
@@ -17,6 +17,7 @@#include <net/net_namespace.h> #include <net/rtnetlink.h> #include <net/sock.h> +#include <linux/virtio_net.h> /* * A macvtap queue is the central object of this driver, it connects
@@ -37,6 +38,7 @@ struct macvtap_queue {struct socket sock;struct macvlan_dev *vlan;struct file *file; +unsigned int flags; };static struct proto macvtap_proto = {
@@ -276,6 +278,7 @@ static int macvtap_open(struct inode *inode, struct file *file) q->sock.ops = &macvtap_socket_ops;sock_init_data (&q->sock, &q->sk); q->sk.sk_write_space = macvtap_sock_write_space; + q->flags =IFF_VNET_HDR |IFF_NO_PI |IFF_TAP ; err =macvtap_set_queue (dev, file, q);if (err)
@@ -318,6 +321,111 @@ out:return mask; } +static inline struct sk_buff *macvtap_alloc_skb (struct sock *sk,size_t prepad, +size_t len,size_t linear, +int noblock,int *err) +{ +struct sk_buff *skb; + +/* Under a page? Don't bother with paged skb. */ +if (prepad + len <PAGE_SIZE || !linear) + linear = len; + + skb =sock_alloc_send_pskb (sk, prepad + linear, len - linear, noblock, + err); +if (!skb) +return NULL ; + +skb_reserve (skb, prepad); +skb_put (skb, linear); + skb->data_len = len - linear; + skb->len += len - linear; + +return skb; +} + +/* +* macvtap_skb_from_vnet_hdr and macvtap_skb_to_vnet_hdr should +* be shared with the tun/tap driver. +*/ +static int macvtap_skb_from_vnet_hdr (struct sk_buff *skb, +struct virtio_net_hdr *vnet_hdr) +{ +unsigned short gso_type =0 ; +if (vnet_hdr->gso_type !=VIRTIO_NET_HDR_GSO_NONE ) { +switch (vnet_hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) { +case VIRTIO_NET_HDR_GSO_TCPV4 : + gso_type =SKB_GSO_TCPV4 ; +break ; +case VIRTIO_NET_HDR_GSO_TCPV6 : + gso_type =SKB_GSO_TCPV6 ; +break ; +case VIRTIO_NET_HDR_GSO_UDP : + gso_type =SKB_GSO_UDP ; +break ; +default : +return -EINVAL ; + } + +if (vnet_hdr->gso_type &VIRTIO_NET_HDR_GSO_ECN ) + gso_type |=SKB_GSO_TCP_ECN ; + +if (vnet_hdr->gso_size ==0 ) +return -EINVAL ; + } + +if (vnet_hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) { +if (!skb_partial_csum_set (skb, vnet_hdr->csum_start, + vnet_hdr->csum_offset)) +return -EINVAL ; + } + +if (vnet_hdr->gso_type !=VIRTIO_NET_HDR_GSO_NONE ) { +skb_shinfo (skb)->gso_size = vnet_hdr->gso_size; +skb_shinfo (skb)->gso_type = gso_type; + +/* Header must be checked, and gso_segs computed. */ +skb_shinfo (skb)->gso_type |=SKB_GSO_DODGY ; +skb_shinfo (skb)->gso_segs =0 ; + } +return 0 ; +} + +static int macvtap_skb_to_vnet_hdr (const struct sk_buff *skb, +struct virtio_net_hdr *vnet_hdr) +{ +memset (vnet_hdr,0 ,sizeof (*vnet_hdr)); + +if (skb_is_gso (skb)) { +struct skb_shared_info *sinfo =skb_shinfo (skb); + +/* This is a hint as to how much should be linear. */ + vnet_hdr->hdr_len =skb_headlen (skb); + vnet_hdr->gso_size = sinfo->gso_size; +if (sinfo->gso_type &SKB_GSO_TCPV4 ) + vnet_hdr->gso_type =VIRTIO_NET_HDR_GSO_TCPV4 ; +else if (sinfo->gso_type &SKB_GSO_TCPV6 ) + vnet_hdr->gso_type =VIRTIO_NET_HDR_GSO_TCPV6 ; +else if (sinfo->gso_type &SKB_GSO_UDP ) + vnet_hdr->gso_type =VIRTIO_NET_HDR_GSO_UDP ; +else +BUG (); +if (sinfo->gso_type &SKB_GSO_TCP_ECN ) + vnet_hdr->gso_type |=VIRTIO_NET_HDR_GSO_ECN ; + }else + vnet_hdr->gso_type =VIRTIO_NET_HDR_GSO_NONE ; + +if (skb->ip_summed ==CHECKSUM_PARTIAL ) { + vnet_hdr->flags =VIRTIO_NET_HDR_F_NEEDS_CSUM ; + vnet_hdr->csum_start = skb->csum_start - +skb_headroom (skb); + vnet_hdr->csum_offset = skb->csum_offset; + }/* else everything is zero */ + +return 0 ; +} + +/* Get packet from user space buffer */ static ssize_t macvtap_get_user(struct macvtap_queue *q, const struct iovec *iv, size_t count,
@@ -327,22 +435,53 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,struct macvlan_dev *vlan;size_t len = count;int err; +struct virtio_net_hdr vnet_hdr = {0 }; +int vnet_hdr_len =0 ; + +if (q->flags &IFF_VNET_HDR ) { + vnet_hdr_len =sizeof (vnet_hdr); + + err = -EINVAL ; +if ((len -= vnet_hdr_len) <0 ) + goto err; + + err =memcpy_fromiovecend ((void *)&vnet_hdr, iv,0 , + vnet_hdr_len); +if (err <0 ) + goto err; +if ((vnet_hdr.flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) && + vnet_hdr.csum_start + vnet_hdr.csum_offset +2 > + vnet_hdr.hdr_len) + vnet_hdr.hdr_len = vnet_hdr.csum_start + + vnet_hdr.csum_offset +2 ; + err = -EINVAL ; +if (vnet_hdr.hdr_len > len) + goto err; + } + err = -EINVAL ;if (unlikely (len <ETH_HLEN )) -return -EINVAL ; + goto err; - skb =sock_alloc_send_skb (&q->sk,NET_IP_ALIGN + len, noblock, &err); + skb =macvtap_alloc_skb (&q->sk,NET_IP_ALIGN , len, vnet_hdr.hdr_len, + noblock, &err);if (!skb) goto err; -skb_reserve (skb,NET_IP_ALIGN ); -skb_put (skb, count); - - err =skb_copy_datagram_from_iovec (skb,0 , iv,0 , len); + err =skb_copy_datagram_from_iovec (skb,0 , iv, vnet_hdr_len, len);if (err) - goto err; + goto err_kfree;skb_set_network_header (skb,ETH_HLEN ); +skb_reset_mac_header (skb); + skb->protocol =eth_hdr (skb)->h_proto; + +if (vnet_hdr_len) { + err =macvtap_skb_from_vnet_hdr (skb, &vnet_hdr); +if (err) + goto err_kfree; + } +rcu_read_lock_bh (); vlan =rcu_dereference (q->vlan);if (vlan)
@@ -353,15 +492,16 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,return count; +err_kfree: +kfree_skb (skb); + err:rcu_read_lock_bh (); vlan =rcu_dereference (q->vlan);if (vlan) -macvlan_count_rx (q->vlan,0 , false, false); +netdev_get_tx_queue (vlan->dev,0 )->tx_dropped++;rcu_read_unlock_bh (); -kfree_skb (skb); -return err; }
@@ -384,10 +524,25 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q, {struct macvlan_dev *vlan;int ret; +int vnet_hdr_len =0 ; + +if (q->flags &IFF_VNET_HDR ) { +struct virtio_net_hdr vnet_hdr; + vnet_hdr_len =sizeof (vnet_hdr); +if ((len -= vnet_hdr_len) <0 ) +return -EINVAL ; + + ret =macvtap_skb_to_vnet_hdr (skb, &vnet_hdr); +if (ret) +return ret; + +if (memcpy_toiovecend (iv, (void *)&vnet_hdr,0 , vnet_hdr_len)) +return -EFAULT ; + } len =min_t (int, skb->len, len); - ret =skb_copy_datagram_const_iovec (skb,0 , iv,0 , len); + ret =skb_copy_datagram_const_iovec (skb,0 , iv, vnet_hdr_len, len);rcu_read_lock_bh (); vlan =rcu_dereference (q->vlan);
@@ -395,7 +550,7 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,macvlan_count_rx (vlan, len, ret ==0 ,0 );rcu_read_unlock_bh (); -return ret ? ret : len; +return ret ? ret : (len + vnet_hdr_len); } static ssize_t macvtap_do_read(struct macvtap_queue *q, struct kiocb *iocb,
@@ -473,9 +628,14 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,/* ignore the name, just look at flags */ if (get_user (u, &ifr->ifr_flags))return -EFAULT ; -if (u != (IFF_TAP |IFF_NO_PI )) -return -EINVAL ; -return 0 ; + + ret =0 ; +if ((u & ~IFF_VNET_HDR ) != (IFF_NO_PI |IFF_TAP )) + ret = -EINVAL ; +else + q->flags = u; + +return ret;case TUNGETIFF :rcu_read_lock_bh ();
@@ -489,13 +649,13 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd, 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)) +put_user (q->flags, &ifr->ifr_flags)) ret = -EFAULT ;dev_put (vlan->dev);return ret;case TUNGETFEATURES : -if (put_user ((IFF_TAP |IFF_NO_PI ), up)) +if (put_user (IFF_TAP |IFF_NO_PI |IFF_VNET_HDR , up))return -EFAULT ;return 0 ;
@@ -509,15 +669,13 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,case TUNSETOFFLOAD :/* let the user check for future flags */ if (arg & ~(TUN_F_CSUM |TUN_F_TSO4 |TUN_F_TSO6 | -TUN_F_TSO_ECN |TUN_F_UFO )) -return -EINVAL ; - -/* TODO: add support for these, so far we don't -support any offload */ -if (arg & (TUN_F_CSUM |TUN_F_TSO4 |TUN_F_TSO6 | -TUN_F_TSO_ECN |TUN_F_UFO )) +TUN_F_TSO_ECN |TUN_F_UFO ))return -EINVAL ; +/* TODO: only accept frames with the features that +got enabled for forwarded frames */ +if (!(q->flags &IFF_VNET_HDR )) +return -EINVAL ;return 0 ;default :
Signature · 1 line
1.6.3.3
Re: [PATCH 3/3] macvtap: add GSO/csum offload support
Quoted 8 lines · Arnd Bergmann
Added flags field to
macvtap_queueto enable/disable processing ofvirtio_net_hdrviaIFF_VNET_HDR. This flag is checked to prependvirtio_net_hdrin the receive path and process/skipvirtio_net_hdrin the send path.Original patch by Sridhar, further changes by Arnd.
Signed-off-by Sridhar Samudrala <[email protected]>Signed-off-by Arnd Bergmann <[email protected]>
The changes look good.
Tested it over gigabit ethernet. I am seeing send side CPU utilization dropping from 80% to 15% with netperf TCP_STREAM from guest to remote host.
Thanks
Sridhar
Quoted 316 lines
--- drivers/net/macvtap.c | 206 +++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 182 insertions(+), 24 deletions(-)drivers/net/macvtap.c +182
-24@@ -17,6 +17,7 @@#include <net/net_namespace.h> #include <net/rtnetlink.h> #include <net/sock.h> +#include <linux/virtio_net.h> /* * A macvtap queue is the central object of this driver, it connects @@ -37,6 +38,7 @@ struct macvtap_queue {struct socket sock;struct macvlan_dev *vlan;struct file *file; +unsigned int flags; };static struct proto macvtap_proto = {@@ -276,6 +278,7 @@ static int macvtap_open(struct inode *inode, struct file *file) q->sock.ops = &macvtap_socket_ops;sock_init_data (&q->sock, &q->sk); q->sk.sk_write_space = macvtap_sock_write_space; + q->flags =IFF_VNET_HDR |IFF_NO_PI |IFF_TAP ; err =macvtap_set_queue (dev, file, q);if (err)@@ -318,6 +321,111 @@ out:return mask; } +static inline struct sk_buff *macvtap_alloc_skb (struct sock *sk,size_t prepad, +size_t len,size_t linear, +int noblock,int *err) +{ +struct sk_buff *skb; + +/* Under a page? Don't bother with paged skb. */ +if (prepad + len <PAGE_SIZE || !linear) + linear = len; + + skb =sock_alloc_send_pskb (sk, prepad + linear, len - linear, noblock, + err); +if (!skb) +return NULL ; + +skb_reserve (skb, prepad); +skb_put (skb, linear); + skb->data_len = len - linear; + skb->len += len - linear; + +return skb; +} + +/* +* macvtap_skb_from_vnet_hdr and macvtap_skb_to_vnet_hdr should +* be shared with the tun/tap driver. +*/ +static int macvtap_skb_from_vnet_hdr (struct sk_buff *skb, +struct virtio_net_hdr *vnet_hdr) +{ +unsigned short gso_type =0 ; +if (vnet_hdr->gso_type !=VIRTIO_NET_HDR_GSO_NONE ) { +switch (vnet_hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN ) { +case VIRTIO_NET_HDR_GSO_TCPV4 : + gso_type =SKB_GSO_TCPV4 ; +break ; +case VIRTIO_NET_HDR_GSO_TCPV6 : + gso_type =SKB_GSO_TCPV6 ; +break ; +case VIRTIO_NET_HDR_GSO_UDP : + gso_type =SKB_GSO_UDP ; +break ; +default : +return -EINVAL ; + } + +if (vnet_hdr->gso_type &VIRTIO_NET_HDR_GSO_ECN ) + gso_type |=SKB_GSO_TCP_ECN ; + +if (vnet_hdr->gso_size ==0 ) +return -EINVAL ; + } + +if (vnet_hdr->flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) { +if (!skb_partial_csum_set (skb, vnet_hdr->csum_start, + vnet_hdr->csum_offset)) +return -EINVAL ; + } + +if (vnet_hdr->gso_type !=VIRTIO_NET_HDR_GSO_NONE ) { +skb_shinfo (skb)->gso_size = vnet_hdr->gso_size; +skb_shinfo (skb)->gso_type = gso_type; + +/* Header must be checked, and gso_segs computed. */ +skb_shinfo (skb)->gso_type |=SKB_GSO_DODGY ; +skb_shinfo (skb)->gso_segs =0 ; + } +return 0 ; +} + +static int macvtap_skb_to_vnet_hdr (const struct sk_buff *skb, +struct virtio_net_hdr *vnet_hdr) +{ +memset (vnet_hdr,0 ,sizeof (*vnet_hdr)); + +if (skb_is_gso (skb)) { +struct skb_shared_info *sinfo =skb_shinfo (skb); + +/* This is a hint as to how much should be linear. */ + vnet_hdr->hdr_len =skb_headlen (skb); + vnet_hdr->gso_size = sinfo->gso_size; +if (sinfo->gso_type &SKB_GSO_TCPV4 ) + vnet_hdr->gso_type =VIRTIO_NET_HDR_GSO_TCPV4 ; +else if (sinfo->gso_type &SKB_GSO_TCPV6 ) + vnet_hdr->gso_type =VIRTIO_NET_HDR_GSO_TCPV6 ; +else if (sinfo->gso_type &SKB_GSO_UDP ) + vnet_hdr->gso_type =VIRTIO_NET_HDR_GSO_UDP ; +else +BUG (); +if (sinfo->gso_type &SKB_GSO_TCP_ECN ) + vnet_hdr->gso_type |=VIRTIO_NET_HDR_GSO_ECN ; + }else + vnet_hdr->gso_type =VIRTIO_NET_HDR_GSO_NONE ; + +if (skb->ip_summed ==CHECKSUM_PARTIAL ) { + vnet_hdr->flags =VIRTIO_NET_HDR_F_NEEDS_CSUM ; + vnet_hdr->csum_start = skb->csum_start - +skb_headroom (skb); + vnet_hdr->csum_offset = skb->csum_offset; + }/* else everything is zero */ + +return 0 ; +} + +/* Get packet from user space buffer */ static ssize_t macvtap_get_user(struct macvtap_queue *q, const struct iovec *iv, size_t count,@@ -327,22 +435,53 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,struct macvlan_dev *vlan;size_t len = count;int err; +struct virtio_net_hdr vnet_hdr = {0 }; +int vnet_hdr_len =0 ; + +if (q->flags &IFF_VNET_HDR ) { + vnet_hdr_len =sizeof (vnet_hdr); + + err = -EINVAL ; +if ((len -= vnet_hdr_len) <0 ) + goto err; + + err =memcpy_fromiovecend ((void *)&vnet_hdr, iv,0 , + vnet_hdr_len); +if (err <0 ) + goto err; +if ((vnet_hdr.flags &VIRTIO_NET_HDR_F_NEEDS_CSUM ) && + vnet_hdr.csum_start + vnet_hdr.csum_offset +2 > + vnet_hdr.hdr_len) + vnet_hdr.hdr_len = vnet_hdr.csum_start + + vnet_hdr.csum_offset +2 ; + err = -EINVAL ; +if (vnet_hdr.hdr_len > len) + goto err; + } + err = -EINVAL ;if (unlikely (len <ETH_HLEN )) -return -EINVAL ; + goto err; - skb =sock_alloc_send_skb (&q->sk,NET_IP_ALIGN + len, noblock, &err); + skb =macvtap_alloc_skb (&q->sk,NET_IP_ALIGN , len, vnet_hdr.hdr_len, + noblock, &err);if (!skb) goto err; -skb_reserve (skb,NET_IP_ALIGN ); -skb_put (skb, count); - - err =skb_copy_datagram_from_iovec (skb,0 , iv,0 , len); + err =skb_copy_datagram_from_iovec (skb,0 , iv, vnet_hdr_len, len);if (err) - goto err; + goto err_kfree;skb_set_network_header (skb,ETH_HLEN ); +skb_reset_mac_header (skb); + skb->protocol =eth_hdr (skb)->h_proto; + +if (vnet_hdr_len) { + err =macvtap_skb_from_vnet_hdr (skb, &vnet_hdr); +if (err) + goto err_kfree; + } +rcu_read_lock_bh (); vlan =rcu_dereference (q->vlan);if (vlan)@@ -353,15 +492,16 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,return count; +err_kfree: +kfree_skb (skb); + err:rcu_read_lock_bh (); vlan =rcu_dereference (q->vlan);if (vlan) -macvlan_count_rx (q->vlan,0 , false, false); +netdev_get_tx_queue (vlan->dev,0 )->tx_dropped++;rcu_read_unlock_bh (); -kfree_skb (skb); -return err; }@@ -384,10 +524,25 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q, {struct macvlan_dev *vlan;int ret; +int vnet_hdr_len =0 ; + +if (q->flags &IFF_VNET_HDR ) { +struct virtio_net_hdr vnet_hdr; + vnet_hdr_len =sizeof (vnet_hdr); +if ((len -= vnet_hdr_len) <0 ) +return -EINVAL ; + + ret =macvtap_skb_to_vnet_hdr (skb, &vnet_hdr); +if (ret) +return ret; + +if (memcpy_toiovecend (iv, (void *)&vnet_hdr,0 , vnet_hdr_len)) +return -EFAULT ; + } len =min_t (int, skb->len, len); - ret =skb_copy_datagram_const_iovec (skb,0 , iv,0 , len); + ret =skb_copy_datagram_const_iovec (skb,0 , iv, vnet_hdr_len, len);rcu_read_lock_bh (); vlan =rcu_dereference (q->vlan);@@ -395,7 +550,7 @@ static ssize_t macvtap_put_user(struct macvtap_queue *q,macvlan_count_rx (vlan, len, ret ==0 ,0 );rcu_read_unlock_bh (); -return ret ? ret : len; +return ret ? ret : (len + vnet_hdr_len); } static ssize_t macvtap_do_read(struct macvtap_queue *q, struct kiocb *iocb,@@ -473,9 +628,14 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,/* ignore the name, just look at flags */ if (get_user (u, &ifr->ifr_flags))return -EFAULT ; -if (u != (IFF_TAP |IFF_NO_PI )) -return -EINVAL ; -return 0 ; + + ret =0 ; +if ((u & ~IFF_VNET_HDR ) != (IFF_NO_PI |IFF_TAP )) + ret = -EINVAL ; +else + q->flags = u; + +return ret;case TUNGETIFF :rcu_read_lock_bh ();@@ -489,13 +649,13 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd, 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)) +put_user (q->flags, &ifr->ifr_flags)) ret = -EFAULT ;dev_put (vlan->dev);return ret;case TUNGETFEATURES : -if (put_user ((IFF_TAP |IFF_NO_PI ), up)) +if (put_user (IFF_TAP |IFF_NO_PI |IFF_VNET_HDR , up))return -EFAULT ;return 0 ;@@ -509,15 +669,13 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,case TUNSETOFFLOAD :/* let the user check for future flags */ if (arg & ~(TUN_F_CSUM |TUN_F_TSO4 |TUN_F_TSO6 | -TUN_F_TSO_ECN |TUN_F_UFO )) -return -EINVAL ; - -/* TODO: add support for these, so far we don't -support any offload */ -if (arg & (TUN_F_CSUM |TUN_F_TSO4 |TUN_F_TSO6 | -TUN_F_TSO_ECN |TUN_F_UFO )) +TUN_F_TSO_ECN |TUN_F_UFO ))return -EINVAL ; +/* TODO: only accept frames with the features that +got enabled for forwarded frames */ +if (!(q->flags &IFF_VNET_HDR )) +return -EINVAL ;return 0 ;default :
Re: [PATCH 3/3] macvtap: add GSO/csum offload support
In reply to Arnd BergmannFrom: Arnd Bergmann <[email protected]>
Date: Thu, 18 Feb 2010 16:48:17 +0100
Quoted 8 lines
Added flags field to
macvtap_queueto enable/disable processing ofvirtio_net_hdrviaIFF_VNET_HDR. This flag is checked to prependvirtio_net_hdrin the receive path and process/skipvirtio_net_hdrin the send path.Original patch by Sridhar, further changes by Arnd.
Signed-off-by Sridhar Samudrala <[email protected]>Signed-off-by Arnd Bergmann <[email protected]>
Applied.
Re: [PATCH v2] net/macvtap: fix reference counting
In reply to Arnd BergmannQuoted 9 lines · Arnd Bergmann
The RCU usage in the original code was broken because there are cases where we possibly sleep with
rcu_read_lockheld. As a fix, change themacvtap_file_get_queueto get a reference on the socket and the netdev instead of taking the fullrcu_read_lock.Also, change
macvtap_file_get_queuefailure case to not require a subsequentmacvtap_file_put_queue, as pointed out by Ed Swierk.
Works for me. Thanks.
Acked-by Ed Swierk <[email protected]>
David Miller
maintainerFrom: Arnd Bergmann <[email protected]>
Date: Sat, 30 Jan 2010 23:22:15 +0100
Quoted 12 lines
This is the fourth version of the macvtap driver,
based on the comments I got for the last version
I got a few days ago. Very few changes:* release netdev in chardev open function so
we can destroy it properly.
* Implement TUNSETSNDBUF
* fix sleeping call inrcu_read_lock
* Fix comment in namespace isolation patch
* Fix small context difference to make it apply
to net-next
Quoted 2 lines
I can't really test here while travelling, so please give it a go if you're interested in this driver.
All applied to net-next-2.6, thanks!
v4 cover macvtap driver
In reply to Arnd BergmannThis is the fourth version of the macvtap driver,
based on the comments I got for the last version
I got a few days ago. Very few changes:
* release netdev in chardev open function so
we can destroy it properly.
* Implement TUNSETSNDBUF
* fix sleeping call in rcu_read_lock
* Fix comment in namespace isolation patch
* Fix small context difference to make it apply
to net-next
I can't really test here while travelling, so please give it a go if you're interested in this driver.
Arnd