netdev - Re: [net-next 1/4] gve: Add basic driver framework for Compute Engine Virtual NIC

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: <20190626160832.3f191a53@cakuba.netronome.com>
Date:   Wed, 26 Jun 2019 16:08:32 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Catherine Sullivan <csully@...gle.com>
Cc:     netdev@...r.kernel.org, Sagi Shahar <sagis@...gle.com>,
        Jon Olson <jonolson@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Luigi Rizzo <lrizzo@...gle.com>
Subject: Re: [net-next 1/4] gve: Add basic driver framework for Compute
 Engine Virtual NIC

On Wed, 26 Jun 2019 11:52:48 -0700, Catherine Sullivan wrote:
> Add a driver framework for the Compute Engine Virtual NIC that will be
> available in the future.
> 
> At this point the only functionality is loading the driver.
> 
> Signed-off-by: Catherine Sullivan <csully@...gle.com>
> Signed-off-by: Sagi Shahar <sagis@...gle.com>
> Signed-off-by: Jon Olson <jonolson@...gle.com>
> Acked-by: Willem de Bruijn <willemb@...gle.com>
> Reviewed-by: Luigi Rizzo <lrizzo@...gle.com>
> ---

> +if NET_VENDOR_GOOGLE
> +
> +config GVE
> +	tristate "Google Virtual NIC (gVNIC) support"
> +	depends on (PCI_MSI && X86)

We usually prefer for drivers not to depend on the platform unless
really necessary, but IDK how that applies to the curious new world 
of NICs nobody can buy :)

> +	help
> +	  This driver supports Google Virtual NIC (gVNIC)"
> +
> +	  To compile this driver as a module, choose M here.
> +	  The module will be called gve.
> +
> +endif #NET_VENDOR_GOOGLE

> +void gve_adminq_release(struct gve_priv *priv)
> +{
> +	int i;
> +
> +	/* Tell the device the adminq is leaving */
> +	writel(0x0, &priv->reg_bar0->adminq_pfn);
> +	for (i = 0; i < GVE_MAX_ADMINQ_RELEASE_CHECK; i++) {
> +		if (!readl(&priv->reg_bar0->adminq_pfn)) {
> +			gve_clear_device_rings_ok(priv);
> +			gve_clear_device_resources_ok(priv);
> +			gve_clear_admin_queue_ok(priv);
> +			return;
> +		}
> +		msleep(GVE_ADMINQ_SLEEP_LEN);
> +	}
> +	/* If this is reached the device is unrecoverable and still holding
> +	 * memory. Anything other than a BUG risks memory corruption.
> +	 */
> +	WARN(1, "Unrecoverable platform error!");
> +	BUG();

Please don't add BUG()s to the kernel.  You're probably better off
spinning for ever in the loop above.  Also if there is an IOMMU in 
the way the device won't be able to mess with the memory.

> +}
> +

> diff --git a/drivers/net/ethernet/google/gve/gve_size_assert.h b/drivers/net/ethernet/google/gve/gve_size_assert.h
> new file mode 100644
> index 000000000000..a58422d4f16e
> --- /dev/null
> +++ b/drivers/net/ethernet/google/gve/gve_size_assert.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT)
> + * Google virtual Ethernet (gve) driver
> + *
> + * Copyright (C) 2015-2019 Google, Inc.
> + */
> +
> +#ifndef _GVE_ASSERT_H_
> +#define _GVE_ASSERT_H_
> +#define GVE_ASSERT_SIZE(tag, type, size) \
> +	static void gve_assert_size_ ## type(void) __attribute__((used)); \
> +	static inline void gve_assert_size_ ## type(void) \
> +	{ \
> +		BUILD_BUG_ON(sizeof(tag type) != (size)); \
> +	}
> +#endif /* _GVE_ASSERT_H_ */

Please use static_assert() directly in your struct size checks.