| 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: <CACVXFVNgYo6Az1GtH+buuT16inG0+8DQAETTTDwBeQ3DkmMt_w@mail.gmail.com>
Date: Tue, 8 Mar 2016 21:42:17 +0800
From: Ming Lei <tom.leiming@...il.com>
To: Stephen Boyd <stephen.boyd@...aro.org>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-arm@...ts.infradead.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Robin Murphy <robin.murphy@....com>,
Laura Abbott <labbott@...hat.com>,
Arnd Bergmann <arnd@...db.de>,
Marek Szyprowski <m.szyprowski@...sung.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Mark Brown <broonie@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
Vikram Mulukutla <markivx@...eaurora.org>,
Mimi Zohar <zohar@...ux.vnet.ibm.com>
Subject: Re: [RFC/PATCH 4/4] firmware: Support requesting firmware directly
into DMA memory
On Tue, Mar 8, 2016 at 5:22 PM, Stephen Boyd <stephen.boyd@...aro.org> wrote:
> Some systems are memory constrained but they need to load very
> large firmwares. The firmware subsystem allows drivers to request
> this firmware be loaded from the filesystem, but this requires
> that the entire firmware be loaded into kernel memory first
> before it's provided to the driver. This can lead to a situation
> where we map the firmware twice, once to load the firmware into
> kernel memory and once to copy the firmware into the final
> resting place.
The 2nd copy can be avoided and it should be OK to feed fw->pages
to DMA since you can get the pages via 'struct firmware *'.
We still can replace the vmalloc() in fw_read_file_contents() with
N*alloc_page() plus vmap() in case of direct loading.
>
> This design creates needless memory pressure and delays loading
> because we have to copy from kernel memory to somewhere else.
Given firmware request can't be a frequent operation, I don't think it is
a big deal about the so called memory pressure and delay.
> Let's add a request_firmware_dma() API that allows drivers to
> request firmware be loaded directly into a DMA buffer that's been
> pre-allocated. This skips the intermediate step of allocating a
> buffer in kernel memory to hold the firmware image while it's
> read from the filesystem and copying it to another location. It
> also requires that drivers know how much memory they'll require
> before requesting the firmware and negates any benefits of
> firmware caching.
>
> This is based on a patch from Vikram Mulukutla on
> codeaurora.org[1].
>
> [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=rel/msm-3.18&id=0a328c5f6cd999f5c591f172216835636f39bcb5
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Vikram Mulukutla <markivx@...eaurora.org>
> Cc: Mimi Zohar <zohar@...ux.vnet.ibm.com>
> Signed-off-by: Stephen Boyd <stephen.boyd@...aro.org>
> ---
>
> TODO:
> * Handle security_kernel_fw_from_file() in the userhelper fallback case
> * Rework for pending patches from Mimi Zohar that change the way files
> are read in kernel space
>
> drivers/base/firmware_class.c | 256 ++++++++++++++++++++++++++++++++----------
> include/linux/firmware.h | 13 +++
> 2 files changed, 207 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 9a616cf7ac9f..8ab7a418f1d7 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -29,6 +29,8 @@
> #include <linux/syscore_ops.h>
> #include <linux/reboot.h>
> #include <linux/security.h>
> +#include <linux/kernel.h>
> +#include <linux/dma-mapping.h>
>
> #include <generated/utsrelease.h>
>
> @@ -154,6 +156,15 @@ struct firmware_buf {
> const char *fw_id;
> };
>
> +struct firmware_dma_desc {
> + struct device *dev;
> + void *cpu_addr;
> + dma_addr_t dma_addr;
> + unsigned long offset;
> + size_t size;
> + struct dma_attrs *attrs;
> +};
> +
> struct fw_cache_entry {
> struct list_head list;
> const char *name;
> @@ -292,39 +303,83 @@ static const char * const fw_path[] = {
> module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
> MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
>
> -static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf,
> + struct firmware_dma_desc *dma)
> {
> - int size;
> + int size, rsize, remaining;
> char *buf;
> int rc;
> + loff_t offset;
> + unsigned long dma_offset;
>
> if (!S_ISREG(file_inode(file)->i_mode))
> return -EINVAL;
> size = i_size_read(file_inode(file));
> if (size <= 0)
> return -EINVAL;
> - buf = vmalloc(size);
> - if (!buf)
> - return -ENOMEM;
> - rc = kernel_read(file, 0, buf, size);
> - if (rc != size) {
> - if (rc > 0)
> - rc = -EIO;
> - goto fail;
> +
> + if (dma) {
> + if (size + dma->offset > dma->size)
> + return -EINVAL;
> +
> + dma_offset = dma->offset;
> + offset = 0;
> + remaining = size;
> +
> + while (remaining > 0) {
> + rsize = min_t(int, remaining, PAGE_SIZE);
> +
> + buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr,
> + rsize, dma_offset, dma->attrs);
> + if (!buf)
> + return -ENOMEM;
> +
> + rc = kernel_read(file, offset, buf, rsize);
> + if (rc != rsize) {
> + if (rc > 0)
> + rc = -EIO;
> + goto fail_dma;
> + }
> + rc = security_kernel_fw_from_file(file, buf, rsize);
> + if (rc)
> + goto fail_dma;
> +
> + dma_unremap(dma->dev, buf, rsize, dma_offset,
> + dma->attrs);
> + dma_offset += rsize;
> + offset += rsize;
> + remaining -= rsize;
> + }
> + } else {
> + buf = vmalloc(size);
> + if (!buf)
> + return -ENOMEM;
> + rc = kernel_read(file, 0, buf, size);
> + if (rc != size) {
> + if (rc > 0)
> + rc = -EIO;
> + goto fail;
> + }
> + rc = security_kernel_fw_from_file(file, buf, size);
> + if (rc)
> + goto fail;
> +
> + fw_buf->data = buf;
> }
> - rc = security_kernel_fw_from_file(file, buf, size);
> - if (rc)
> - goto fail;
> - fw_buf->data = buf;
> +
> fw_buf->size = size;
> return 0;
> fail:
> vfree(buf);
> return rc;
> +fail_dma:
> + dma_unremap(dma->dev, buf, rsize, dma_offset, dma->attrs);
> + return rc;
> }
>
> -static int fw_get_filesystem_firmware(struct device *device,
> - struct firmware_buf *buf)
> +static int
> +fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf,
> + struct firmware_dma_desc *dma)
> {
> int i, len;
> int rc = -ENOENT;
> @@ -351,7 +406,7 @@ static int fw_get_filesystem_firmware(struct device *device,
> file = filp_open(path, O_RDONLY, 0);
> if (IS_ERR(file))
> continue;
> - rc = fw_read_file_contents(file, buf);
> + rc = fw_read_file_contents(file, buf, dma);
> fput(file);
> if (rc)
> dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
> @@ -470,6 +525,7 @@ struct firmware_priv {
> struct device dev;
> struct firmware_buf *buf;
> struct firmware *fw;
> + struct firmware_dma_desc *dma;
> };
>
> static struct firmware_priv *to_firmware_priv(struct device *dev)
> @@ -716,6 +772,52 @@ out:
>
> static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
>
> +static ssize_t firmware_dma_rw(struct firmware_dma_desc *dma, char *buffer,
> + loff_t *offset, size_t count, bool read)
> +{
> + char *fw_buf;
> + int retval = count;
> +
> + if ((dma->offset + *offset + count) > dma->size)
> + return -EINVAL;
> +
> + fw_buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr, count,
> + dma->offset + *offset, dma->attrs);
> + if (!fw_buf)
> + return -ENOMEM;
> +
> + if (read)
> + memcpy(buffer, fw_buf, count);
> + else
> + memcpy(fw_buf, buffer, count);
> +
> + dma_unremap(dma->dev, fw_buf, count, dma->offset + *offset, dma->attrs);
> + *offset += count;
> +
> + return retval;
> +}
> +
> +static void firmware_rw(struct firmware_buf *buf, char *buffer,
> + loff_t offset, size_t count, bool read)
> +{
> + while (count) {
> + void *page_data;
> + int page_nr = offset >> PAGE_SHIFT;
> + int page_ofs = offset & (PAGE_SIZE-1);
> + int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> +
> + page_data = kmap(buf->pages[page_nr]);
> +
> + memcpy(buffer, page_data + page_ofs, page_cnt);
> +
> + kunmap(buf->pages[page_nr]);
> + buffer += page_cnt;
> + offset += page_cnt;
> + count -= page_cnt;
> + }
> +
> +}
> +
> static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr,
> char *buffer, loff_t offset, size_t count)
> @@ -740,21 +842,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>
> ret_count = count;
>
> - while (count) {
> - void *page_data;
> - int page_nr = offset >> PAGE_SHIFT;
> - int page_ofs = offset & (PAGE_SIZE-1);
> - int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> -
> - page_data = kmap(buf->pages[page_nr]);
> + if (fw_priv->dma)
> + ret_count = firmware_dma_rw(fw_priv->dma, buffer, &offset,
> + count, true);
> + else
> + firmware_rw(buf, buffer, offset, count, true);
>
> - memcpy(buffer, page_data + page_ofs, page_cnt);
> -
> - kunmap(buf->pages[page_nr]);
> - buffer += page_cnt;
> - offset += page_cnt;
> - count -= page_cnt;
> - }
> out:
> mutex_unlock(&fw_lock);
> return ret_count;
> @@ -817,6 +910,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
> {
> struct device *dev = kobj_to_dev(kobj);
> struct firmware_priv *fw_priv = to_firmware_priv(dev);
> + struct firmware_dma_desc *dma = fw_priv->dma;
> struct firmware_buf *buf;
> ssize_t retval;
>
> @@ -830,26 +924,17 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
> goto out;
> }
>
> - retval = fw_realloc_buffer(fw_priv, offset + count);
> - if (retval)
> - goto out;
> -
> - retval = count;
> -
> - while (count) {
> - void *page_data;
> - int page_nr = offset >> PAGE_SHIFT;
> - int page_ofs = offset & (PAGE_SIZE - 1);
> - int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> -
> - page_data = kmap(buf->pages[page_nr]);
> -
> - memcpy(page_data + page_ofs, buffer, page_cnt);
> + if (dma) {
> + retval = firmware_dma_rw(dma, buffer, &offset, count, false);
> + if (retval < 0)
> + goto out;
> + } else {
> + retval = fw_realloc_buffer(fw_priv, offset + count);
> + if (retval)
> + goto out;
>
> - kunmap(buf->pages[page_nr]);
> - buffer += page_cnt;
> - offset += page_cnt;
> - count -= page_cnt;
> + retval = count;
> + firmware_rw(buf, buffer, offset, count, false);
> }
>
> buf->size = max_t(size_t, offset, buf->size);
> @@ -887,7 +972,8 @@ static const struct attribute_group *fw_dev_attr_groups[] = {
>
> static struct firmware_priv *
> fw_create_instance(struct firmware *firmware, const char *fw_name,
> - struct device *device, unsigned int opt_flags)
> + struct device *device, unsigned int opt_flags,
> + struct firmware_dma_desc *dma)
> {
> struct firmware_priv *fw_priv;
> struct device *f_dev;
> @@ -900,6 +986,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
>
> fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
> fw_priv->fw = firmware;
> + fw_priv->dma = dma;
> f_dev = &fw_priv->dev;
>
> device_initialize(f_dev);
> @@ -920,7 +1007,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
> struct firmware_buf *buf = fw_priv->buf;
>
> /* fall back on userspace loading */
> - buf->is_paged_buf = true;
> + if (!fw_priv->dma)
> + buf->is_paged_buf = true;
>
> dev_set_uevent_suppress(f_dev, true);
>
> @@ -955,7 +1043,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>
> if (is_fw_load_aborted(buf))
> retval = -EAGAIN;
> - else if (!buf->data)
> + else if (buf->is_paged_buf && !buf->data)
> retval = -ENOMEM;
>
> device_del(f_dev);
> @@ -966,11 +1054,12 @@ err_put_dev:
>
> static int fw_load_from_user_helper(struct firmware *firmware,
> const char *name, struct device *device,
> - unsigned int opt_flags, long timeout)
> + unsigned int opt_flags, long timeout,
> + struct firmware_dma_desc *dma)
> {
> struct firmware_priv *fw_priv;
>
> - fw_priv = fw_create_instance(firmware, name, device, opt_flags);
> + fw_priv = fw_create_instance(firmware, name, device, opt_flags, dma);
> if (IS_ERR(fw_priv))
> return PTR_ERR(fw_priv);
>
> @@ -998,7 +1087,7 @@ static void kill_requests_without_uevent(void)
> static inline int
> fw_load_from_user_helper(struct firmware *firmware, const char *name,
> struct device *device, unsigned int opt_flags,
> - long timeout)
> + long timeout, struct firmware_dma_desc *dma)
> {
> return -ENOENT;
> }
> @@ -1119,7 +1208,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
> /* called from request_firmware() and request_firmware_work_func() */
> static int
> _request_firmware(const struct firmware **firmware_p, const char *name,
> - struct device *device, unsigned int opt_flags)
> + struct device *device, struct firmware_dma_desc *dma,
> + unsigned int opt_flags)
> {
> struct firmware *fw = NULL;
> long timeout;
> @@ -1156,7 +1246,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> }
> }
>
> - ret = fw_get_filesystem_firmware(device, fw->priv);
> + ret = fw_get_filesystem_firmware(device, fw->priv, dma);
> if (ret) {
> if (!(opt_flags & FW_OPT_NO_WARN))
> dev_warn(device,
> @@ -1165,7 +1255,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> if (opt_flags & FW_OPT_USERHELPER) {
> dev_warn(device, "Falling back to user helper\n");
> ret = fw_load_from_user_helper(fw, name, device,
> - opt_flags, timeout);
> + opt_flags, timeout,
> + dma);
> }
> }
>
> @@ -1212,7 +1303,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
>
> /* Need to pin this module until return */
> __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware_p, name, device,
> + ret = _request_firmware(firmware_p, name, device, NULL,
> FW_OPT_UEVENT | FW_OPT_FALLBACK);
> module_put(THIS_MODULE);
> return ret;
> @@ -1236,7 +1327,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
> int ret;
>
> __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware_p, name, device,
> + ret = _request_firmware(firmware_p, name, device, NULL,
> FW_OPT_UEVENT | FW_OPT_NO_WARN);
> module_put(THIS_MODULE);
> return ret;
> @@ -1244,6 +1335,47 @@ int request_firmware_direct(const struct firmware **firmware_p,
> EXPORT_SYMBOL_GPL(request_firmware_direct);
>
> /**
> + * request_firmware_dma - load firmware into a DMA allocated buffer
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded and DMA region allocated
> + * @cpu_addr: address returned from dma_alloc_*()
> + * @dma_addr: address returned from dma_alloc_*()
> + * @offset: offset into DMA buffer to copy firmware into
> + * @size: size of DMA buffer
> + * @attrs: attributes used during DMA allocation time
> + *
> + * This function works pretty much like request_firmware(), but it doesn't
> + * load the firmware into @firmware_p's data member. Instead, the firmware
> + * is loaded directly into the buffer pointed to by @cpu_addr and @dma_addr.
> + * This function doesn't cache firmware either.
> + */
> +int
> +request_firmware_dma(const struct firmware **firmware_p, const char *name,
> + struct device *device, void *cpu_addr, dma_addr_t dma_addr,
> + unsigned long offset, size_t size, struct dma_attrs *attrs)
> +{
> + int ret;
> + struct firmware_dma_desc dma = {
> + .dev = device,
> + .cpu_addr = cpu_addr,
> + .dma_addr = dma_addr,
> + .offset = offset,
> + .size = size,
> + .attrs = attrs,
> + };
> +
> + /* Need to pin this module until return */
> + __module_get(THIS_MODULE);
> + ret = _request_firmware(firmware_p, name, device, &dma,
> + FW_OPT_UEVENT | FW_OPT_FALLBACK |
> + FW_OPT_NOCACHE);
> + module_put(THIS_MODULE);
> + return ret;
> +}
> +EXPORT_SYMBOL(request_firmware_dma);
> +
> +/**
> * release_firmware: - release the resource associated with a firmware image
> * @fw: firmware resource to release
> **/
> @@ -1275,7 +1407,7 @@ static void request_firmware_work_func(struct work_struct *work)
>
> fw_work = container_of(work, struct firmware_work, work);
>
> - _request_firmware(&fw, fw_work->name, fw_work->device,
> + _request_firmware(&fw, fw_work->name, fw_work->device, NULL,
> fw_work->opt_flags);
> fw_work->cont(fw, fw_work->context);
> put_device(fw_work->device); /* taken in request_firmware_nowait() */
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5c41c5e75b5c..06bc8d5965ca 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -19,6 +19,7 @@ struct firmware {
>
> struct module;
> struct device;
> +struct dma_attrs;
>
> struct builtin_fw {
> char *name;
> @@ -47,6 +48,10 @@ int request_firmware_nowait(
> void (*cont)(const struct firmware *fw, void *context));
> int request_firmware_direct(const struct firmware **fw, const char *name,
> struct device *device);
> +int request_firmware_dma(const struct firmware **firmware_p, const char *name,
> + struct device *device, void *cpu_addr, dma_addr_t dma_addr,
> + unsigned long offset, size_t size,
> + struct dma_attrs *attrs);
>
> void release_firmware(const struct firmware *fw);
> #else
> @@ -75,5 +80,13 @@ static inline int request_firmware_direct(const struct firmware **fw,
> return -EINVAL;
> }
>
> +static inline int request_firmware_dma(const struct firmware **firmware_p,
> + const char *name, struct device *device, void *cpu_addr,
> + dma_addr_t dma_addr, unsigned long offset, size_t size,
> + struct dma_attrs *attrs)
> +{
> + return -EINVAL;
> +}
> +
> #endif
> #endif
> --
> 2.7.0.25.gfc10eb5
>
--
Ming Lei
Powered by blists - more mailing lists