netdev - Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

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: <CAEh+42h79Sxc4JwzH_zMm+MZ-CmQWGdX1Qiz7epJmj2nCiAAVQ@mail.gmail.com>
Date:	Wed, 6 Jan 2016 16:18:30 -0800
From:	Jesse Gross <jesse@...nel.org>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 8/8] geneve: break dependency to network drivers

On Wed, Jan 6, 2016 at 3:39 PM, Hannes Frederic Sowa
<hannes@...essinduktion.org> wrote:
> Signed-off-by: Hannes Frederic Sowa <hannes@...essinduktion.org>
> ---
>  drivers/net/geneve.c | 30 +++++++++++++++++++++++++++---
>  include/net/geneve.h |  7 +++----
>  2 files changed, 30 insertions(+), 7 deletions(-)

Thanks a lot for going through all the drivers! You found more things
than I thought. I noticed just a couple new things with this version
but overall I think it is a good direction.

> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 24b077a32c1c9c..5b8d728b1204be 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1110,7 +1110,7 @@ static struct device_type geneve_type = {
>   * supply the listening GENEVE udp ports. Callers are expected
>   * to implement the ndo_add_geneve_port.
>   */
> -void geneve_get_rx_port(struct net_device *dev)
> +static void geneve_notify_refresh_netdev(struct net_device *dev)
>  {
>         struct net *net = dev_net(dev);
>         struct geneve_net *gn = net_generic(net, geneve_net_id);

Both this function and the corresponding one in VXLAN assume that the
add port NDO is implemented and blindly dereference the pointer. Now
that all protocols get refreshed at the same time, we should check
first. There's also a comment above the function that says this, which
we can remove now.

I also think we can update the comments in netdevice.h for the
VXLAN/Geneve add/remove NDOs to say that they are protected by RTNL.

> diff --git a/include/net/geneve.h b/include/net/geneve.h
> index e6c23dc765f7ec..7d52077b72faa3 100644
> --- a/include/net/geneve.h
> +++ b/include/net/geneve.h
> -#if IS_ENABLED(CONFIG_GENEVE)
> -void geneve_get_rx_port(struct net_device *netdev);
> -#else
>  static inline void geneve_get_rx_port(struct net_device *netdev)
>  {
> +       call_netdevice_notifiers(NETDEV_REFRESH_OFFLOADS, netdev);
>  }
> -#endif

I think we should merge vxlan_get_rx_port() and geneve_get_rx_port()
into a single generic function. For drivers that support both, they
now have two calls to get notified for all offloaded ports. This
actually can cause problems related to duplicates, similar to what you
feared before.
--
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