On Thu, Nov 22, 2012 at 7:56 AM, Pravin B Shelar <pshe...@nicira.com> wrote:
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 0d4eecd..829fe3d 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> -static int ipgre_tunnel_bind_dev(struct net_device *dev)
> +static int ipgre_get_ioctl_param(struct net_device *dev, struct ifreq *ifr,
[...]
> + if (copy_from_user(p, ifr->ifr_ifru.ifru_data, sizeof(*p)))
> + return -EFAULT;
Can we keep this in common code?
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> new file mode 100644
> index 0000000..e5e2fde
> --- /dev/null
> +++ b/net/ipv4/ip_tunnel.c
One general thing that concerns me a little bit is the ipt_ prefix
sounds very similar to IPtables.
> +#define HASH(addr) ((__force u32)addr^((__force u32)addr>>4))
I think now that the hash table size has been expanded it probably
makes sense to use a real function.
> +static unsigned int ip_tunnel_hash(struct ip_tunnel_net *itn,
> + __be32 key, __be32 remote)
> +{
> + unsigned int h = HASH(key);
> +
> + if (!(itn->ops->flags & HASH_ON_KEY))
> + h ^= HASH(remote);
In the cases where we're wildcarding the remote, don't we need to not
include it in the hash as well?
> +static struct ip_tunnel *__tunnel_lookup(struct ip_tunnel_net *itn,
> + int link, __be16 flags,
> + __be32 remote, __be32 local,
> + __be32 key, int hash,
> + struct ip_tunnel **cand)
> +{
> + struct ip_tunnel *t;
> +
> + for_each_ip_tunnel_rcu(t, itn->tunnels[hash]) {
> + if (local != t->parms.iph.saddr ||
> + remote != t->parms.iph.daddr ||
I don't think that it's sufficient to just set our lookup parameter to
zero and then continue to do the comparison since the tunnel might
have a real value even if we are wildcarding it now.
> + if (!remote &&
> + !(ipv4_is_multicast(local) && local ==
> t->parms.iph.daddr))
> + continue;
> +
> + if (!ip_tunnel_key_match(&t->parms, flags, key))
> + continue;
> +
> + if (t->parms.link == link)
> + return t;
> + else
> + *cand = t;
Since we're going from more specific lookups to less, won't this
result in not necessarily getting the best candidate?
> +struct ip_tunnel *ip_tunnel_lookup_key(struct ip_tunnel_net *itn, __be32 key)
> +{
> + struct ip_tunnel *t;
> + unsigned int h = HASH(key);
> +
> + h = jhash_1word(h, ip_tunnel_salt) & (IPT_HASH_SIZE-1);
> + for_each_ip_tunnel_rcu(t, itn->tunnels[h]) {
> + if (key == t->parms.i_key)
> + return t;
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL(ip_tunnel_lookup_key);
Is this function used anywhere?
> +void ip_tunnel_link(struct ip_tunnel_net *itn, struct ip_tunnel *t)
> +{
> + struct ip_tunnel __rcu **tp = ip_bucket(itn, &t->parms);
> +
> + rcu_assign_pointer(t->next, rtnl_dereference(*tp));
> + rcu_assign_pointer(*tp, t);
> +}
> +EXPORT_SYMBOL(ip_tunnel_link);
> +
> +void ip_tunnel_unlink(struct ip_tunnel_net *itn, struct ip_tunnel *t)
> +{
> + struct ip_tunnel __rcu **tp;
> + struct ip_tunnel *iter;
> +
> + for (tp = ip_bucket(itn, &t->parms);
> + (iter = rtnl_dereference(*tp)) != NULL;
> + tp = &iter->next) {
> + if (t == iter) {
> + rcu_assign_pointer(*tp, t->next);
> + break;
> + }
> + }
> +}
> +EXPORT_SYMBOL(ip_tunnel_unlink);
Do these two functions need to be exported?
> +struct ip_tunnel *ip_tunnel_find(struct ip_tunnel_net *itn,
> + struct ip_tunnel_parm *parms,
> + int type)
> +{
> + __be32 remote = parms->iph.daddr;
> + __be32 local = parms->iph.saddr;
> + __be32 key = parms->i_key;
> + int link = parms->link;
> + struct ip_tunnel *t;
> + struct ip_tunnel __rcu **tp;
> +
> + for (tp = ip_bucket(itn, parms);
> + (t = rtnl_dereference(*tp)) != NULL;
> + tp = &t->next)
> + if (local == t->parms.iph.saddr &&
> + remote == t->parms.iph.daddr &&
> + key == t->parms.i_key &&
> + link == t->parms.link &&
> + type == t->dev->type)
Shouldn't all tunnels within a given ip_tunnel_net have the same type?
> + break;
> +
> + return t;
> +}
> +EXPORT_SYMBOL(ip_tunnel_find);
Does this need to be exported?
> +static struct net_device *__ip_tunnel_create(struct net *net,
> + const struct rtnl_link_ops *ops,
> + struct ip_tunnel_parm *parms)
> +{
> + int err;
> + struct ip_tunnel *tunnel;
> + struct net_device *dev;
> + char name[IFNAMSIZ];
> +
> + if (parms->name[0])
> + strlcpy(name, parms->name, IFNAMSIZ);
> + else {
> + strlcpy(name, ops->kind, IFNAMSIZ);
> + strncat(name, "%d", IFNAMSIZ);
I don't think the strncat() call is really safe since it just limits
the string being copied to IFNAMSIZ (which it obvious is) but doesn't
take into account either the length of ops->kind or the NULL
terminator. This is safe for the types that we have here since they
are short but it seems a little dangerous (or at the very least the
code above gives a false sense of security).
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
