| 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: <1a82dcb5-2ccf-4c8b-b0a4-fd055cfe99f1@suse.com>
Date: Fri, 11 Jul 2025 09:16:50 +0200
From: Jürgen Groß <jgross@...e.com>
To: Stefano Stabellini <sstabellini@...nel.org>
Cc: linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
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 11.07.25 03:03, Stefano Stabellini wrote:
> On Wed, 9 Jul 2025, Jürgen Groß wrote:
>> On 08.07.25 21:01, Stefano Stabellini wrote:
>>> 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.
>>
>> Unbounded? It can be at most the number of threads using the interface
>> concurrently.
>
> That's what I meant
1 kB additional memory per thread won't be the end of the world.
Each thread will consume much more memory for other purposes anyway.
>>> 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?
>>
>> As this interface is e.g. used by the qemu based qdisk backend, the chances
>> are very high that there are concurrent users. This would hurt multi-ring
>> qdisk quite badly!
>>
>> It would be possible to replace the mutex with a spinlock and do the kmalloc()
>> outside the locked region.
>>
>>>
>>> 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
>>
>> I don't think you are right. There was a "return ret" before the "out:"
>> label before my patch.
>
> You are right, I missed it.
>
> Reviewed-by: Stefano Stabellini <sstabellini@...nel.org>
Thanks,
Juergen
Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)
Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)
Powered by blists - more mailing lists