netdev - Re: [PATCH 5/5] geneve: add initial netdev driver for GENEVE tunnels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEP_g=-=GEZn+Q48=OdWVNHSONf9EVfcz3PGE2qR4ynWXbsHeQ@mail.gmail.com>
Date:	Fri, 8 May 2015 16:19:28 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	"John W. Linville" <linville@...driver.com>
Cc:	netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Andy Zhou <azhou@...ira.com>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Alexander Duyck <alexander.h.duyck@...hat.com>
Subject: Re: [PATCH 5/5] geneve: add initial netdev driver for GENEVE tunnels

On Fri, May 8, 2015 at 10:20 AM, John W. Linville
<linville@...driver.com> wrote:
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> new file mode 100644
> index 000000000000..102030de1d45
> --- /dev/null
> +++ b/drivers/net/geneve.c
> +/* geneve receive/decap routine */
> +static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
[...]
> +       /* Find the device for this VNI */
> +       hash = geneve_net_vni_hash(gnvh->vni);
> +       vni_list_head = &gn->vni_list[hash];
> +       hlist_for_each_entry_rcu(dummy, vni_list_head, hlist) {
> +               if (!memcmp(gnvh->vni, dummy->vni, sizeof(dummy->vni)) &&
> +                   iph->saddr == dummy->remote.sin_addr.s_addr)
> +                       geneve = dummy;

I guess we might as well break out of the loop at this point rather
than keep searching.

> +static int geneve_newlink(struct net *net, struct net_device *dev,
> +                        struct nlattr *tb[], struct nlattr *data[])
> +{
[...]
> +       if (!data[IFLA_GENEVE_ID])
> +               return -EINVAL;

Should we enforce that IFLA_GENEVE_REMOTE is present? Otherwise, it's
not clear what we would do without it.

[...]
> +       list_add(&geneve->next, &gn->geneve_list);
> +
> +       geneve_net_vni_add(gn, hash, geneve);

The locking seems a bit inconsistent for these two pieces - they are
accessed in the same places but one has a special lock and the other
doesn't. I think the answer is that neither needs a lock because they
are both protected by RTNL but it made me pause.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html