Re: [PATCH v2] drivers/virt/vSMP: new driver

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Aug 25, 2022 at 06:24:02AM +0000, Czerwacki, Eial wrote:
> +obj-$(CONFIG_VSMP) = vsmp.o
> +vsmp-y := vsmp_main.o api/api.o version/version.o
> diff --git a/drivers/virt/vsmp/api/api.c b/drivers/virt/vsmp/api/api.c
> new file mode 100644
> index 000000000000..6e40935907bc
> --- /dev/null
> +++ b/drivers/virt/vsmp/api/api.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * vSMP driver api
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#include "api.h"
> +
> +static void __iomem *cfg_addr;
> +static struct kobject *vsmp_sysfs_kobj;
> +static struct pci_dev *vsmp_dev_obj;

Try to avoid globals as much as possible.

> +
> +/* R/W ops handlers */
> +
> +/*
> + * Init a vsmp firmware operation object
> + */
> +int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
> +		 enum vsmp_fw_action action)
> +{
> +	op->hwi_block_size = max_size;
> +	op->action = action;
> +	op->buff_offset = op->hwi_block_size;

The ->buff_offset is the number of empty bytes.  It's badly named,
because it's not an offset at all.

> +
> +	op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL);
> +	if (!op->buff)
> +		return -ENOMEM;
> +
> +	vsmp_reset_op(op);

This is a no-op.  op->buff_offset is already set and ->buf is already
zero.  Delete.

> +
> +	return 0;
> +}
> +
> +/*
> + * Release an vsmp firmware operation object
> + */
> +void vsmp_release_op(struct fw_ops *op)
> +{
> +	if (!op) {
> +		WARN_ON(!op);

	if (WARN_ON(!op))
		return;

> +		return;
> +	}
> +
> +	if (!op->buff) {
> +		WARN_ON(!op->buff);
> +		return;
> +	}
> +
> +	kfree(op->buff);
> +	memset(op, 0, sizeof(*op));
> +}
> +
> +/*
> + * Reset a vsmp firmware operation object
> + */
> +void vsmp_reset_op(struct fw_ops *op)
> +{
> +	memset(op->buff, 0, op->hwi_block_size);
> +	op->buff_offset = op->hwi_block_size;
> +}
> +
> +/* Regs/Buffs R/W handlers */
> +
> +/*
> + * Read a value from a specific register in the vSMP's device config space
> + */
> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type)
> +{
> +	u64 ret_val;
> +
> +	switch (type) {
> +	case VSMP_CTL_REG_SIZE_8BIT:
> +		ret_val = readb(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_16BIT:
> +		ret_val = readw(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_32BIT:
> +		ret_val = readl(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_64BIT:
> +		ret_val = readq(cfg_addr + reg);
> +		break;
> +
> +	default:
> +		dev_err(get_dev(), "Unsupported reg size type %d.\n", type);
> +		ret_val = (u64) -EINVAL;

This can never happen and there are two places which check for negative
returns but both are wrong.  Delete the dead code or make it simple.

		dev_err(get_dev(), "Unsupported reg size type %d.\n", type);
		ret_val = 0;

Then delete all the error handling in the callers.

> +	}
> +
> +	dev_dbg(get_dev(), "%s: read 0x%llx from reg 0x%llx of %d bits\n",
> +		__func__, ret_val, reg, (type + 1) * 8);

This function only exists because we want this debug statement.  I have
a strong bias towards deleting debug code and calling readl() directly.
In other words, delete this whole function.  Or keep it...  But at least
consider deleting it.

> +	return ret_val;
> +}
> +
> +/*
> + * Read a buffer from the bar byte by byte for halt on
> + * null termination.
> + * Expected buffs are strings.
> + */
> +static ssize_t read_buff_from_bar_in_bytes(char *out, u8 __iomem *buff, ssize_t len)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < len; i++) {
> +		out[i] = ioread8(&buff[i]);
> +		if (!out[i])
> +			break;
> +	}
> +
> +	return i;
> +}
> +
> +/*
> + * Read a buffer from a specific offset in a specific bar,
> + * maxed to a predefined len size-wise from the vSMP device
> + */
> +int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,

The type mismatches bother me.  In some of the callers "bar" and "offset"
are u64 reg_vals but here they are a u8 and a u32.

> +			    bool halt_on_null)
> +{
> +	u8 __iomem *buff;
> +	u64 bar_start = pci_resource_start(vsmp_dev_obj, bar);
> +	u32 bar_len = pci_resource_len(vsmp_dev_obj, bar);
> +	ssize_t actual_len = len;
> +
> +	/* incase of overflow, warn and use max len possible */
> +	if ((offset + len) > bar_len) {
> +		WARN_ON((offset + len) > actual_len);
> +		actual_len = bar_len - offset;
> +		dev_dbg(get_dev(), "%lu overflows bar len, using %ld len instead\n",
> +			len, actual_len);
> +	}
> +
> +	buff = ioremap(bar_start + offset, actual_len);
> +	if (!buff)
> +		return -ENOMEM;
> +
> +	if (halt_on_null)
> +		read_buff_from_bar_in_bytes(out, buff, len);
> +	else
> +		memcpy_fromio(out, buff, len);

These are buffer overflows.  s/len/actual_len/

> +
> +	iounmap(buff);
> +
> +	return 0;
> +}
> +
> +/*
> + * Generic function to read from the bar
> + */
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
> +			       char *buf, loff_t off, ssize_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (op->buff_offset >= op->hwi_block_size) {	/* perform H/W op */

Hopefully op->buff_offset can never be > op->hwi_block_size.  (I worry
that because "op" is a global maybe race conditions mean this is
possible.)  Remeber that op->buff_offset means "bytes_available", it is
not an offset.  So if all the bytes are available then...

> +		vsmp_reset_op(op);

Delete everything, and set op->buff_offset == op->hwi_block_size.

> +
> +		ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff, op->hwi_block_size, false);

