From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shaun Jackman Subject: Re: Multicast filtering for tun.c [PATCH] Date: Thu, 2 Dec 2004 09:28:36 -0800 Message-ID: <7f45d9390412020928f298944@mail.gmail.com> References: <7f45d9390411251715138b35d0@mail.gmail.com> <20041127011006.03232bb6.akpm@osdl.org> <7f45d939041201140329d0273f@mail.gmail.com> <20041201233400.45078efe.akpm@osdl.org> Reply-To: Shaun Jackman Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: To: netdev@oss.sgi.com In-Reply-To: <20041201233400.45078efe.akpm@osdl.org> Sender: netdev-bounce@oss.sgi.com Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org Thanks for your careful review, Andrew. I appreciate your help. > You're sure this shouldn't be using crc-ccitt? I used ether_crc because every driver in drivers/net except ppp_async.c does. Either would work as long as the chosen hash function is used consistently. > del_multi(u32 *filter, const u8 *addr) > > would be a more typical layout. I prefer the asterisk with the rest of the type information, but I know the latter is more typical. Fixed. > > + const u8 ones[ ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; > > The space after the `[' is gratuitous. > > This array will be allocated onthe stack and populated by hand each time > this function is called. It should be made static. > > > + u8 addr[ ETH_ALEN]; > > extraneous space. Fixed. > > + memcpy(addr, skb->data, min(sizeof addr, skb->len)); > > We normally put parentheses around the argument of the sizeof operator, for > no particular reason ;) > > > + if (copy_from_user(&ifr, argp, sizeof ifr)) > > Ditto I prefer not to add the extraneous punctuation. I find it makes it harder to read. > > + case SIOCGIFFLAGS: > > + ifr.ifr_flags = tun->if_flags; > > + if (copy_to_user( argp, &ifr, sizeof ifr)) > > extraneous space. > > > + if (copy_to_user( argp, &ifr, sizeof ifr)) > > ditto Fixed. > > + DBG(KERN_DEBUG "%s: add multi: %x:%x:%x:%x:%x:%x\n", > > + tun->dev->name, > > + (u8)ifr.ifr_hwaddr.sa_data[0], (u8)ifr.ifr_hwaddr.sa_data[1], > > + (u8)ifr.ifr_hwaddr.sa_data[2], (u8)ifr.ifr_hwaddr.sa_data[3], > > + (u8)ifr.ifr_hwaddr.sa_data[4], (u8)ifr.ifr_hwaddr.sa_data[5]); > > Why all the typecasts? [clip] > ditto. sa_data is signed for some reason, which causes the signed byte to be sign extended resulting in the mac address being printed as 0:c:6e:14:ffffffa0:5a, for example. Here's a short diff of the changes to my patch. The full patch follows. 27c27 < +add_multi(u32* filter, const u8* addr) --- > +add_multi(u32 *filter, const u8 *addr) 38c38 < +del_multi(u32* filter, const u8* addr) --- > +del_multi(u32 *filter, const u8 *addr) 71,72c71,72 < + const u8 ones[ ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; < + u8 addr[ ETH_ALEN]; --- > + static const u8 ones[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; > + u8 addr[ETH_ALEN]; 137c137 < + void __user* argp = (void __user*)arg; --- > + void __user *argp = (void __user*)arg; 168c168 < + if (copy_to_user( argp, &ifr, sizeof ifr)) --- > + if (copy_to_user(argp, &ifr, sizeof ifr)) 183c183 < + if (copy_to_user( argp, &ifr, sizeof ifr)) --- > + if (copy_to_user(argp, &ifr, sizeof ifr)) Cheers, Shaun 2004-11-25 Shaun Jackman * drivers/net/tun.c: Add multicast filtering for packets travelling from the network device to the character device. * include/linux/if_tun.h (tun_struct): Add interface flags, a hardware device addres, and a multicast filter. diff -ur linux-2.6.8.1.orig/drivers/net/tun.c linux-2.6.8.1/drivers/net/tun.c --- linux-2.6.8.1.orig/drivers/net/tun.c 2004-08-14 03:55:23.000000000 -0700 +++ linux-2.6.8.1/drivers/net/tun.c 2004-11-25 17:00:22.000000000 -0800 @@ -41,6 +41,7 @@ #include #include #include +#include #include #include @@ -104,11 +105,42 @@ return 0; } -static void tun_net_mclist(struct net_device *dev) +/** Add the specified Ethernet address to this multicast filter. */ +static void +add_multi(u32 *filter, const u8 *addr) { - /* Nothing to do for multicast filters. - * We always accept all frames. */ - return; + 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) +{ + 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; + 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: %x:%x:%x:%x:%x:%x\n", + dev->name, + mclist->dmi_addr[0], mclist->dmi_addr[1], mclist->dmi_addr[2], + mclist->dmi_addr[3], mclist->dmi_addr[4], mclist->dmi_addr[5]); + } } static struct net_device_stats *tun_net_stats(struct net_device *dev) @@ -301,6 +333,10 @@ add_wait_queue(&tun->read_wait, &wait); while (len) { + static 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 */ @@ -320,10 +356,37 @@ } netif_start_queue(tun->dev); - ret = tun_put_user(tun, skb, (struct iovec *) iv, len); - - kfree_skb(skb); - break; + /** 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. + */ + memcpy(addr, skb->data, min(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: %x:%x:%x:%x:%x:%x\n", + tun->dev->name, addr[0], addr[1], addr[2], + addr[3], addr[4], addr[5]); + ret = tun_put_user(tun, skb, (struct iovec *) iv, len); + kfree_skb(skb); + break; + } else { + DBG(KERN_DEBUG "%s: tun_chr_readv: rejected: %x:%x:%x:%x:%x:%x\n", + tun->dev->name, addr[0], addr[1], addr[2], + addr[3], addr[4], addr[5]); + kfree_skb(skb); + continue; + } } current->state = TASK_RUNNING; @@ -417,6 +480,12 @@ 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. */ + *(u16 *)tun->dev_addr = htons(0x00FF); + get_random_bytes(tun->dev_addr + sizeof(u16), 4); + memset(tun->chr_filter, 0, sizeof tun->chr_filter); tun_net_init(dev); @@ -457,13 +526,16 @@ unsigned int cmd, unsigned long arg) { struct tun_struct *tun = file->private_data; + void __user *argp = (void __user*)arg; + struct ifreq ifr; + + if (cmd == TUNSETIFF || _IOC_TYPE(cmd) == 0x89) + if (copy_from_user(&ifr, argp, sizeof ifr)) + return -EFAULT; if (cmd == TUNSETIFF && !tun) { - struct ifreq ifr; int err; - if (copy_from_user(&ifr, (void __user *)arg, sizeof(ifr))) - return -EFAULT; ifr.ifr_name[IFNAMSIZ-1] = '\0'; rtnl_lock(); @@ -473,7 +545,7 @@ if (err) return err; - if (copy_to_user((void __user *)arg, &ifr, sizeof(ifr))) + if (copy_to_user(argp, &ifr, sizeof(ifr))) return -EFAULT; return 0; } @@ -519,6 +591,61 @@ break; #endif + 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 SIOCGIFHWADDR: + memcpy(ifr.ifr_hwaddr.sa_data, tun->dev_addr, + min(sizeof ifr.ifr_hwaddr.sa_data, sizeof tun->dev_addr)); + if (copy_to_user(argp, &ifr, sizeof ifr)) + return -EFAULT; + return 0; + + case SIOCSIFHWADDR: + /** Set the character device's hardware address. This is used when + * filtering packets being sent from the network device to the character + * device. */ + memcpy(tun->dev_addr, ifr.ifr_hwaddr.sa_data, + min(sizeof ifr.ifr_hwaddr.sa_data, sizeof tun->dev_addr)); + DBG(KERN_DEBUG "%s: set hardware address: %x:%x:%x:%x:%x:%x\n", + tun->dev->name, + tun->dev_addr[0], tun->dev_addr[1], tun->dev_addr[2], + tun->dev_addr[3], tun->dev_addr[4], tun->dev_addr[5]); + return 0; + + case SIOCADDMULTI: + /** Add the specified group to the character device's multicast filter + * list. */ + add_multi(tun->chr_filter, ifr.ifr_hwaddr.sa_data); + DBG(KERN_DEBUG "%s: add multi: %x:%x:%x:%x:%x:%x\n", + tun->dev->name, + (u8)ifr.ifr_hwaddr.sa_data[0], (u8)ifr.ifr_hwaddr.sa_data[1], + (u8)ifr.ifr_hwaddr.sa_data[2], (u8)ifr.ifr_hwaddr.sa_data[3], + (u8)ifr.ifr_hwaddr.sa_data[4], (u8)ifr.ifr_hwaddr.sa_data[5]); + return 0; + + case SIOCDELMULTI: + /** Remove the specified group from the character device's multicast + * filter list. */ + del_multi(tun->chr_filter, ifr.ifr_hwaddr.sa_data); + DBG(KERN_DEBUG "%s: del multi: %x:%x:%x:%x:%x:%x\n", + tun->dev->name, + (u8)ifr.ifr_hwaddr.sa_data[0], (u8)ifr.ifr_hwaddr.sa_data[1], + (u8)ifr.ifr_hwaddr.sa_data[2], (u8)ifr.ifr_hwaddr.sa_data[3], + (u8)ifr.ifr_hwaddr.sa_data[4], (u8)ifr.ifr_hwaddr.sa_data[5]); + return 0; + default: return -EINVAL; }; diff -ur linux-2.6.8.1.orig/include/linux/if_tun.h linux-2.6.8.1/include/linux/if_tun.h --- linux-2.6.8.1.orig/include/linux/if_tun.h 2004-08-14 03:55:09.000000000 -0700 +++ linux-2.6.8.1/include/linux/if_tun.h 2004-11-25 16:47:31.000000000 -0800 @@ -45,6 +45,11 @@ struct fasync_struct *fasync; + unsigned long if_flags; + u8 dev_addr[ETH_ALEN]; + u32 chr_filter[2]; + u32 net_filter[2]; + #ifdef TUN_DEBUG int debug; #endif