Message-ID: <4508315A.9070809@trash.net>
Date:	Wed, 13 Sep 2006 18:27:06 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Joerg Roedel <joro-lkml@...g.org>
CC:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	davem@...emloft.net
Subject: Re: [PATCH] EtherIP tunnel driver (RFC 3378)

There are lots of whitespace errors (trailing whitespace, whitespace
following opening parens, no whitespace after comma, ...) which I'm
going to ignore below, please fix them anyway.

Joerg Roedel wrote:
> diff -uprN -X linux-2.6.17.13/Documentation/dontdiff linux-2.6.17.13-vanilla/net/ipv4/etherip.c linux-2.6.17.13/net/ipv4/etherip.c
> --- linux-2.6.17.13-vanilla/net/ipv4/etherip.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.17.13/net/ipv4/etherip.c	2006-09-11 21:59:40.000000000 +0200
> @@ -0,0 +1,546 @@
> +/*
> + * etherip.c: Ethernet over IPv4 tunnel driver (according to RFC3378)
> + *
> + * This driver could be used to tunnel Ethernet packets through IPv4
> + * networks. This is especially usefull together with the bridging
> + * code in Linux.
> + *
> + * This code was written with an eye on the IPIP driver in linux from
> + * Sam Lantinga. Thanks for the great work.
> + *
> + *      This program is free software; you can redistribute it and/or
> + *      modify it under the terms of the GNU General Public License
> + *      version 2 (no later version) as published by the
> + *      Free Software Foundation.
> + *
> + */
> +
> +#include <linux/capability.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/mutex.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/skbuff.h>
> +#include <linux/ip.h>
> +#include <linux/if_tunnel.h>
> +#include <linux/list.h>
> +#include <linux/string.h>
> +#include <linux/netfilter_ipv4.h>
> +#include <net/ip.h>
> +#include <net/protocol.h>
> +#include <net/route.h>
> +#include <net/ipip.h>
> +#include <net/xfrm.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Joerg Roedel <joerg@...g.org>");
> +MODULE_DESCRIPTION("Ethernet over IPv4 tunnel driver");
> +
> +/* 
> + * These 2 defines are taken from ipip.c - if it's good enough for them
> + * it's good enough for me.
> + */
> +#define HASH_SIZE        16
> +#define HASH(addr)       ((addr^(addr>>4))&0xF)
> +
> +#define ETHERIP_HEADER   ((u16)0x0300)
> +#define ETHERIP_HLEN     2
> +
> +#define BANNER1 "etherip: Ethernet over IPv4 tunneling driver\n"
> +#define BANNER2 "etherip: (C) 2006 by Joerg Roedel <joerg@...g.org>\n"
> +
> +struct etherip_tunnel {
> +	struct list_head list;
> +	struct net_device *dev;
> +	struct net_device_stats stats;
> +	struct ip_tunnel_parm parms;
> +	unsigned int recursion;
> +};
> +
> +static struct net_device *etherip_tunnel_dev;
> +static struct list_head tunnels[HASH_SIZE];
> +
> +static DEFINE_RWLOCK(etherip_lock);
> +
> +static void etherip_tunnel_setup(struct net_device *dev);
> +
> +/* add a tunnel to the hash */
> +static void etherip_tunnel_add(struct etherip_tunnel *tun)
> +{
> +	unsigned h = HASH(tun->parms.iph.daddr);
> +	list_add_tail(&tun->list, &tunnels[h]);
> +}
> +
> +/* delete a tunnel from the hash*/
> +static void etherip_tunnel_del(struct etherip_tunnel *tun)
> +{
> +	list_del(&tun->list);
> +}
> +
> +/* find a tunnel in the hash by parameters from userspace */
> +static struct etherip_tunnel* etherip_tunnel_find(struct ip_tunnel_parm *p)
> +{
> +	struct list_head *ptr;
> +	struct etherip_tunnel *ret;
> +	unsigned h = HASH(p->iph.daddr);
> +
> +	list_for_each(ptr, &tunnels[h]) {
> +		ret = list_entry(ptr, struct etherip_tunnel, list);

Again, please use list_for_each_entry

> +		if (ret->parms.iph.daddr == p->iph.daddr) {
> +			return ret;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +/* find a tunnel by its destination address */
> +static struct etherip_tunnel* etherip_tunnel_locate(u32 remote)
> +{
> +	struct list_head *ptr;
> +	struct etherip_tunnel *ret;
> +	unsigned h = HASH(remote);
> +
> +	list_for_each(ptr, &tunnels[h]) {
> +		ret = list_entry(ptr, struct etherip_tunnel, list);
> +		if (ret->parms.iph.daddr == remote) {
> +			return ret;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +/* netdevice start function */
> +static int etherip_tunnel_open(struct net_device *dev)
> +{
> +	netif_start_queue(dev);
> +	return 0;
> +}
> +
> +/* netdevice stop function */
> +static int etherip_tunnel_stop(struct net_device *dev)
> +{
> +	netif_stop_queue(dev);
> +	return 0;
> +}
> +
> +/* netdevice hard_start_xmit function
> + * it gets an Ethernet packet in skb and encapsulates it in another IP
> + * packet */
> +static int etherip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct etherip_tunnel *tunnel = netdev_priv(dev);
> +	struct rtable *rt;
> +	struct iphdr *iph;
> +	struct flowi fl;
> +	struct net_device *tdev;
> +	int max_headroom;
> +	struct net_device_stats *stats = &tunnel->stats;
> +
> +	if (tunnel->recursion++) {
> +		tunnel->stats.collisions++;
> +		goto tx_error;
> +	}
> +
> +	fl.oif = fl.iif      = 0;
> +	fl.proto             = IPPROTO_ETHERIP;
> +	fl.nl_u.ip4_u.daddr  = tunnel->parms.iph.daddr;
> +	fl.nl_u.ip4_u.saddr  = tunnel->parms.iph.saddr;
> +	fl.nl_u.ip4_u.fwmark = 0;
> +	fl.nl_u.ip4_u.tos    = 0;

This still leaves the entire uli_u part as well as flags uninitialized.
Just use memset or initialize the entire structure at once. fl.oif
should be set to parms.link.

> +
> +	if (ip_route_output_key(&rt, &fl)) {
> +		tunnel->stats.tx_carrier_errors++;
> +		goto tx_error_icmp;
> +	}
> +
> +	tdev = rt->u.dst.dev;
> +	if (tdev == dev) {
> +		ip_rt_put(rt);
> +		tunnel->stats.collisions++;
> +		goto tx_error;
> +	}
> +
> +	max_headroom = (LL_RESERVED_SPACE(tdev)+sizeof(struct iphdr)
> +			+ ETHERIP_HLEN);
> +
> +	if (skb_headroom(skb) < max_headroom || skb_cloned(skb)
> +			|| skb_shared(skb)) {
> +		struct sk_buff *n_skb = skb_realloc_headroom(skb,max_headroom);
> +		if (!n_skb) {
> +			ip_rt_put(rt);
> +			dev_kfree_skb(skb);
> +			tunnel->stats.tx_dropped++;
> +			return 0;
> +		}
> +		if (skb->sk)
> +			skb_set_owner_w(n_skb, skb->sk);
> +		dev_kfree_skb(skb);
> +		skb = n_skb;
> +	}
> +
> +	skb->h.raw = skb->nh.raw;
> +	skb->nh.raw = skb_push(skb, sizeof(struct iphdr)+ETHERIP_HLEN);
> +	memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
> +	IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED |
> +			IPSKB_REROUTED); 
> +	dst_release(skb->dst);

Still no propagation of the pmtu to the original dst_entry.

> +	skb->dst = &rt->u.dst;
> +
> +	/* Build the IP header for the outgoing packet
> +	 *
> +	 * Note: This driver never sets the DF flag on outgoing packets
> +	 *       to ensure that the tunnel provides the full Ethernet MTU.
> +	 *       This behavior guarantees that protocols can be
> +	 *       encapsulated within the Ethernet packet which do not
> +	 *       know the concept of a path MTU
> +	 */
> +	iph = skb->nh.iph;
> +	iph->version = 4;
> +	iph->ihl = sizeof(struct iphdr)>>2;
> +	iph->frag_off = 0;
> +	iph->protocol = IPPROTO_ETHERIP;
> +	iph->tos = 0;
> +	iph->daddr = rt->rt_dst;
> +	iph->saddr = rt->rt_src;
> +	iph->ttl = tunnel->parms.iph.ttl;
> +	if (iph->ttl == 0)
> +		iph->ttl = dst_metric(&rt->u.dst, RTAX_HOPLIMIT);
> +
> +	/* add the 16bit etherip header after the ip header */
> +	*((u16*)(skb->nh.raw + sizeof(struct iphdr))) = ntohs(ETHERIP_HEADER);
> +	nf_reset(skb);
> +	IPTUNNEL_XMIT();
> +	tunnel->dev->trans_start = jiffies;
> +	tunnel->recursion--;
> +
> +	return 0;
> +
> +tx_error_icmp:
> +	dst_link_failure(skb);
> +
> +tx_error:
> +	tunnel->stats.tx_errors++;
> +	dev_kfree_skb(skb);
> +	tunnel->recursion--;
> +	return 0;
> +}
> +
> +/* get statistics callback */
> +static struct net_device_stats *etherip_tunnel_stats(struct net_device *dev)
> +{
> +	struct etherip_tunnel *ethip = netdev_priv(dev);
> +	return &ethip->stats;
> +}
> +
> +/* checks parameters the driver gets from userspace */
> +static int etherip_param_check(struct ip_tunnel_parm *p)
> +{
> +	if ((p->iph.version != 4)
> +		|| (p->iph.protocol != IPPROTO_ETHERIP)
> +		|| (p->iph.ihl != 5)
> +		|| (p->iph.daddr == INADDR_ANY)
> +		|| MULTICAST(p->iph.daddr))
> +		return -EINVAL;

Lots of unnecessary parens (here and elsewhere). It also becomes more
readable IMO if you put the || at the end of the line and align the
conditions:

if (p->iph.version != 4 ||
    p->iph.protocol != IPPROTO_ETHERIP ||
    p->iph.ihl != 4 ||
...

Well, thats just my taste, but a single tab is in my opinion the
worst of all choices since it fails to make the distinction between
the condition and the following expression visible.

> +
> +	return 0;
> +}
> +
> +/* central ioctl function for all netdevices this driver manages
> + * it allows to create, delete, modify a tunnel and fetch tunnel
> + * information */
> +static int etherip_tunnel_ioctl(struct net_device *dev, struct ifreq *ifr,
> +		int cmd)
> +{
> +	int err = 0;
> +	struct ip_tunnel_parm p;
> +	struct net_device *new_dev;
> +	char *dev_name;
> +	struct etherip_tunnel *t;
> +
> +
> +	switch (cmd) {
> +	case SIOCGETTUNNEL:
> +		err = -EINVAL;
> +		if (dev == etherip_tunnel_dev)
> +			goto out;
> +		t = netdev_priv(dev);
> +		if (copy_to_user(ifr->ifr_ifru.ifru_data, &t->parms,
> +				sizeof(t->parms)))
> +			err = -EFAULT;

break

> +		err = 0;
> +		break;
> +	case SIOCADDTUNNEL:
> +		err = -EINVAL;
> +		if (dev != etherip_tunnel_dev)
> +			goto out;
> +
> +	case SIOCCHGTUNNEL:
> +		err = -EPERM;
> +		if (!capable(CAP_NET_ADMIN))
> +			goto out;
> +
> +		err = -EFAULT;
> +		if (copy_from_user(&p, ifr->ifr_ifru.ifru_data,
> +					sizeof(p)))
> +			goto out;
> +
> +		if ((err = etherip_param_check(&p)) < 0)
> +			goto out;
> +
> +		t = etherip_tunnel_find(&p);
> +
> +		err = -EEXIST;
> +		if ((t != NULL) && (t->dev != dev))
> +			goto out;
> +
> +		if (cmd == SIOCADDTUNNEL) {
> +
> +			p.name[IFNAMSIZ-1] = 0;
> +			dev_name = p.name;
> +			if (dev_name[0] == 0)
> +				dev_name = "ethip%d";
> +
> +			err = -ENOMEM;
> +			new_dev = alloc_netdev(
> +					sizeof(struct etherip_tunnel),
> +					dev_name,
> +					etherip_tunnel_setup);
> +
> +			if (new_dev == NULL)
> +				goto out;
> +				
> +			if (strchr(new_dev->name, '%')) {
> +				err = dev_alloc_name( new_dev, new_dev->name);
> +				if (err < 0)
> +					goto add_err;
> +			}
> +			
> +			t = netdev_priv(new_dev);
> +			t->dev = new_dev;
> +			strncpy(p.name, new_dev->name, IFNAMSIZ);
> +			memcpy(&(t->parms), &p, sizeof(p));
> +				
> +			err = register_netdevice(new_dev);
> +			if (err < 0)
> +				goto add_err;
> +
> +			err = -EFAULT;
> +			if (copy_to_user(ifr->ifr_ifru.ifru_data, &p,
> +						sizeof(p)))
> +				goto add_err;

You can't just free the device once its registered.

> +			
> +			write_lock(&etherip_lock);

Needs bh protection.

> +			etherip_tunnel_add(t);
> +			write_unlock(&etherip_lock);
> +
> +		} else {
> +			err = -EINVAL;
> +			if ((t = netdev_priv(dev)) == NULL)
> +				goto out;
> +			if (dev == etherip_tunnel_dev)
> +				goto out;
> +			write_lock(&etherip_lock);
> +			memcpy(&(t->parms), &p, sizeof(p));
> +			write_unlock(&etherip_lock);
> +		}
> +
> +		err = 0;
> +		break;
> +add_err:
> +		free_netdev(new_dev);
> +		goto out;
> +
> +	case SIOCDELTUNNEL:
> +		err = -EPERM;
> +		if (!capable(CAP_NET_ADMIN))
> +			goto out;
> +
> +		err = -EINVAL;
> +		if (dev == etherip_tunnel_dev)
> +			goto out;
> +
> +		t = netdev_priv(dev);
> +			
> +		write_lock(&etherip_lock);
> +		etherip_tunnel_del(t);
> +		write_unlock(&etherip_lock);
> +
> +		unregister_netdevice(t->dev);
> +		err = 0;
> +
> +		break;
> +	default:
> +		err = -EINVAL;
> +	}
> +
> +out:
> +	return err;
> +}
> +
> +/* device init function - called via register_netdevice
> + * The tunnel is registered as an Ethernet device. This allows
> + * the tunnel to be added to a bridge */
> +static void etherip_tunnel_setup(struct net_device *dev)
> +{
> +	SET_MODULE_OWNER(dev);
> +	dev->open = etherip_tunnel_open;
> +	dev->hard_start_xmit = etherip_tunnel_xmit;
> +	dev->stop = etherip_tunnel_stop;
> +	dev->get_stats = etherip_tunnel_stats;
> +	dev->do_ioctl = etherip_tunnel_ioctl;
> +	dev->destructor = free_netdev;
> +
> +	ether_setup(dev);
> +	dev->tx_queue_len = 0;
> +	random_ether_addr(dev->dev_addr);
> +}
> +
> +/* receive function for EtherIP packets
> + * Does some basic checks on the MAC addresses and
> + * interface modes */
> +static int etherip_rcv(struct sk_buff *skb)
> +{
> +	struct iphdr *iph;
> +	struct ethhdr *ehdr;
> +	struct etherip_tunnel *tunnel;
> +	struct net_device *dev;
> +
> +	iph = skb->nh.iph;
> +
> +	read_lock(&etherip_lock);
> +	tunnel = etherip_tunnel_locate(iph->saddr);
> +	if (tunnel == NULL)
> +		goto drop;
> +
> +	dev = tunnel->dev;
> +	secpath_reset(skb);
> +	memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options));
> +	skb_pull(skb, (skb->nh.raw - skb->data)
> +			+ sizeof(struct iphdr) + ETHERIP_HLEN);
> +	ehdr = (struct ethhdr*)skb->data;
> +	skb->dev = dev;
> +	skb->pkt_type = PACKET_HOST;
> +	skb->protocol = eth_type_trans(skb, tunnel->dev);
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	dst_release(skb->dst);
> +	skb->dst = NULL;
> +
> +	/* do some checks */
> +	if (skb->pkt_type == PACKET_HOST || skb->pkt_type == PACKET_BROADCAST)
> +		goto accept;
> +
> +	if (skb->pkt_type == PACKET_MULTICAST &&
> +			(dev->mc_count > 0 || dev->flags & IFF_ALLMULTI))
> +		goto accept;
> +	
> +	if (skb->pkt_type == PACKET_OTHERHOST && dev->flags & IFF_PROMISC)
> +		goto accept;
> +
> +	goto drop;
> +
> +accept:
> +	tunnel->dev->last_rx = jiffies;
> +	tunnel->stats.rx_packets++;
> +	tunnel->stats.rx_bytes += skb->len;
> +	nf_reset(skb);
> +	netif_rx(skb);
> +	read_unlock(&etherip_lock);
> +	return 0;
> +
> +drop:
> +	read_unlock(&etherip_lock);
> +	kfree_skb(skb);
> +	return 0;
> +}
> +
> +static struct net_protocol etherip_protocol = {
> +	.handler      = etherip_rcv,
> +	.err_handler  = 0,
> +	.no_policy    = 1,

This is wrong, you don't do any policy checks in your code.

> +};
> +
> +/* module init function
> + * initializes the EtherIP protocol (97) and registers the initial
> + * device */
> +static int __init etherip_init(void)
> +{
> +	int err, i;
> +	struct etherip_tunnel *p;
> +
> +	printk(KERN_INFO BANNER1);
> +	printk(KERN_INFO BANNER2);
> +
> +	for (i=0;i<HASH_SIZE;++i)
> +		INIT_LIST_HEAD(&tunnels[i]);
> +
> +	if (inet_add_protocol(&etherip_protocol, IPPROTO_ETHERIP)) {
> +		printk(KERN_ERR "etherip: can't add protocol\n");
> +		return -EAGAIN;
> +	}
> +
> +	etherip_tunnel_dev = alloc_netdev(sizeof(struct etherip_tunnel),
> +			"ethip0",
> +			etherip_tunnel_setup);
> +	
> +	if (!etherip_tunnel_dev) {
> +		err = -ENOMEM;
> +		goto err2;
> +	}
> +
> +	p = netdev_priv(etherip_tunnel_dev);
> +	p->dev = etherip_tunnel_dev;
> +
> +	if ((err = register_netdev(etherip_tunnel_dev)))
> +		goto err1;
> +
> +out:
> +	return err;
> +err1:
> +	free_netdev(etherip_tunnel_dev);
> +err2:
> +	inet_del_protocol(&etherip_protocol, IPPROTO_ETHERIP);
> +	goto out;
> +}
> +
> +/* destroy all tunnels */
> +static void __exit etherip_destroy_tunnels(void)
> +{
> +	int i;
> +	struct list_head *ptr;
> +	struct etherip_tunnel *tun;
> +	
> +	for (i=0;i<HASH_SIZE;++i) {
> +		/*
> +		ptr = tunnels[i].next;
> +		while (ptr != &(tunnels[i])) {
> +			ret = list_entry(ptr, struct etherip_tunnel, list);
> +			ptr = ptr->next;
> +			unregister_netdevice(ret->dev);
> +		}*/
> +		list_for_each(ptr, &tunnels[i]) {
> +			tun = list_entry(ptr, struct etherip_tunnel, list);
> +			ptr = ptr->prev;
> +			etherip_tunnel_del(tun);
> +			unregister_netdevice(tun->dev);
> +		}
> +	}
> +}
> +
> +/* module cleanup function */
> +static void __exit etherip_exit(void)
> +{
> +	rtnl_lock();
> +	etherip_destroy_tunnels();
> +	unregister_netdevice(etherip_tunnel_dev);
> +	rtnl_unlock();
> +	if (inet_del_protocol(&etherip_protocol, IPPROTO_ETHERIP))
> +		printk(KERN_ERR "etherip: can't remove protocol\n");
> +}
> +
> +module_init(etherip_init);
> +module_exit(etherip_exit);


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists