[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220825124123.GE2030@kadam>
Date: Thu, 25 Aug 2022 15:41:23 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: "Czerwacki, Eial" <eial.czerwacki@....com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Arsh, Leonid" <leonid.arsh@....com>,
"Twaig, Oren" <oren.twaig@....com>,
SAP vSMP Linux Maintainer <linux.vsmp@....com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arnd Bergmann <arnd@...db.de>,
Andra Paraschiv <andraprs@...zon.com>,
Borislav Petkov <bp@...e.de>,
Brijesh Singh <brijesh.singh@....com>,
Eric Biggers <ebiggers@...gle.com>, Fei Li <fei1.li@...el.com>,
Hans de Goede <hdegoede@...hat.com>,
Jens Axboe <axboe@...nel.dk>,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Subject: Re: [PATCH v2] drivers/virt/vSMP: new driver
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@....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);
Powered by blists - more mailing lists