Read some data.

> +		if (ret_val) {
> +			dev_err(get_dev(), "%s operation failed\n",
> +				(op->action == FW_READ) ? "read" : "write");
> +		}
> +		op->buff_offset = 0;

No bytes available.

> +	}

Or else read what we have.

> +
> +	if (ret_val)
> +		return ret_val;
> +
> +	return memory_read_from_buffer(buf, count, &op->buff_offset, op->buff, op->hwi_block_size);
                                                   ^^^^^^^^^^^^^^^^
Here offset is used as an offset.  If it's not zero then this is read
nonsense data.


> +}
> +
> +/* sysfs handlers */
> +
> +/*
> + * Register the vSMP sysfs object for user space interaction
> + */
> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	int error = -EINVAL;
> +
> +	if (vsmp_sysfs_kobj && bin_attr) {

Always do error handling.  Never do success handling.

	if (!vsmp_sysfs_kobj || !bin_attr)
		return -EINVAL;

	return sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);


> +		error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr);
> +		if (error)
> +			dev_err(get_dev(), "Failed to register sysfs entry (%d)\n", error);
> +	}
> +
> +	return error;
> +}
> +
> +/*
> + * Deregister the vSMP sysfs object for user space interaction
> + */
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	if (vsmp_sysfs_kobj && bin_attr)
> +		sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
> +}
> +
> +/* Generic functions */
> +
> +/*
> + * Open the cfg address space of the vSDP device
> + */
> +int open_cfg_addr(struct pci_dev *pdev)
> +{
> +	u64 cfg_start;
> +	u32 cfg_len;
> +
> +	vsmp_dev_obj = pdev;
> +	cfg_start = pci_resource_start(vsmp_dev_obj, 0);
> +	cfg_len = pci_resource_len(vsmp_dev_obj, 0);
> +
> +	dev_dbg(get_dev(), "Mapping bar 0: [0x%llx,0x%llx]\n",
> +		cfg_start, cfg_start + cfg_len);
> +
> +	cfg_addr = ioremap(cfg_start, cfg_len);
> +	if (!cfg_addr) {
> +		dev_err(get_dev(), "Failed to map vSMP pci controller, exiting.\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +int init_sysfs(void)
> +{
> +	vsmp_sysfs_kobj = kobject_create_and_add("vsmp", hypervisor_kobj);
> +	if (!vsmp_sysfs_kobj) {
> +		dev_err(get_dev(), "Failed to create vSMP sysfs entry, exiting.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +void cleanup(void)
> +{
> +	iounmap(cfg_addr);
> +	kobject_put(vsmp_sysfs_kobj);
> +}
> +
> +const struct device *get_dev(void)
> +{
> +	return &vsmp_dev_obj->dev;
> +}
> diff --git a/drivers/virt/vsmp/api/api.h b/drivers/virt/vsmp/api/api.h
> new file mode 100644
> index 000000000000..6142e947979f
> --- /dev/null
> +++ b/drivers/virt/vsmp/api/api.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * vSMP driver api header
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#ifndef VSMP_API_H
> +#define VSMP_API_H
> +
> +#include <linux/pci.h>
> +
> +// R/W ops handlers
> +#define vsmp_read_reg32_from_cfg(_reg_) \
> +	((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
> +
> +enum reg_size_type {
> +	VSMP_CTL_REG_SIZE_8BIT = 0,
> +	VSMP_CTL_REG_SIZE_16BIT,
> +	VSMP_CTL_REG_SIZE_32BIT,
> +	VSMP_CTL_REG_SIZE_64BIT
> +};
> +
> +enum vsmp_fw_action {
> +	FW_READ = 0,
> +	FW_WRITE = 1
> +};
> +
> +struct fw_ops {
> +	enum vsmp_fw_action action;
> +	ssize_t hwi_block_size;
> +	unsigned char *buff;
> +	loff_t buff_offset;
> +	bool in_progress;
> +};
> +
> +int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
> +		 enum vsmp_fw_action action);
> +void vsmp_release_op(struct fw_ops *op);
> +void vsmp_reset_op(struct fw_ops *op);
> +
> +#define FILE_PREM 0444

Spelling mistake.  Delete the define.  Avoid pointless indirection.

> +
> +/* Regs/Buffs R/W handlers */
> +#define vsmp_read_reg32_from_cfg(_reg_) \
> +	((u32) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))

Delete this duplicate define.

> +
> +u64 vsmp_read_reg_from_cfg(u64 reg, enum reg_size_type type);
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, u8 bar, u64 reg,
> +			       char *buf, loff_t off, ssize_t count);
> +int vsmp_read_buff_from_bar(u8 bar, u32 offset, char *out, ssize_t len,
> +			    bool halt_on_null);
> +
> +typedef int (*sysfs_register_cb)(void);
> +typedef void (*sysfs_deregister_cb)(void);
> +
> +struct sysfs_entry_cbs {
> +	sysfs_register_cb reg_cb;
> +	sysfs_deregister_cb dereg_cb;
> +};
> +
> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr);
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr);
> +
> +int open_cfg_addr(struct pci_dev *pdev);
> +int init_sysfs(void);
> +void cleanup(void);
> +const struct device *get_dev(void);
> +#endif /* VSMP_API_H */
> diff --git a/drivers/virt/vsmp/include/registers.h b/drivers/virt/vsmp/include/registers.h
> new file mode 100644
> index 000000000000..b6458d25e3b7
> --- /dev/null
> +++ b/drivers/virt/vsmp/include/registers.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP driver registers
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#ifndef VSMP_REGSITERS_H
> +#define VSMP_REGSITERS_H
> +
> +#define VSMP_VERSION_REG 0x0c
> +
> +#endif /* VSMP_REGSITERS_H */
> diff --git a/drivers/virt/vsmp/version/version.c b/drivers/virt/vsmp/version/version.c
> new file mode 100644
> index 000000000000..d8ad771daf28
> --- /dev/null
> +++ b/drivers/virt/vsmp/version/version.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * vSMP driver version module
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kobject.h>
> +
> +#include "../api/api.h"
> +#include "../include/registers.h"
> +
> +/*
> + * This is the maximal possible length of the version which is a text string
> + * the real len is usually much smaller, thus the driver uses this once to read
> + * the version string and record it's actual len.
> + * From that point and on, the actual len will be used in each call.
> + */
> +#define VERSION_MAX_LEN (1 << 19)
> +
> +static struct fw_ops op;

Hate hate hate this global.

> +
> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
> +			    struct bin_attribute *bin_attr,
> +			    char *buf, loff_t off, size_t count)
> +{
> +	u64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	ssize_t ret_val;
> +
> +	if (reg_val < 0) {

Delete impossible error handling.

> +		dev_err(get_dev(), "Failed to value of reg 0x%x\n", VSMP_VERSION_REG);
> +		return 0;
> +	}
> +
> +	ret_val = vsmp_generic_buff_read(&op, 0, reg_val, buf, off, count);
> +	if (ret_val < 0) {
> +		dev_err(get_dev(), "Failed to read version (%ld)\n", ret_val);
> +		return 0;

Return the error code.

> +	}
> +
> +	buf[ret_val++] = '\n';

This is a buffer overflow.

> +
> +	return ret_val;
> +}
> +
> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM,
> +						   version_read, NULL, VERSION_MAX_LEN);
> +
> +/*
> + * Retrieve str in order to determine the proper length.
> + * This is the best way to maintain backwards compatibility with all
> + * vSMP versions.
> + */
> +static ssize_t get_version_len(void)

Just make this an int.  kzalloc() cannot allocate anywhere close to
INT_MAX.

> +{
> +	ssize_t len = 0;
> +	u64 reg_val = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_KERNEL);

Don't put function calls which can fail in the declaration block.

> +
> +	if (!version_str)
> +		return len;

Return -ENOMEM instead of zero.

> +
> +	if (vsmp_read_reg32_from_cfg(VSMP_VERSION_REG) < 0) {

Delete this impossible error handling.  Delete this block.  We already
saved VSMP_VERSION_REG in the declaration block.

> +		kfree(version_str);
> +		dev_err(get_dev(), "Failed to read value of reg 0x%x\n", VSMP_VERSION_REG);
> +		return len;
> +	}
> +
> +	memset(version_str, 0, VERSION_MAX_LEN);

kzalloc() already zeroes your memory.

> +	if (vsmp_read_buff_from_bar(0, reg_val, version_str, VERSION_MAX_LEN, true)) {
> +		kfree(version_str);
> +		dev_err(get_dev(), "Failed to read buffer from bar\n");
> +		return len;
> +	}
> +
> +	len = strlen(version_str);

This is a read overflow because there is no guarantee that
vsmp_read_buff_from_bar() returns a NULL terminated string.

> +	kfree(version_str);
> +
> +	return len;
> +}
> +
> +/*
> + * Register the version sysfs entry
> + */
> +int sysfs_register_version_cb(void)
> +{
> +	ssize_t len = get_version_len();
> +	int ret_val;
> +
> +	if (!len) {
> +		dev_err(get_dev(), "Failed to init vSMP version module\n");
> +		return -EINVAL;
> +	}
> +	version_raw_attr.size = len;
> +
> +	if (vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {

get_version_len() does not count the NUL terminator on the end of the
string so this does not allocate enough memory.

> +		dev_err(get_dev(), "Failed to init vSMP version op\n");
> +		return -ENODEV;

Preserve the error code.

> +	}
> +
> +	ret_val = vsmp_register_sysfs_group(&version_raw_attr);
> +	if (ret_val) {
> +		dev_err(get_dev(), "Failed to init vSMP version support\n");
> +		vsmp_release_op(&op);

Better to return here.  (Avoid mixing error path and success path).

> +	}
> +
> +	return ret_val;

return 0;

> +}
> +
> +/*
> + * Deregister the version sysfs entry
> + */
> +void sysfs_deregister_version_cb(void)
> +{
> +	vsmp_deregister_sysfs_group(&version_raw_attr);
> +	vsmp_release_op(&op);
> +}
> diff --git a/drivers/virt/vsmp/version/version.h b/drivers/virt/vsmp/version/version.h
> new file mode 100644
> index 000000000000..c4430b3065e4
> --- /dev/null
> +++ b/drivers/virt/vsmp/version/version.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * vSMP driver version module header
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#ifndef VSMP_VERSION_COMMON_H
> +#define VSMP_VERSION_COMMON_H
> +
> +int sysfs_register_version_cb(void);
> +void sysfs_deregister_version_cb(void);
> +
> +#endif /* VSMP_VERSION_COMMON_H */
> diff --git a/drivers/virt/vsmp/vsmp_main.c b/drivers/virt/vsmp/vsmp_main.c
> new file mode 100644
> index 000000000000..95704bc7a32f
> --- /dev/null
> +++ b/drivers/virt/vsmp/vsmp_main.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * vSMP driver main
> + * (C) Copyright 2022 SAP SE
> + */
> +
> +#include <linux/module.h>
> +
> +#include "api/api.h"
> +#include "version/version.h"
> +
> +/* modules info */
> +#define DEVICE_NAME "vSMP"
> +#define DRIVER_LICENSE "GPL v2"
> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> +#define DRIVER_DESC "vSMP hypervisor driver"
> +#define DRIVER_VERSION "0.1"
> +
> +#define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
> +
> +MODULE_LICENSE(DRIVER_LICENSE);
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_VERSION(DRIVER_VERSION);
> +
> +/* Sysfs handlers */
> +#define create_entry(_label_) \
> +	{ \
> +		.reg_cb = sysfs_register_ ## _label_ ## _cb, \
> +		.dereg_cb = sysfs_deregister_ ## _label_ ## _cb, \
> +	}
> +
> +static struct sysfs_entry_cbs cbs_arr[] = {
> +	create_entry(version),
> +};
> +
> +static const struct pci_device_id vsmp_pci_table[] = {
> +	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
> +	{ 0, },			/* terminate list */
> +};
> +
> +/*
> + * Init all submodules's sysfs entries
> + */
> +static int add_sysfs_entries(void)
> +{
> +	int ret_val = 0, i;
> +
> +	for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
> +		ret_val = cbs_arr[i].reg_cb();
> +		if (ret_val) {
> +			dev_err(get_dev(), "Failed to init sysfs entry %d (%d).\n",
> +				i, ret_val);

Clean up on error.

> +		}
> +	}
> +
> +	return ret_val;
> +}

