LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "Ørjan Eide" <orjan.eide@arm.com>
To: unlisted-recipients:; (no To-header on input)
Cc: orjan.eide@arm.com, anders.pedersen@arm.com,
	john.stultz@linaro.org, "Laura Abbott" <labbott@redhat.com>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Arve Hjønnevåg" <arve@android.com>,
	"Todd Kjos" <tkjos@android.com>,
	"Martijn Coenen" <maco@android.com>,
	"Joel Fernandes" <joel@joelfernandes.org>,
	"Christian Brauner" <christian@brauner.io>,
	"Daniel Vetter" <daniel.vetter@ffwll.ch>,
	"Darren Hart (VMware)" <dvhart@infradead.org>,
	"Lecopzer Chen" <lecopzer.chen@mediatek.com>,
	"Arnd Bergmann" <arnd@arndb.de>,
	devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org,
	linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org
Subject: [PATCH] staging: android: ion: Skip sync if not mapped
Date: Tue, 14 Apr 2020 15:46:27 +0200
Message-ID: <20200414134629.54567-1-orjan.eide@arm.com> (raw)

Only sync the sg-list of an Ion dma-buf attachment when the attachment
is actually mapped on the device.

dma-bufs may be synced at any time. It can be reached from user space
via DMA_BUF_IOCTL_SYNC, so there are no guarantees from callers on when
syncs may be attempted, and dma_buf_end_cpu_access() and
dma_buf_begin_cpu_access() may not be paired.

Since the sg_list's dma_address isn't set up until the buffer is used
on the device, and dma_map_sg() is called on it, the dma_address will be
NULL if sync is attempted on the dma-buf before it's mapped on a device.

Before v5.0 (commit 55897af63091 ("dma-direct: merge swiotlb_dma_ops
into the dma_direct code")) this was a problem as the dma-api (at least
the swiotlb_dma_ops on arm64) would use the potentially invalid
dma_address. How that failed depended on how the device handled physical
address 0. If 0 was a valid address to physical ram, that page would get
flushed a lot, while the actual pages in the buffer would not get synced
correctly. While if 0 is an invalid physical address it may cause a
fault and trigger a crash.

In v5.0 this was incidentally fixed by commit 55897af63091 ("dma-direct:
merge swiotlb_dma_ops into the dma_direct code"), as this moved the
dma-api to use the page pointer in the sg_list, and (for Ion buffers at
least) this will always be valid if the sg_list exists at all.

But, this issue is re-introduced in v5.3 with
commit 449fa54d6815 ("dma-direct: correct the physical addr in
dma_direct_sync_sg_for_cpu/device") moves the dma-api back to the old
behaviour and picks the dma_address that may be invalid.

dma-buf core doesn't ensure that the buffer is mapped on the device, and
thus have a valid sg_list, before calling the exporter's
begin_cpu_access.

Signed-off-by: Ørjan Eide <orjan.eide@arm.com>
---
 drivers/staging/android/ion/ion.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

This seems to be part of a bigger issue where dma-buf exporters assume
that their dma-buf begin_cpu_access and end_cpu_access callbacks have a
certain guaranteed behavior, which isn't ensured by dma-buf core.

This patch fixes this in ion only, but it also needs to be fixed for
other exporters, either handled like this in each exporter, or in
dma-buf core before calling into the exporters.

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 38b51eace4f9..7b752ba0cb6d 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -173,6 +173,7 @@ struct ion_dma_buf_attachment {
        struct device *dev;
        struct sg_table *table;
        struct list_head list;
+       bool mapped:1;
 };

 static int ion_dma_buf_attach(struct dma_buf *dmabuf,
@@ -195,6 +196,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf,
        a->table = table;
        a->dev = attachment->dev;
        INIT_LIST_HEAD(&a->list);
+       a->mapped = false;

        attachment->priv = a;

@@ -231,6 +233,8 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
                        direction))
                return ERR_PTR(-ENOMEM);

+       a->mapped = true;
+
        return table;
 }

@@ -238,6 +242,10 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
                              struct sg_table *table,
                              enum dma_data_direction direction)
 {
+       struct ion_dma_buf_attachment *a = attachment->priv;
+
+       a->mapped = false;
+
        dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
 }

@@ -297,6 +305,8 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,

        mutex_lock(&buffer->lock);
        list_for_each_entry(a, &buffer->attachments, list) {
+               if (!a->mapped)
+                       continue;
                dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
                                    direction);
        }
@@ -320,6 +330,8 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,

        mutex_lock(&buffer->lock);
        list_for_each_entry(a, &buffer->attachments, list) {
+               if (!a->mapped)
+                       continue;
                dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
                                       direction);
        }
--
2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

             reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-14 13:46 Ørjan Eide [this message]
2020-04-14 14:04 ` Greg Kroah-Hartman
2020-04-14 14:18 ` Ørjan Eide
2020-04-14 14:28   ` Greg Kroah-Hartman
2020-04-14 16:11     ` Ørjan Eide
2020-04-15  5:16       ` John Stultz
2020-04-15  4:41     ` John Stultz
2020-04-16 10:25       ` Greg Kroah-Hartman
2020-04-17 15:00         ` Daniel Vetter
2020-04-20  7:53           ` Christoph Hellwig
2020-04-20  8:22         ` Christian Brauner
2020-04-20 20:03           ` John Stultz
2020-04-21  8:05             ` Greg Kroah-Hartman
2020-07-03  7:04               ` Greg Kroah-Hartman
2020-07-08  3:43                 ` John Stultz
2020-07-10 11:47                   ` Greg Kroah-Hartman
2020-04-16  9:49   ` Dan Carpenter
2020-04-16 16:25     ` Ørjan Eide
2020-04-16 17:36       ` Dan Carpenter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200414134629.54567-1-orjan.eide@arm.com \
    --to=orjan.eide@arm.com \
    --cc=anders.pedersen@arm.com \
    --cc=arnd@arndb.de \
    --cc=arve@android.com \
    --cc=christian@brauner.io \
    --cc=daniel.vetter@ffwll.ch \
    --cc=devel@driverdev.osuosl.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=dvhart@infradead.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=joel@joelfernandes.org \
    --cc=john.stultz@linaro.org \
    --cc=labbott@redhat.com \
    --cc=lecopzer.chen@mediatek.com \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=maco@android.com \
    --cc=sumit.semwal@linaro.org \
    --cc=tkjos@android.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git