| 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: <alpine.DEB.2.22.394.2507081150230.605088@ubuntu-linux-20-04-desktop>
Date: Tue, 8 Jul 2025 12:01:22 -0700 (PDT)
From: Stefano Stabellini <sstabellini@...nel.org>
To: Juergen Gross <jgross@...e.com>
cc: linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
Stefano Stabellini <sstabellini@...nel.org>,
Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
Nathan Chancellor <nathan@...nel.org>,
Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
xen-devel@...ts.xenproject.org,
Abinash Singh <abinashsinghlalotra@...il.com>
Subject: Re: [PATCH] xen/gntdev: remove struct gntdev_copy_batch from stack
On Thu, 3 Jul 2025, Juergen Gross wrote:
> When compiling the kernel with LLVM, the following warning was issued:
>
> drivers/xen/gntdev.c:991: warning: stack frame size (1160) exceeds
> limit (1024) in function 'gntdev_ioctl'
>
> The main reason is struct gntdev_copy_batch which is located on the
> stack and has a size of nearly 1kb.
>
> For performance reasons it shouldn't by just dynamically allocated
> instead, so allocate a new instance when needed and instead of freeing
> it put it into a list of free structs anchored in struct gntdev_priv.
>
> Fixes: a4cdb556cae0 ("xen/gntdev: add ioctl for grant copy")
> Reported-by: Abinash Singh <abinashsinghlalotra@...il.com>
> Signed-off-by: Juergen Gross <jgross@...e.com>
> ---
> drivers/xen/gntdev-common.h | 4 +++
> drivers/xen/gntdev.c | 71 ++++++++++++++++++++++++++-----------
> 2 files changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
> index 9c286b2a1900..ac8ce3179ba2 100644
> --- a/drivers/xen/gntdev-common.h
> +++ b/drivers/xen/gntdev-common.h
> @@ -26,6 +26,10 @@ struct gntdev_priv {
> /* lock protects maps and freeable_maps. */
> struct mutex lock;
>
> + /* Free instances of struct gntdev_copy_batch. */
> + struct gntdev_copy_batch *batch;
> + struct mutex batch_lock;
> +
> #ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> /* Device for which DMA memory is allocated. */
> struct device *dma_dev;
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 61faea1f0663..1f2160765618 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -56,6 +56,18 @@ MODULE_AUTHOR("Derek G. Murray <Derek.Murray@...cam.ac.uk>, "
> "Gerd Hoffmann <kraxel@...hat.com>");
> MODULE_DESCRIPTION("User-space granted page access driver");
>
> +#define GNTDEV_COPY_BATCH 16
> +
> +struct gntdev_copy_batch {
> + struct gnttab_copy ops[GNTDEV_COPY_BATCH];
> + struct page *pages[GNTDEV_COPY_BATCH];
> + s16 __user *status[GNTDEV_COPY_BATCH];
> + unsigned int nr_ops;
> + unsigned int nr_pages;
> + bool writeable;
> + struct gntdev_copy_batch *next;
> +};
> +
> static unsigned int limit = 64*1024;
> module_param(limit, uint, 0644);
> MODULE_PARM_DESC(limit,
> @@ -584,6 +596,8 @@ static int gntdev_open(struct inode *inode, struct file *flip)
> INIT_LIST_HEAD(&priv->maps);
> mutex_init(&priv->lock);
>
> + mutex_init(&priv->batch_lock);
> +
> #ifdef CONFIG_XEN_GNTDEV_DMABUF
> priv->dmabuf_priv = gntdev_dmabuf_init(flip);
> if (IS_ERR(priv->dmabuf_priv)) {
> @@ -608,6 +622,7 @@ static int gntdev_release(struct inode *inode, struct file *flip)
> {
> struct gntdev_priv *priv = flip->private_data;
> struct gntdev_grant_map *map;
> + struct gntdev_copy_batch *batch;
>
> pr_debug("priv %p\n", priv);
>
> @@ -620,6 +635,14 @@ static int gntdev_release(struct inode *inode, struct file *flip)
> }
> mutex_unlock(&priv->lock);
>
> + mutex_lock(&priv->batch_lock);
> + while (priv->batch) {
> + batch = priv->batch;
> + priv->batch = batch->next;
> + kfree(batch);
> + }
> + mutex_unlock(&priv->batch_lock);
> +
> #ifdef CONFIG_XEN_GNTDEV_DMABUF
> gntdev_dmabuf_fini(priv->dmabuf_priv);
> #endif
> @@ -785,17 +808,6 @@ static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
> return rc;
> }
>
> -#define GNTDEV_COPY_BATCH 16
> -
> -struct gntdev_copy_batch {
> - struct gnttab_copy ops[GNTDEV_COPY_BATCH];
> - struct page *pages[GNTDEV_COPY_BATCH];
> - s16 __user *status[GNTDEV_COPY_BATCH];
> - unsigned int nr_ops;
> - unsigned int nr_pages;
> - bool writeable;
> -};
> -
> static int gntdev_get_page(struct gntdev_copy_batch *batch, void __user *virt,
> unsigned long *gfn)
> {
> @@ -953,36 +965,53 @@ static int gntdev_grant_copy_seg(struct gntdev_copy_batch *batch,
> static long gntdev_ioctl_grant_copy(struct gntdev_priv *priv, void __user *u)
> {
> struct ioctl_gntdev_grant_copy copy;
> - struct gntdev_copy_batch batch;
> + struct gntdev_copy_batch *batch;
> unsigned int i;
> int ret = 0;
>
> if (copy_from_user(©, u, sizeof(copy)))
> return -EFAULT;
>
> - batch.nr_ops = 0;
> - batch.nr_pages = 0;
> + mutex_lock(&priv->batch_lock);
> + if (!priv->batch) {
> + batch = kmalloc(sizeof(*batch), GFP_KERNEL);
> + } else {
> + batch = priv->batch;
> + priv->batch = batch->next;
> + }
> + mutex_unlock(&priv->batch_lock);
I am concerned about the potentially unbounded amount of memory that
could be allocated this way.
The mutex is already a potentially very slow operation. Could we instead
allocate a single batch, and if it is currently in use, use the mutex to
wait until it becomes available?
I am also OK with the current approach but I thought I would ask.
> + if (!batch)
> + return -ENOMEM;
> +
> + batch->nr_ops = 0;
> + batch->nr_pages = 0;
>
> for (i = 0; i < copy.count; i++) {
> struct gntdev_grant_copy_segment seg;
>
> if (copy_from_user(&seg, ©.segments[i], sizeof(seg))) {
> ret = -EFAULT;
> + gntdev_put_pages(batch);
> goto out;
> }
>
> - ret = gntdev_grant_copy_seg(&batch, &seg, ©.segments[i].status);
> - if (ret < 0)
> + ret = gntdev_grant_copy_seg(batch, &seg, ©.segments[i].status);
> + if (ret < 0) {
> + gntdev_put_pages(batch);
> goto out;
> + }
>
> cond_resched();
> }
> - if (batch.nr_ops)
> - ret = gntdev_copy(&batch);
> - return ret;
> + if (batch->nr_ops)
> + ret = gntdev_copy(batch);
> +
> + out:
> + mutex_lock(&priv->batch_lock);
> + batch->next = priv->batch;
> + priv->batch = batch;
> + mutex_unlock(&priv->batch_lock);
>
> - out:
> - gntdev_put_pages(&batch);
One change from before is that in case of no errors, gntdev_put_pages is
not called anymore. Do we want that? Specifically, we are missing the
call to unpin_user_pages_dirty_lock
> return ret;
> }
>
> --
> 2.43.0
>
Powered by blists - more mailing lists