static int add_sysfs_entries(void)
{
	int ret_val, i;

	for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
		ret_val = cbs_arr[i].reg_cb();
		if (ret_val) {
			dev_err(get_dev(), "Failed to init sysfs entry %d (%d).\n",
				i, ret_val);
			goto cleanup;
		}
	}

	return 0;

cleanup:
	while (--i >= 0)
		cbs_arr[i].dereg_cb()
	return ret_val;
}

> +
> +/*
> + * Remove all submodules's sysfs entries
> + */
> +static void remove_sysfs_entries(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cbs_arr); i++)
> +		cbs_arr[i].dereg_cb();
> +}
> +
> +static int vsmp_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> +	int ret_val;
> +
> +	ret_val = open_cfg_addr(pci);
> +	if (ret_val) {
> +		dev_err(get_dev(), "Failed to open cfg addr\n");
> +		return ret_val;
> +	}
> +
> +	if (init_sysfs()) {
> +		dev_err(get_dev(), "Failed to create sysfs folder\n");
> +		return -ENODEV;

Add cleanup.  Preserve the error code.

> +	}
> +
> +	if (add_sysfs_entries()) {
> +		dev_err(get_dev(), "Failed to create sysfs entries\n");
> +		return -ENODEV;

Don't leak.  Preserve the error code.

regards,
dan carpenter

> +	}
> +
> +	dev_info(get_dev(), "%s up and running\n", DRIVER_DESC);
> +
> +	return 0;
> +}
> +
> +static void vsmp_pci_remove(struct pci_dev *pci)
> +{
> +	remove_sysfs_entries();
> +	cleanup();
> +}
> +
> +static struct pci_driver vsmp_pci_driver = {
> +	.name		= DEVICE_NAME,
> +	.id_table	= vsmp_pci_table,
> +	.probe		= vsmp_pci_probe,
> +	.remove	= vsmp_pci_remove,
> +};
> +
> +module_pci_driver(vsmp_pci_driver);