| 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
| ||
|
Message-ID: <5653FEEF.6010005@gmail.com>
Date: Mon, 23 Nov 2015 22:08:47 -0800
From: Alexander Duyck <alexander.duyck@...il.com>
To: Anjali Singhai Jain <anjali.singhai@...el.com>,
netdev@...r.kernel.org
Cc: jesse@...nel.org, Kiran Patil <kiran.patil@...el.com>
Subject: Re: [PATCH v1 2/6] net: Add a generic udp_offload_get_port function
On 11/23/2015 01:02 PM, Anjali Singhai Jain wrote:
> The new function udp_offload_get_port replaces vxlan_get_rx_port().
> This is a generic function that will help replay all udp tunnel ports
> irrespective of tunnel type.
> This way when new udp tunnels get added this function need not change.
>
> Note: Drivers besides i40e are compile tested with this change.
>
> Signed-off-by: Anjali Singhai Jain <anjali.singhai@...el.com>
> Signed-off-by: Kiran Patil <kiran.patil@...el.com>
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 5 ++--
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 +++-
> drivers/net/ethernet/emulex/benet/be_main.c | 4 +++-
> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 3 ++-
> drivers/net/ethernet/intel/i40e/i40e_main.c | 5 ++--
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++--
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 3 ++-
> drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 3 ++-
> drivers/net/vxlan.c | 29 ++----------------------
> include/linux/netdevice.h | 2 ++
> include/net/protocol.h | 2 ++
> include/net/vxlan.h | 8 -------
> net/ipv4/udp_offload.c | 27 ++++++++++++++++++++++
> 13 files changed, 53 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index ad2782f..56777c8 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -60,6 +60,7 @@
> #include <linux/semaphore.h>
> #include <linux/stringify.h>
> #include <linux/vmalloc.h>
> +#include <net/protocol.h>
>
> #include "bnx2x.h"
> #include "bnx2x_init.h"
> @@ -10293,7 +10294,7 @@ sp_rtnl_not_reset:
> netdev_info(bp->dev,
> "Deleted vxlan dest port %d", port);
> bp->vxlan_dst_port = 0;
> - vxlan_get_rx_port(bp->dev);
> + udp_offload_get_port(bp->dev);
> }
> }
> #endif
> @@ -12499,7 +12500,7 @@ static int bnx2x_open(struct net_device *dev)
>
> #ifdef CONFIG_BNX2X_VXLAN
> if (IS_PF(bp))
> - vxlan_get_rx_port(dev);
> + udp_offload_get_port(dev);
> #endif
>
> return 0;
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 5b96ddf..f49ca38 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -39,6 +39,7 @@
> #include <net/ip6_checksum.h>
> #if defined(CONFIG_VXLAN) || defined(CONFIG_VXLAN_MODULE)
> #include <net/vxlan.h>
> +#include <net/protocol.h>
> #endif
> #ifdef CONFIG_NET_RX_BUSY_POLL
> #include <net/busy_poll.h>
> @@ -4589,7 +4590,7 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
>
> if (irq_re_init) {
> #if defined(CONFIG_VXLAN) || defined(CONFIG_VXLAN_MODULE)
> - vxlan_get_rx_port(bp->dev);
> + udp_offload_get_port(bp->dev);
> #endif
> if (!bnxt_hwrm_tunnel_dst_port_alloc(
> bp, htons(0x17c1),
> @@ -5458,6 +5459,7 @@ static void bnxt_del_vxlan_port(struct net_device *dev, sa_family_t sa_family,
>
> if (type != UDP_TUNNEL_VXLAN)
> return;
> +
> if (bp->vxlan_port_cnt && bp->vxlan_port == port) {
> bp->vxlan_port_cnt--;
>
This looks like a bit of white-space bleed-through from your first
patch. You probably need to place it there.
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index e699deca..a4da753 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -25,6 +25,7 @@
> #include <net/busy_poll.h>
> #include <net/udp_tunnel.h>
> #include <net/vxlan.h>
> +#include <net/protocol.h>
>
> MODULE_VERSION(DRV_VER);
> MODULE_DESCRIPTION(DRV_DESC " " DRV_VER);
> @@ -3604,7 +3605,7 @@ static int be_open(struct net_device *netdev)
>
> #ifdef CONFIG_BE2NET_VXLAN
> if (skyhawk_chip(adapter))
> - vxlan_get_rx_port(netdev);
> + udp_offload_get_port(netdev);
> #endif
>
> return 0;
> @@ -5239,6 +5240,7 @@ static void be_del_vxlan_port(struct net_device *netdev, sa_family_t sa_family,
>
> if (type != UDP_TUNNEL_VXLAN)
> return;
> +
> if (lancer_chip(adapter) || BEx_chip(adapter) || be_is_mc(adapter))
> return;
>
Same here. We don't need white space changes in these patches as they
just clutter things up.
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> index 447d5e6..1564a13 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> @@ -23,6 +23,7 @@
> #if IS_ENABLED(CONFIG_FM10K_VXLAN)
> #include <net/udp_tunnel.h>
> #include <net/vxlan.h>
> +#include <net/protocol.h>
> #endif /* CONFIG_FM10K_VXLAN */
>
> /**
> @@ -573,7 +574,7 @@ int fm10k_open(struct net_device *netdev)
>
> #if IS_ENABLED(CONFIG_FM10K_VXLAN)
> /* update VXLAN port configuration */
> - vxlan_get_rx_port(netdev);
> + udp_offload_get_port(netdev);
>
> #endif
> fm10k_up(interface);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 520e34e..4be0a26 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -31,6 +31,7 @@
> #include <net/vxlan.h>
> #endif
> #include <net/udp_tunnel.h>
> +#include <net/protocol.h>
>
> const char i40e_driver_name[] = "i40e";
> static const char i40e_driver_string[] =
> @@ -5303,9 +5304,7 @@ int i40e_open(struct net_device *netdev)
> TCP_FLAG_CWR) >> 16);
> wr32(&pf->hw, I40E_GLLAN_TSOMSK_L, be32_to_cpu(TCP_FLAG_CWR) >> 16);
>
> -#ifdef CONFIG_I40E_VXLAN
> - vxlan_get_rx_port(netdev);
> -#endif
> + udp_offload_get_port(netdev);
>
> return 0;
> }
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 76ccc77..ba92c7a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -52,6 +52,7 @@
> #include <scsi/fc/fc_fcoe.h>
> #include <net/udp_tunnel.h>
> #include <net/vxlan.h>
> +#include <net/protocol.h>
>
> #ifdef CONFIG_OF
> #include <linux/of_net.h>
> @@ -5823,7 +5824,7 @@ static int ixgbe_open(struct net_device *netdev)
>
> ixgbe_clear_vxlan_port(adapter);
> #ifdef CONFIG_IXGBE_VXLAN
> - vxlan_get_rx_port(netdev);
> + udp_offload_get_port(netdev);
> #endif
>
> return 0;
> @@ -6913,7 +6914,7 @@ static void ixgbe_service_task(struct work_struct *work)
> #ifdef CONFIG_IXGBE_VXLAN
> if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) {
> adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED;
> - vxlan_get_rx_port(adapter->netdev);
> + udp_offload_get_port(adapter->netdev);
> }
> #endif /* CONFIG_IXGBE_VXLAN */
> ixgbe_reset_subtask(adapter);
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 2cb19c7..b91b8f1 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -41,6 +41,7 @@
> #include <net/busy_poll.h>
> #include <net/udp_tunnel.h>
> #include <net/vxlan.h>
> +#include <net/protocol.h>
>
> #include <linux/mlx4/driver.h>
> #include <linux/mlx4/device.h>
> @@ -1684,7 +1685,7 @@ int mlx4_en_start_port(struct net_device *dev)
>
> #ifdef CONFIG_MLX4_EN_VXLAN
> if (priv->mdev->dev->caps.tunnel_offload_mode == MLX4_TUNNEL_OFFLOAD_MODE_VXLAN)
> - vxlan_get_rx_port(dev);
> + udp_offload_get_port(dev);
> #endif
> priv->port_up = true;
> netif_tx_start_all_queues(dev);
> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> index aa38dbb..a640872 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> @@ -19,6 +19,7 @@
> #ifdef CONFIG_QLCNIC_VXLAN
> #include <net/udp_tunnel.h>
> #include <net/vxlan.h>
> +#include <net/protocol.h>
> #endif
>
> #include "qlcnic.h"
> @@ -2026,7 +2027,7 @@ qlcnic_attach(struct qlcnic_adapter *adapter)
>
> #ifdef CONFIG_QLCNIC_VXLAN
> if (qlcnic_encap_rx_offload(adapter))
> - vxlan_get_rx_port(netdev);
> + udp_offload_get_port(netdev);
> #endif
>
> adapter->is_up = QLCNIC_ADAPTER_UP_MAGIC;
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 5490629..702f9be 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -2424,33 +2424,6 @@ static struct device_type vxlan_type = {
> .name = "vxlan",
> };
>
> -/* Calls the ndo_add_udp_tunnel_port of the caller in order to
> - * supply the listening VXLAN udp ports. Callers are expected
> - * to implement the ndo_add_tunnel_port.
> - */
> -void vxlan_get_rx_port(struct net_device *dev)
> -{
> - struct vxlan_sock *vs;
> - struct net *net = dev_net(dev);
> - struct vxlan_net *vn = net_generic(net, vxlan_net_id);
> - sa_family_t sa_family;
> - __be16 port;
> - unsigned int i;
> -
> - spin_lock(&vn->sock_lock);
> - for (i = 0; i < PORT_HASH_SIZE; ++i) {
> - hlist_for_each_entry_rcu(vs, &vn->sock_list[i], hlist) {
> - port = inet_sk(vs->sock->sk)->inet_sport;
> - sa_family = vxlan_get_sk_family(vs);
> - dev->netdev_ops->ndo_add_udp_tunnel_port(dev, sa_family,
> - port,
> - UDP_TUNNEL_VXLAN);
> - }
> - }
> - spin_unlock(&vn->sock_lock);
> -}
> -EXPORT_SYMBOL_GPL(vxlan_get_rx_port);
> -
> /* Initialize the device structure. */
> static void vxlan_setup(struct net_device *dev)
> {
> @@ -2639,6 +2612,8 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, bool ipv6,
>
> /* Initialize the vxlan udp offloads structure */
> vs->udp_offloads.port = port;
> + vs->udp_offloads.tunnel_type = UDP_TUNNEL_VXLAN;
> + vs->udp_offloads.family = ipv6 ? AF_INET6 : AF_INET;
> vs->udp_offloads.callbacks.gro_receive = vxlan_gro_receive;
> vs->udp_offloads.callbacks.gro_complete = vxlan_gro_complete;
>
It seems like you are losing functionality here. The function
vxlan_get_sk_family was what was being used to obtain the socket family
before and it retrieved it from the socket.
If you are going to drop use of the function vxlan_get_sk_family like
this you should probably just to through the code and drop all of the
references to it since you have duplicated the functionality with the
tunnel_type field. It might make sense to make that a seperate patch
and to take care of it before you make this change.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index eaecc42..0073009 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2075,6 +2075,8 @@ struct udp_offload_callbacks {
>
> struct udp_offload {
> __be16 port;
> + u8 tunnel_type;
> + u8 family;
> u8 ipproto;
> struct udp_offload_callbacks callbacks;
> };
You cast family as a u8, but skc_family is an unsigned short. You
should probably use either that or sa_family_t.
> diff --git a/include/net/protocol.h b/include/net/protocol.h
> index d6fcc1f..738bfc6 100644
> --- a/include/net/protocol.h
> +++ b/include/net/protocol.h
> @@ -110,6 +110,8 @@ void inet_unregister_protosw(struct inet_protosw *p);
> int udp_add_offload(struct udp_offload *prot);
> void udp_del_offload(struct udp_offload *prot);
>
> +void udp_offload_get_port(struct net_device *dev);
> +
> #if IS_ENABLED(CONFIG_IPV6)
> int inet6_add_protocol(const struct inet6_protocol *prot, unsigned char num);
> int inet6_del_protocol(const struct inet6_protocol *prot, unsigned char num);
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index c1c899c..926455e 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -242,14 +242,6 @@ static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
> /* IPv6 header + UDP + VXLAN + Ethernet header */
> #define VXLAN6_HEADROOM (40 + 8 + 8 + 14)
>
> -#if IS_ENABLED(CONFIG_VXLAN)
> -void vxlan_get_rx_port(struct net_device *netdev);
> -#else
> -static inline void vxlan_get_rx_port(struct net_device *netdev)
> -{
> -}
> -#endif
> -
> static inline unsigned short vxlan_get_sk_family(struct vxlan_sock *vs)
> {
> return vs->sock->sk->sk_family;
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index f938616..8597020 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -290,6 +290,33 @@ unlock:
> }
> EXPORT_SYMBOL(udp_del_offload);
>
> +void udp_offload_get_port(struct net_device *dev)
> +{
> + struct udp_offload_priv __rcu **head;
> + struct udp_offload_priv *uo_priv;
> + struct udp_offload *uo;
> +
> + if (udp_offload_base)
> + head = &udp_offload_base;
> + else
> + return;
> +
> + spin_lock(&udp_offload_lock);
> + uo_priv = udp_deref_protected(*head);
> + for (; uo_priv != NULL; uo_priv = udp_deref_protected(*head)) {
You can really simplify all of this by using a while loop here instead
of a for loop. Just check for "while (uo_priv)".
> + /* call the right add port */
> + uo = uo_priv->offload;
> + if (uo && dev->netdev_ops->ndo_add_udp_tunnel_port)
> + dev->netdev_ops->ndo_add_udp_tunnel_port(dev,
> + uo->family,
> + uo->port,
> + uo->tunnel_type);
> + head = &uo_priv->next;
No need to carry head, it is just dead weight. At this point you could do:
uo_priv = udp_deref_protected(uo_priv->next);
> + }
> + spin_unlock(&udp_offload_lock);
> +}
> +EXPORT_SYMBOL(udp_offload_get_port);
> +
> struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
> struct udphdr *uh)
> {
>
--
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
Powered by blists - more mailing lists