Messages in this thread Patch in this message | | | Date | Thu, 25 Aug 2022 15:41:23 +0300 | From | Dan Carpenter <> | 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@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);
| |