From: Michel Dänzer <mdaenzer(a)redhat.com>
This makes sure we don't hit the
BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
in dma_buf_release, which could be triggered by user space closing the
dma-buf file description while there are outstanding fence callbacks
from dma_buf_poll.
Cc: stable(a)vger.kernel.org
Signed-off-by: Michel Dänzer <mdaenzer(a)redhat.com>
---
drivers/dma-buf/dma-buf.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 6c520c9bd93c..ec25498a971f 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -65,12 +65,9 @@ static void dma_buf_release(struct dentry *dentry)
BUG_ON(dmabuf->vmapping_counter);
/*
- * Any fences that a dma-buf poll can wait on should be signaled
- * before releasing dma-buf. This is the responsibility of each
- * driver that uses the reservation objects.
- *
- * If you hit this BUG() it means someone dropped their ref to the
- * dma-buf while still having pending operation to the buffer.
+ * If you hit this BUG() it could mean:
+ * * There's a file reference imbalance in dma_buf_poll / dma_buf_poll_cb or somewhere else
+ * * dmabuf->cb_in/out.active are non-0 despite no pending fence callback
*/
BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
@@ -196,6 +193,7 @@ static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence)
static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
{
struct dma_buf_poll_cb_t *dcb = (struct dma_buf_poll_cb_t *)cb;
+ struct dma_buf *dmabuf = container_of(dcb->poll, struct dma_buf, poll);
unsigned long flags;
spin_lock_irqsave(&dcb->poll->lock, flags);
@@ -203,6 +201,8 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
dcb->active = 0;
spin_unlock_irqrestore(&dcb->poll->lock, flags);
dma_fence_put(fence);
+ /* Paired with get_file in dma_buf_poll */
+ fput(dmabuf->file);
}
static bool dma_buf_poll_shared(struct dma_resv *resv,
@@ -278,6 +278,9 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
spin_unlock_irq(&dmabuf->poll.lock);
if (events & EPOLLOUT) {
+ /* Paired with fput in dma_buf_poll_cb */
+ get_file(dmabuf->file);
+
if (!dma_buf_poll_shared(resv, dcb) &&
!dma_buf_poll_excl(resv, dcb))
/* No callback queued, wake up any other waiters */
@@ -299,6 +302,9 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
spin_unlock_irq(&dmabuf->poll.lock);
if (events & EPOLLIN) {
+ /* Paired with fput in dma_buf_poll_cb */
+ get_file(dmabuf->file);
+
if (!dma_buf_poll_excl(resv, dcb))
/* No callback queued, wake up any other waiters */
dma_buf_poll_cb(NULL, &dcb->cb);
--
2.32.0
Am 29.10.21 um 04:15 schrieb guangming.cao(a)mediatek.com:
> From: Guangming Cao <Guangming.Cao(a)mediatek.com>
>
> On Fri, 2021-10-08 at 12:24 +0200, Christian König wrote:
>> Am 08.10.21 um 09:54 schrieb guangming.cao(a)mediatek.com:
>>> From: Guangming Cao <Guangming.Cao(a)mediatek.com>
>>>
>>> Because dma-buf.name can be freed in func: "dma_buf_set_name",
>>> so, we need to acquire lock first before we read/write dma_buf.name
>>> to prevent Use After Free(UAF) issue.
>>>
>>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com>
>> Reviewed-by: Christian König <christian.koenig(a)amd.com>
>>
>> Going to push that upstream if nobody else objects.
>>
>> Thanks,
>> Christian.
> Just a gentle ping for this patch, please kindly let me know how is it going.
Ah, yes. Thanks for the reminder.
I've just pushed this to drm-misc-fixes.
Christian.
>
>>> ---
>>> drivers/dma-buf/dma-buf.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index 511fe0d217a0..a7f6fd13a635 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -1372,6 +1372,8 @@ static int dma_buf_debug_show(struct seq_file
>>> *s, void *unused)
>>> if (ret)
>>> goto error_unlock;
>>>
>>> +
>>> + spin_lock(&buf_obj->name_lock);
>>> seq_printf(s,
>>> "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n",
>>> buf_obj->size,
>>> buf_obj->file->f_flags, buf_obj->file-
>>>> f_mode,
>>> @@ -1379,6 +1381,7 @@ static int dma_buf_debug_show(struct seq_file
>>> *s, void *unused)
>>> buf_obj->exp_name,
>>> file_inode(buf_obj->file)->i_ino,
>>> buf_obj->name ?: "");
>>> + spin_unlock(&buf_obj->name_lock);
>>>
>>> robj = buf_obj->resv;
>>> fence = dma_resv_excl_fence(robj);
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek(a)lists.infradead.org
>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr…
Hi guys,
a few more bug fixes, looks like the more selftests I add the more odies I find.
Assuming the CI tests now pass I will start pushing patches I've already got an rb for to drm-misc-next.
Please review and/or comment,
Christian.
Am 26.10.21 um 13:52 schrieb guangming.cao(a)mediatek.com:
> From: Guangming Cao <Guangming.Cao(a)mediatek.com>
>
> On Tue, 2021-10-26 at 13:18 +0200, Christian König wrote:
>> Am 14.10.21 um 12:25 schrieb guangming.cao(a)mediatek.com:
>>> From: Guangming Cao <Guangming.Cao(a)mediatek.com>
>>>
>>> In this patch(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat…;reserved=0),
>>> it add a new IOCTL to support dma-buf user to set debug name.
>>>
>>> But it also added a limitation of this IOCTL, it needs the
>>> attachments of dmabuf should be empty, otherwise it will fail.
>>>
>>> For the original series, the idea was that allowing name change
>>> mid-use could confuse the users about the dma-buf.
>>> However, the rest of the series also makes sure each dma-buf have a
>>> unique
>>> inode(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat…;reserved=0), and any
>>> accounting
>>> should probably use that, without relying on the name as much.
>>>
>>> So, removing this restriction will let dma-buf userspace users to
>>> use it
>>> more comfortably and without any side effect.
>>>
>>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com>
>> We could now cleanup the return value from dma_buf_set_name() into a
>> void since that function can't fail any more as far as I can see.
>>
>> But that isn't mandatory I think, patch is Reviewed-by: Christian
>> König
>> <christian.koenig(a)amd.com>
>>
> So, here is no need to check return value of 'strndup_user',
> just return without error code if the almost impossible error occurs?
Good point, totally missed that one.
In that case I'm going to push the patch to drm-misc-next as is.
Regards,
Christian.
>
> Guangming.
>
>> Regards,
>> Christian.
>>
>>> ---
>>> drivers/dma-buf/dma-buf.c | 17 +++--------------
>>> 1 file changed, 3 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index 511fe0d217a0..5fbb3a2068a3 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file
>>> *file, poll_table *poll)
>>>
>>> /**
>>> * dma_buf_set_name - Set a name to a specific dma_buf to track
>>> the usage.
>>> - * The name of the dma-buf buffer can only be set when the dma-buf
>>> is not
>>> - * attached to any devices. It could theoritically support
>>> changing the
>>> - * name of the dma-buf if the same piece of memory is used for
>>> multiple
>>> - * purpose between different devices.
>>> + * It could support changing the name of the dma-buf if the same
>>> + * piece of memory is used for multiple purpose between different
>>> devices.
>>> *
>>> * @dmabuf: [in] dmabuf buffer that will be renamed.
>>> * @buf: [in] A piece of userspace memory that contains
>>> the name of
>>> @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file
>>> *file, poll_table *poll)
>>> static long dma_buf_set_name(struct dma_buf *dmabuf, const char
>>> __user *buf)
>>> {
>>> char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
>>> - long ret = 0;
>>>
>>> if (IS_ERR(name))
>>> return PTR_ERR(name);
>>>
>>> - dma_resv_lock(dmabuf->resv, NULL);
>>> - if (!list_empty(&dmabuf->attachments)) {
>>> - ret = -EBUSY;
>>> - kfree(name);
>>> - goto out_unlock;
>>> - }
>>> spin_lock(&dmabuf->name_lock);
>>> kfree(dmabuf->name);
>>> dmabuf->name = name;
>>> spin_unlock(&dmabuf->name_lock);
>>>
>>> -out_unlock:
>>> - dma_resv_unlock(dmabuf->resv);
>>> - return ret;
>>> + return 0;
>>> }
>>>
>>> static long dma_buf_ioctl(struct file *file,
>>
Am 14.10.21 um 12:25 schrieb guangming.cao(a)mediatek.com:
> From: Guangming Cao <Guangming.Cao(a)mediatek.com>
>
> In this patch(https://patchwork.freedesktop.org/patch/310349),
> it add a new IOCTL to support dma-buf user to set debug name.
>
> But it also added a limitation of this IOCTL, it needs the
> attachments of dmabuf should be empty, otherwise it will fail.
>
> For the original series, the idea was that allowing name change
> mid-use could confuse the users about the dma-buf.
> However, the rest of the series also makes sure each dma-buf have a unique
> inode(https://patchwork.freedesktop.org/patch/310387/), and any accounting
> should probably use that, without relying on the name as much.
>
> So, removing this restriction will let dma-buf userspace users to use it
> more comfortably and without any side effect.
>
> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com>
We could now cleanup the return value from dma_buf_set_name() into a
void since that function can't fail any more as far as I can see.
But that isn't mandatory I think, patch is Reviewed-by: Christian König
<christian.koenig(a)amd.com>
Regards,
Christian.
> ---
> drivers/dma-buf/dma-buf.c | 17 +++--------------
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 511fe0d217a0..5fbb3a2068a3 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
>
> /**
> * dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
> - * The name of the dma-buf buffer can only be set when the dma-buf is not
> - * attached to any devices. It could theoritically support changing the
> - * name of the dma-buf if the same piece of memory is used for multiple
> - * purpose between different devices.
> + * It could support changing the name of the dma-buf if the same
> + * piece of memory is used for multiple purpose between different devices.
> *
> * @dmabuf: [in] dmabuf buffer that will be renamed.
> * @buf: [in] A piece of userspace memory that contains the name of
> @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
> {
> char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
> - long ret = 0;
>
> if (IS_ERR(name))
> return PTR_ERR(name);
>
> - dma_resv_lock(dmabuf->resv, NULL);
> - if (!list_empty(&dmabuf->attachments)) {
> - ret = -EBUSY;
> - kfree(name);
> - goto out_unlock;
> - }
> spin_lock(&dmabuf->name_lock);
> kfree(dmabuf->name);
> dmabuf->name = name;
> spin_unlock(&dmabuf->name_lock);
>
> -out_unlock:
> - dma_resv_unlock(dmabuf->resv);
> - return ret;
> + return 0;
> }
>
> static long dma_buf_ioctl(struct file *file,
Am 26.10.21 um 10:34 schrieb Arnd Bergmann:
> From: Arnd Bergmann <arnd(a)arndb.de>
>
> The new driver incorrectly unwinds after errors, as clang points out:
>
> drivers/dma-buf/st-dma-resv.c:295:7: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> if (r) {
> ^
> drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here
> while (i--)
> ^
> drivers/dma-buf/st-dma-resv.c:295:3: note: remove the 'if' if its condition is always false
> if (r) {
> ^~~~~~~~
> drivers/dma-buf/st-dma-resv.c:288:6: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> if (r) {
> ^
> drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here
> while (i--)
> ^
> drivers/dma-buf/st-dma-resv.c:288:2: note: remove the 'if' if its condition is always false
> if (r) {
> ^~~~~~~~
> drivers/dma-buf/st-dma-resv.c:280:10: note: initialize the variable 'i' to silence this warning
> int r, i;
> ^
> = 0
>
> Skip cleaning up the bits that have not been allocated at this point.
>
> Fixes: 1d51775cd3f5 ("dma-buf: add dma_resv selftest v4")
> Signed-off-by: Arnd Bergmann <arnd(a)arndb.de>
I already send out a patch to fix this up, but forgot to fix both gotos.
Going to add my rb and using that one here instead.
Thanks,
Christian.
> ---
> I'm not familiar with these interfaces, so I'm just guessing where
> we should jump after an error, please double-check and fix if necessary.
> ---
> drivers/dma-buf/st-dma-resv.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/st-dma-resv.c b/drivers/dma-buf/st-dma-resv.c
> index 6f3ba756da3e..bc32b3eedcb6 100644
> --- a/drivers/dma-buf/st-dma-resv.c
> +++ b/drivers/dma-buf/st-dma-resv.c
> @@ -287,7 +287,7 @@ static int test_get_fences(void *arg, bool shared)
> r = dma_resv_lock(&resv, NULL);
> if (r) {
> pr_err("Resv locking failed\n");
> - goto err_free;
> + goto err_resv;
> }
>
> if (shared) {
> @@ -295,7 +295,7 @@ static int test_get_fences(void *arg, bool shared)
> if (r) {
> pr_err("Resv shared slot allocation failed\n");
> dma_resv_unlock(&resv);
> - goto err_free;
> + goto err_resv;
> }
>
> dma_resv_add_shared_fence(&resv, f);
> @@ -336,6 +336,7 @@ static int test_get_fences(void *arg, bool shared)
> while (i--)
> dma_fence_put(fences[i]);
> kfree(fences);
> +err_resv:
> dma_resv_fini(&resv);
> dma_fence_put(f);
> return r;
Hi,
On 10/23/21 4:14 AM, Flora Fu wrote:
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index d9bac2710494..074b0cf24c44 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -24,6 +24,24 @@ config MTK_APU_PM
> APU power domain shall be enabled before accessing the
> internal sub modules.
>
> +config MTK_APU
> + tristate "MediaTek APUSYS Support"
> + select REGMAP
> + select MTK_APU_PM
> + select MTK_SCP
> + help
> + Say yes here to add support for the APU tinysys. The tinsys is
tinysys runs on
> + running on a micro processor in APU.
a microprocessor in the APU.
> + Its firmware is load and boot from Kernel side. Kernel and tinysys use
is loaded and booted
> + IPI to tx/rx messages.
to send/receive messages.
> +
> +config MTK_APU_DEBUG
> + tristate "MediaTek APUSYS debug functions"
> + depends on MTK_APU
> + help
> + Say yes here to enalbe debug on APUSYS.
enable
> + Disable it if you don't need them.
--
~Randy
On Tue, Oct 19, 2021 at 08:23:45PM +0800, guangming.cao(a)mediatek.com wrote:
> From: Guangming Cao <Guangming.Cao(a)mediatek.com>
>
> Since there is no mandatory inspection for attachments in dma_buf_release.
> There will be a case that dma_buf already released but attachment is still
> in use, which can points to the dmabuf, and it maybe cause
> some unexpected issues.
>
> With IOMMU, when this cases occurs, there will have IOMMU address
> translation fault(s) followed by this warning,
> I think it's useful for dma devices to debug issue.
>
> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com>
This feels a lot like hand-rolling kobject debugging. If you want to do
this then I think adding kobject debug support to
dma_buf/dma_buf_attachment would be better than hand-rolling something
bespoke here.
Also on the patch itself: You don't need the trylock. For correctly
working code non one else can get at the dma-buf, so no locking needed to
iterate through the attachment list. For incorrect code the kernel will be
on fire pretty soon anyway, trying to do locking won't help :-) And
without the trylock we can catch more bugs (e.g. if you also forgot to
unlock and not just forgot to detach).
-Daniel
> ---
> drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 511fe0d217a0..672404857d6a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -74,6 +74,29 @@ static void dma_buf_release(struct dentry *dentry)
> */
> BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active);
>
> + /* attachment check */
> + if (dma_resv_trylock(dmabuf->resv) && WARN(!list_empty(&dmabuf->attachments),
> + "%s err, inode:%08lu size:%08zu name:%s exp_name:%s flags:0x%08x mode:0x%08x, %s\n",
> + __func__, file_inode(dmabuf->file)->i_ino, dmabuf->size,
> + dmabuf->name, dmabuf->exp_name,
> + dmabuf->file->f_flags, dmabuf->file->f_mode,
> + "Release dmabuf before detach all attachments, dump attach:\n")) {
> + int attach_cnt = 0;
> + dma_addr_t dma_addr;
> + struct dma_buf_attachment *attach_obj;
> + /* dump all attachment info */
> + list_for_each_entry(attach_obj, &dmabuf->attachments, node) {
> + dma_addr = (dma_addr_t)0;
> + if (attach_obj->sgt)
> + dma_addr = sg_dma_address(attach_obj->sgt->sgl);
> + pr_err("attach[%d]: dev:%s dma_addr:0x%-12lx\n",
> + attach_cnt, dev_name(attach_obj->dev), dma_addr);
> + attach_cnt++;
> + }
> + pr_err("Total %d devices attached\n\n", attach_cnt);
> + dma_resv_unlock(dmabuf->resv);
> + }
> +
> dmabuf->ops->release(dmabuf);
>
> if (dmabuf->resv == (struct dma_resv *)&dmabuf[1])
> --
> 2.17.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch