Max Krasnyansky wrote: > Please see the following thread to get some context on this > http://marc.info/?l=linux-netdev&m=121564433018903&w=2 > > Basically the issue is that current multi-cast filtering stuff in > the TUN/TAP driver is seriously broken. > Original patch went in without proper review and ACK. It was broken and > confusing to start with and subsequent patches broke it completely. > To give you an idea of what's broken here are some of the issues: > > - Very confusing comments throughout the code that imply that the > character device is a network interface in its own right, and that packets > are passed between the two nics. Which is completely wrong. > > - Wrong set of ioctls is used for setting up filters. They look like > shortcuts for manipulating state of the tun/tap network interface but > in reality manipulate the state of the TX filter. > > - ioctls that were originally used for setting address of the the TX filter > got "fixed" and now set the address of the network interface itself. Which > made filter totaly useless. > > - Filtering is done too late. Instead of filtering early on, to avoid > unnecessary wakeups, filtering is done in the read() call. > > The list goes on and on :) > > So the patch cleans all that up. It introduces simple and clean interface for > setting up TX filters (TUNSETTXFILTER + tun_filter spec) and does filtering > before enqueuing the packets. > > TX filtering is useful in the scenarios where TAP is part of a bridge, in > which case it gets all broadcast, multicast and potentially other packets when > the bridge is learning. So for example Ethernet tunnelling app may want to > setup TX filters to avoid tunnelling multicast traffic. QEMU and other > hypervisors can push RX filtering that is currently done in the guest into the > host context therefore saving wakeups and unnecessary data transfer. > > This is on top of the latest and greatest :). Assuming virt folks are ok with > the API this should go into 2.6.27. > > Signed-off-by: Max Krasnyansky <maxk@xxxxxxxxxxxx> > --- > drivers/net/tun.c | 316 +++++++++++++++++++++++------------------------- > include/linux/if_tun.h | 24 +++- > 2 files changed, 174 insertions(+), 166 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 2693f88..901551c 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -18,15 +18,11 @@ > /* > * Changes: > * > - * Brian Braunstein <linuxkernel@xxxxxxxxxxxx> 2007/03/23 > - * Fixed hw address handling. Now net_device.dev_addr is kept consistent > - * with tun.dev_addr when the address is set by this module. > - * > * Mike Kershaw <dragorn@xxxxxxxxxxxxxxxxxx> 2005/08/14 > * Add TUNSETLINK ioctl to set the link encapsulation > * > * Mark Smith <markzzzsmith@xxxxxxxxxxxx> > - * Use random_ether_addr() for tap MAC address. > + * Use random_ether_addr() for tap MAC address. > * > * Harald Roelle <harald.roelle@xxxxxxxxxx> 2004/04/20 > * Fixes in packet dropping, queue length setting and queue wakeup. > @@ -83,9 +79,16 @@ static int debug; > #define DBG1( a... ) > #endif > > +#define FLT_EXACT_COUNT 8 > +struct tap_filter { > + unsigned int count; /* Number of addrs. Zero means disabled */ > + u32 mask[2]; /* Mask of the hashed addrs */ > + unsigned char addr[FLT_EXACT_COUNT][ETH_ALEN]; > +}; > + > struct tun_struct { > struct list_head list; > - unsigned long flags; > + unsigned int flags; > int attached; > uid_t owner; > gid_t group; > @@ -94,19 +97,119 @@ struct tun_struct { > struct sk_buff_head readq; > > struct net_device *dev; > + struct fasync_struct *fasync; > > - struct fasync_struct *fasync; > - > - unsigned long if_flags; > - u8 dev_addr[ETH_ALEN]; > - u32 chr_filter[2]; > - u32 net_filter[2]; > + struct tap_filter txflt; > > #ifdef TUN_DEBUG > int debug; > #endif > }; > > +/* TAP filterting */ > +static void addr_hash_set(u32 *mask, const u8 *addr) > +{ > + int n = ether_crc(ETH_ALEN, addr) >> 26; > + mask[n >> 5] |= (1 << (n & 31)); > +} > + > +static unsigned int addr_hash_test(const u32 *mask, const u8 *addr) > +{ > + int n = ether_crc(ETH_ALEN, addr) >> 26; > + return mask[n >> 5] & (1 << (n & 31)); > +} > + > +static int update_filter(struct tap_filter *filter, void __user *arg) > +{ > + struct { u8 u[ETH_ALEN]; } *addr; > + struct tun_filter uf; > + int err, alen, n, nexact; > + > + if (copy_from_user(&uf, arg, sizeof(uf))) > + return -EFAULT; > + > + if (!uf.count) { > + /* Disabled */ > + filter->count = 0; > + return 0; > + } > + > + alen = ETH_ALEN * uf.count; > + addr = kmalloc(alen, GFP_KERNEL); > + if (!addr) > + return -ENOMEM; > + > + if (copy_from_user(addr, arg + sizeof(uf), alen)) { > + err = -EFAULT; > + goto done; > + } > + > + /* The filter is updated without holding any locks. Which is > + * perfectly safe. We disable it first and in the worst > + * case we'll accept a few undesired packets. */ > + filter->count = 0; > + wmb(); > + > + /* Use first set of addresses as an exact filter */ > + for (n = 0; n < uf.count && n < FLT_EXACT_COUNT; n++) > + memcpy(filter->addr[n], addr[n].u, ETH_ALEN); > + > + nexact = n; > + > + /* The rest is hashed */ > + memset(filter->mask, 0, sizeof(filter->mask)); > + for (; n < uf.count; n++) > + addr_hash_set(filter->mask, addr[n].u); > + > + /* For ALLMULTI just set the mask to all ones. > + * This overrides the mask populated above. */ > + if ((uf.flags & TUN_FLT_ALLMULTI)) > + memset(filter->mask, ~0, sizeof(filter->mask)); > + > + /* Now enable the filter */ > + wmb(); > + filter->count = nexact; > + > + /* Return the number of exact filters */ > + err = nexact; > + > +done: > + kfree(addr); > + return err; > +} > + > +/* Returns: 0 - drop, !=0 - accept */ > +static int run_filter(struct tap_filter *filter, const struct sk_buff *skb) > +{ > + /* Cannot use eth_hdr(skb) here because skb_mac_hdr() is incorrect > + * at this point. */ > + struct ethhdr *eh = (struct ethhdr *) skb->data; > + int i; > + > + /* Exact match */ > + for (i = 0; i < filter->count; i++) > + if (!compare_ether_addr(eh->h_dest, filter->addr[i])) > + return 1; > + > + /* Inexact match (multicast only) */ > + if (is_multicast_ether_addr(eh->h_dest)) > + return addr_hash_test(filter->mask, eh->h_dest); > + > + return 0; > +} > + > +/* > + * Checks whether the packet is accepted or not. > + * Returns: 0 - drop, !=0 - accept > + */ > +static int check_filter(struct tap_filter *filter, const struct sk_buff *skb) > +{ > + if (!filter->count) > + return 1; > + > + return run_filter(filter, skb); > +} > + > /* Network device part of the driver */ > > static unsigned int tun_net_id; > @@ -141,7 +244,12 @@ static int tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > if (!tun->attached) > goto drop; > > - /* Packet dropping */ > + /* Drop if the filter does not like it. > + * This is a noop if the filter is disabled. > + * Filter can be enabled only for the TAP devices. */ > + if (!check_filter(&tun->txflt, skb)) > + goto drop; > + > if (skb_queue_len(&tun->readq) >= dev->tx_queue_len) { > if (!(tun->flags & TUN_ONE_QUEUE)) { > /* Normal queueing mode. */ > @@ -158,7 +266,7 @@ static int tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > } > } > > - /* Queue packet */ > + /* Enqueue packet */ > skb_queue_tail(&tun->readq, skb); > dev->trans_start = jiffies; > > @@ -174,41 +282,14 @@ drop: > return 0; > } > > -/** Add the specified Ethernet address to this multicast filter. */ > -static void > -add_multi(u32* filter, const u8* addr) > -{ > - int bit_nr = ether_crc(ETH_ALEN, addr) >> 26; > - filter[bit_nr >> 5] |= 1 << (bit_nr & 31); > -} > - > -/** Remove the specified Ethernet addres from this multicast filter. */ > -static void > -del_multi(u32* filter, const u8* addr) > +static void tun_net_mclist(struct net_device *dev) > { > - int bit_nr = ether_crc(ETH_ALEN, addr) >> 26; > - filter[bit_nr >> 5] &= ~(1 << (bit_nr & 31)); > -} > - > -/** Update the list of multicast groups to which the network device belongs. > - * This list is used to filter packets being sent from the character device to > - * the network device. */ > -static void > -tun_net_mclist(struct net_device *dev) > -{ > - struct tun_struct *tun = netdev_priv(dev); > - const struct dev_mc_list *mclist; > - int i; > - DECLARE_MAC_BUF(mac); > - DBG(KERN_DEBUG "%s: tun_net_mclist: mc_count %d\n", > - dev->name, dev->mc_count); > - memset(tun->chr_filter, 0, sizeof tun->chr_filter); > - for (i = 0, mclist = dev->mc_list; i < dev->mc_count && mclist != NULL; > - i++, mclist = mclist->next) { > - add_multi(tun->net_filter, mclist->dmi_addr); > - DBG(KERN_DEBUG "%s: tun_net_mclist: %s\n", > - dev->name, print_mac(mac, mclist->dmi_addr)); > - } > + /* > + * This callback is supposed to deal with mc filter in > + * _rx_ path and has nothing to do with the _tx_ path. > + * In rx path we always accept everything userspace gives us. > + */ > + return; > } > > #define MIN_MTU 68 > @@ -244,13 +325,11 @@ static void tun_net_init(struct net_device *dev) > > case TUN_TAP_DEV: > /* Ethernet TAP Device */ > - dev->set_multicast_list = tun_net_mclist; > - > ether_setup(dev); > - dev->change_mtu = tun_net_change_mtu; > + dev->change_mtu = tun_net_change_mtu; > + dev->set_multicast_list = tun_net_mclist; > > - /* random address already created for us by tun_set_iff, use it */ > - memcpy(dev->dev_addr, tun->dev_addr, min(sizeof(tun->dev_addr), sizeof(dev->dev_addr)) ); > + random_ether_addr(dev->dev_addr); > > dev->tx_queue_len = TUN_READQ_SIZE; /* We prefer our own queue length */ > break; > @@ -486,7 +565,6 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv, > DECLARE_WAITQUEUE(wait, current); > struct sk_buff *skb; > ssize_t len, ret = 0; > - DECLARE_MAC_BUF(mac); > > if (!tun) > return -EBADFD; > @@ -499,10 +577,6 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv, > > add_wait_queue(&tun->read_wait, &wait); > while (len) { > - const u8 ones[ ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; > - u8 addr[ ETH_ALEN]; > - int bit_nr; > - > current->state = TASK_INTERRUPTIBLE; > > /* Read frames from the queue */ > @@ -522,36 +596,9 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv, > } > netif_wake_queue(tun->dev); > > - /** Decide whether to accept this packet. This code is designed to > - * behave identically to an Ethernet interface. Accept the packet if > - * - we are promiscuous. > - * - the packet is addressed to us. > - * - the packet is broadcast. > - * - the packet is multicast and > - * - we are multicast promiscous. > - * - we belong to the multicast group. > - */ > - skb_copy_from_linear_data(skb, addr, min_t(size_t, sizeof addr, > - skb->len)); > - bit_nr = ether_crc(sizeof addr, addr) >> 26; > - if ((tun->if_flags & IFF_PROMISC) || > - memcmp(addr, tun->dev_addr, sizeof addr) == 0 || > - memcmp(addr, ones, sizeof addr) == 0 || > - (((addr[0] == 1 && addr[1] == 0 && addr[2] == 0x5e) || > - (addr[0] == 0x33 && addr[1] == 0x33)) && > - ((tun->if_flags & IFF_ALLMULTI) || > - (tun->chr_filter[bit_nr >> 5] & (1 << (bit_nr & 31)))))) { > - DBG(KERN_DEBUG "%s: tun_chr_readv: accepted: %s\n", > - tun->dev->name, print_mac(mac, addr)); > - ret = tun_put_user(tun, skb, (struct iovec *) iv, len); > - kfree_skb(skb); > - break; > - } else { > - DBG(KERN_DEBUG "%s: tun_chr_readv: rejected: %s\n", > - tun->dev->name, print_mac(mac, addr)); > - kfree_skb(skb); > - continue; > - } > + ret = tun_put_user(tun, skb, (struct iovec *) iv, len); > + kfree_skb(skb); > + break; > } > > current->state = TASK_RUNNING; > @@ -647,12 +694,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) > tun = netdev_priv(dev); > tun->dev = dev; > tun->flags = flags; > - /* Be promiscuous by default to maintain previous behaviour. */ > - tun->if_flags = IFF_PROMISC; > - /* Generate random Ethernet address. */ > - *(__be16 *)tun->dev_addr = htons(0x00FF); > - get_random_bytes(tun->dev_addr + sizeof(u16), 4); > - memset(tun->chr_filter, 0, sizeof tun->chr_filter); > + tun->txflt.count = 0; > > tun_net_init(dev); > > @@ -751,6 +793,7 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file, > struct tun_struct *tun = file->private_data; > void __user* argp = (void __user*)arg; > struct ifreq ifr; > + int ret; > DECLARE_MAC_BUF(mac); > > if (cmd == TUNSETIFF || _IOC_TYPE(cmd) == 0x89) > @@ -826,9 +869,6 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file, > break; > > case TUNSETLINK: > - { > - int ret; > - > /* Only allow setting the type when the interface is down */ > rtnl_lock(); > if (tun->dev->flags & IFF_UP) { > @@ -842,94 +882,44 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file, > } > rtnl_unlock(); > return ret; > - } > > #ifdef TUN_DEBUG > case TUNSETDEBUG: > tun->debug = arg; > break; > #endif > - > case TUNSETOFFLOAD: > - { > - int ret; > rtnl_lock(); > ret = set_offload(tun->dev, arg); > rtnl_unlock(); > return ret; > - } > > - case SIOCGIFFLAGS: > - ifr.ifr_flags = tun->if_flags; > - if (copy_to_user( argp, &ifr, sizeof ifr)) > - return -EFAULT; > - return 0; > - > - case SIOCSIFFLAGS: > - /** Set the character device's interface flags. Currently only > - * IFF_PROMISC and IFF_ALLMULTI are used. */ > - tun->if_flags = ifr.ifr_flags; > - DBG(KERN_INFO "%s: interface flags 0x%lx\n", > - tun->dev->name, tun->if_flags); > - return 0; > + case TUNSETTXFILTER: > + /* Can be set only for TAPs */ > + if ((tun->flags & TUN_TYPE_MASK) != TUN_TAP_DEV) > + return -EINVAL; > + rtnl_lock(); > + ret = update_filter(&tun->txflt, (void *) __user arg); looks mostly OK, but stuff like the above should be (void __user *) arg Did you check this with sparse (Documentation/sparse.txt)? Jeff _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/virtualization