Re: [Linaro-mm-sig] [PATCH v3] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
On Wed, Feb 3, 2021 at 2:25 AM Daniel Vetter wrote: > > On Mon, Feb 01, 2021 at 01:02:30PM -0800, Hridya Valsaraju wrote: > > On Mon, Feb 1, 2021 at 10:37 AM Daniel Vetter wrote: > > > > > > On Thu, Jan 28, 2021 at 1:03 PM Sumit Semwal > > > wrote: > > > > > > > > On Thu, 28 Jan 2021 at 17:23, Christian König > > > > wrote: > > > > > > > > > > Am 28.01.21 um 12:00 schrieb Sumit Semwal: > > > > > > Hi Hridya, > > > > > > > > > > > > On Wed, 27 Jan 2021 at 17:36, Greg KH > > > > > > wrote: > > > > > >> On Tue, Jan 26, 2021 at 12:42:36PM -0800, Hridya Valsaraju wrote: > > > > > >>> This patch allows statistics to be enabled for each DMA-BUF in > > > > > >>> sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. > > > > > >>> > > > > > >>> The following stats will be exposed by the interface: > > > > > >>> > > > > > >>> /sys/kernel/dmabuf/buffers//exporter_name > > > > > >>> /sys/kernel/dmabuf/buffers//size > > > > > >>> /sys/kernel/dmabuf/buffers//attachments//device > > > > > >>> /sys/kernel/dmabuf/buffers//attachments//map_counter > > > > > >>> > > > > > >>> The inode_number is unique for each DMA-BUF and was added earlier > > > > > >>> [1] > > > > > >>> in order to allow userspace to track DMA-BUF usage across > > > > > >>> different > > > > > >>> processes. > > > > > >>> > > > > > >>> Currently, this information is exposed in > > > > > >>> /sys/kernel/debug/dma_buf/bufinfo. > > > > > >>> However, since debugfs is considered unsafe to be mounted in > > > > > >>> production, > > > > > >>> it is being duplicated in sysfs. > > > > > >>> > > > > > >>> This information will be used to derive DMA-BUF > > > > > >>> per-exporter stats and per-device usage stats for Android Bug > > > > > >>> reports. > > > > > >>> The corresponding userspace changes can be found at [2]. > > > > > >>> Telemetry tools will also capture this information(along with > > > > > >>> other > > > > > >>> memory metrics) periodically as well as on important events like a > > > > > >>> foreground app kill (which might have been triggered by Low Memory > > > > > >>> Killer). It will also contribute to provide a snapshot of the > > > > > >>> system > > > > > >>> memory usage on other events such as OOM kills and Application Not > > > > > >>> Responding events. > > > > > >>> > > > > > >>> A shell script that can be run on a classic Linux environment to > > > > > >>> read > > > > > >>> out the DMA-BUF statistics can be found at [3](suggested by John > > > > > >>> Stultz). > > > > > >>> > > > > > >>> The patch contains the following improvements over the previous > > > > > >>> version: > > > > > >>> 1) Each attachment is represented by its own directory to allow > > > > > >>> creating > > > > > >>> a symlink to the importing device and to also provide room for > > > > > >>> future > > > > > >>> expansion. > > > > > >>> 2) The number of distinct mappings of each attachment is exposed > > > > > >>> in a > > > > > >>> separate file. > > > > > >>> 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers > > > > > >>> inorder to make the interface expandable in future. > > > > > >>> > > > > > >>> All of the improvements above are based on suggestions/feedback > > > > > >>> from > > > > > >>> Daniel Vetter and Christian König. > > > > > >>> > > > > > >>> [1]: https://lore.kernel.org/patchwork/patch/1088791/ > > > > > >>> [2]: > &g
Re: [Linaro-mm-sig] [PATCH v3] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
On Mon, Feb 1, 2021 at 10:37 AM Daniel Vetter wrote: > > On Thu, Jan 28, 2021 at 1:03 PM Sumit Semwal wrote: > > > > On Thu, 28 Jan 2021 at 17:23, Christian König > > wrote: > > > > > > Am 28.01.21 um 12:00 schrieb Sumit Semwal: > > > > Hi Hridya, > > > > > > > > On Wed, 27 Jan 2021 at 17:36, Greg KH > > > > wrote: > > > >> On Tue, Jan 26, 2021 at 12:42:36PM -0800, Hridya Valsaraju wrote: > > > >>> This patch allows statistics to be enabled for each DMA-BUF in > > > >>> sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. > > > >>> > > > >>> The following stats will be exposed by the interface: > > > >>> > > > >>> /sys/kernel/dmabuf/buffers//exporter_name > > > >>> /sys/kernel/dmabuf/buffers//size > > > >>> /sys/kernel/dmabuf/buffers//attachments//device > > > >>> /sys/kernel/dmabuf/buffers//attachments//map_counter > > > >>> > > > >>> The inode_number is unique for each DMA-BUF and was added earlier [1] > > > >>> in order to allow userspace to track DMA-BUF usage across different > > > >>> processes. > > > >>> > > > >>> Currently, this information is exposed in > > > >>> /sys/kernel/debug/dma_buf/bufinfo. > > > >>> However, since debugfs is considered unsafe to be mounted in > > > >>> production, > > > >>> it is being duplicated in sysfs. > > > >>> > > > >>> This information will be used to derive DMA-BUF > > > >>> per-exporter stats and per-device usage stats for Android Bug reports. > > > >>> The corresponding userspace changes can be found at [2]. > > > >>> Telemetry tools will also capture this information(along with other > > > >>> memory metrics) periodically as well as on important events like a > > > >>> foreground app kill (which might have been triggered by Low Memory > > > >>> Killer). It will also contribute to provide a snapshot of the system > > > >>> memory usage on other events such as OOM kills and Application Not > > > >>> Responding events. > > > >>> > > > >>> A shell script that can be run on a classic Linux environment to read > > > >>> out the DMA-BUF statistics can be found at [3](suggested by John > > > >>> Stultz). > > > >>> > > > >>> The patch contains the following improvements over the previous > > > >>> version: > > > >>> 1) Each attachment is represented by its own directory to allow > > > >>> creating > > > >>> a symlink to the importing device and to also provide room for future > > > >>> expansion. > > > >>> 2) The number of distinct mappings of each attachment is exposed in a > > > >>> separate file. > > > >>> 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers > > > >>> inorder to make the interface expandable in future. > > > >>> > > > >>> All of the improvements above are based on suggestions/feedback from > > > >>> Daniel Vetter and Christian König. > > > >>> > > > >>> [1]: https://lore.kernel.org/patchwork/patch/1088791/ > > > >>> [2]: > > > >>> https://android-review.googlesource.com/q/topic:%22dmabuf-sysfs%22+(status:open%20OR%20status:merged) > > > >>> [3]: > > > >>> https://android-review.googlesource.com/c/platform/system/memory/libmeminfo/+/1549734 > > > >>> > > > >>> Signed-off-by: Hridya Valsaraju > > > >>> Reported-by: kernel test robot > > > > Thanks for the patch! > > > > > > > > Christian: If you're satisfied with the explanation around not > > > > directly embedding kobjects into the dma_buf and dma_buf_attachment > > > > structs, then with Greg's r-b from sysfs PoV, I think we can merge it. > > > > Please let me know if you feel otherwise! > > > > > > From the technical side it looks clean to me, feel free to add my > > > acked-by while pushing. > > > > > > But I would at least try to convince Daniel on the design. At least some > > > of his concerns seems to be valid and keep in mind that we nee
Re: [Linaro-mm-sig] [PATCH v3] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
On Thu, Jan 28, 2021 at 6:35 AM Sumit Semwal wrote: > > Hi Simon, > > On Thu, 28 Jan 2021 at 20:01, Simon Ser wrote: > > > > On Thursday, January 28th, 2021 at 1:03 PM, Sumit Semwal > > wrote: > > > > > Since he didn't comment over Hridya's last clarification about the > > > tracepoints to track total GPU memory allocations being orthogonal to > > > this series, I assumed he agreed with it. > > > > IIRC he's away this week. (I don't remember when he comes back.) > > > > > Daniel, do you still have objections around adding this patch in? > > > > (Adding him explicitly in CC) > Thanks for doing this! > > Best, > Sumit. Thank you all for the help :) Regards, Hridya
Re: [PATCH v4 3/4] dt-bindings: reserved-memory: Make DMA-BUF CMA heap DT-configurable
On Mon, Jan 25, 2021 at 11:07 PM John Stultz wrote:
>
> On Thu, Jan 21, 2021 at 9:55 AM Minchan Kim wrote:
> > .../reserved-memory/dma_heap_chunk.yaml | 56 +++
> > 1 file changed, 56 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml
> > b/Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml
> > new file mode 100644
> > index ..00db0ae6af61
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml
> > @@ -0,0 +1,56 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/reserved-memory/dma_heap_chunk.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Device tree binding for chunk heap on DMA HEAP FRAMEWORK
> > +
> > +description: |
> > + The DMA chunk heap is backed by the Contiguous Memory Allocator (CMA) and
> > + supports bulk allocation of fixed size pages.
> > +
> > +maintainers:
> > + - Hyesoo Yu
> > + - John Stultz
> > + - Minchan Kim
> > + - Hridya Valsaraju
> > +
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - dma_heap,chunk
> > +
> > + chunk-order:
> > +description: |
> > +order of pages that will get allocated from the chunk DMA heap.
> > +maxItems: 1
> > +
> > + size:
> > +maxItems: 1
> > +
> > + alignment:
> > +maxItems: 1
> > +
> > +required:
> > + - compatible
> > + - size
> > + - alignment
> > + - chunk-order
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +reserved-memory {
> > +#address-cells = <2>;
> > +#size-cells = <1>;
> > +
> > +chunk_memory: chunk_memory {
> > +compatible = "dma_heap,chunk";
> > +size = <0x300>;
>
> Hey Minchan,
> Looking closer here, would it make more sense to document the "reg =
> <>" parameter here as well instead of just "size = <>"?
>
> That way the address of the region could be explicitly specified (for
> instance, to ensure the CMA region created is 32bit addressable). And
> more practically, trying to satisfy the base address alignment checks
> in the final patch when its set dynamically may require a fair amount
> of luck - I couldn't manage it in my own testing on the hikey960 w/o
> resorting to reg= :)
>
> It does look like the RESERVEDMEM_OF_DECLARE() logic already supports
> this, so it's likely just a matter of documenting it here?
Thank you John, yes, that makes sense. We will add the 'reg' parameter
as well when we send out the next version.
Regards,
Hridya
>
> thanks
> -john
[PATCH v3] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
This patch allows statistics to be enabled for each DMA-BUF in sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. The following stats will be exposed by the interface: /sys/kernel/dmabuf/buffers//exporter_name /sys/kernel/dmabuf/buffers//size /sys/kernel/dmabuf/buffers//attachments//device /sys/kernel/dmabuf/buffers//attachments//map_counter The inode_number is unique for each DMA-BUF and was added earlier [1] in order to allow userspace to track DMA-BUF usage across different processes. Currently, this information is exposed in /sys/kernel/debug/dma_buf/bufinfo. However, since debugfs is considered unsafe to be mounted in production, it is being duplicated in sysfs. This information will be used to derive DMA-BUF per-exporter stats and per-device usage stats for Android Bug reports. The corresponding userspace changes can be found at [2]. Telemetry tools will also capture this information(along with other memory metrics) periodically as well as on important events like a foreground app kill (which might have been triggered by Low Memory Killer). It will also contribute to provide a snapshot of the system memory usage on other events such as OOM kills and Application Not Responding events. A shell script that can be run on a classic Linux environment to read out the DMA-BUF statistics can be found at [3](suggested by John Stultz). The patch contains the following improvements over the previous version: 1) Each attachment is represented by its own directory to allow creating a symlink to the importing device and to also provide room for future expansion. 2) The number of distinct mappings of each attachment is exposed in a separate file. 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers inorder to make the interface expandable in future. All of the improvements above are based on suggestions/feedback from Daniel Vetter and Christian König. [1]: https://lore.kernel.org/patchwork/patch/1088791/ [2]: https://android-review.googlesource.com/q/topic:%22dmabuf-sysfs%22+(status:open%20OR%20status:merged) [3]: https://android-review.googlesource.com/c/platform/system/memory/libmeminfo/+/1549734 Signed-off-by: Hridya Valsaraju Reported-by: kernel test robot --- Changes in v3: Fix a warning reported by the kernel test robot. Changes in v2: -Move statistics to /sys/kernel/dmabuf/buffers in oder to allow addition of other DMA-BUF-related sysfs stats in future. Based on feedback from Daniel Vetter. -Each attachment has its own directory to represent attaching devices as symlinks and to introduce map_count as a separate file. Based on feedback from Daniel Vetter and Christian König. Thank you both! -Commit messages updated to point to userspace code in AOSP that will read the DMA-BUF sysfs stats. .../ABI/testing/sysfs-kernel-dmabuf-buffers | 52 drivers/dma-buf/Kconfig | 11 + drivers/dma-buf/Makefile | 1 + drivers/dma-buf/dma-buf-sysfs-stats.c | 285 ++ drivers/dma-buf/dma-buf-sysfs-stats.h | 62 drivers/dma-buf/dma-buf.c | 37 +++ include/linux/dma-buf.h | 20 ++ 7 files changed, 468 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers new file mode 100644 index ..6f7c65209f07 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers @@ -0,0 +1,52 @@ +What: /sys/kernel/dmabuf/buffers +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: The /sys/kernel/dmabuf/buffers directory contains a + snapshot of the internal state of every DMA-BUF. + /sys/kernel/dmabuf/buffers/ will contain the + statistics for the DMA-BUF with the unique inode number + +Users: kernel memory tuning/debugging tools + +What: /sys/kernel/dmabuf/buffers//exporter_name +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: This file is read-only and contains the name of the exporter of + the DMA-BUF. + +What: /sys/kernel/dmabuf/buffers//size +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: This file is read-only and specifies the size of the DMA-BUF in + bytes. + +What: /sys/kernel/dmabuf/buffers//attachments +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: This directory will contain subdirectories representing every + attachment of the DMA-BUF. + +What: /sys/kernel/dmabuf/buffers//attachments/ +Date: January 2021
Re: [Linaro-mm-sig] [PATCH v2] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
On Wed, Jan 20, 2021 at 4:42 AM Daniel Vetter wrote: > > On Wed, Jan 20, 2021 at 1:22 PM Christian König > wrote: > > > > Am 19.01.21 um 23:57 schrieb Hridya Valsaraju: > > > This patch allows statistics to be enabled for each DMA-BUF in > > > sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. > > > > > > The following stats will be exposed by the interface: > > > > > > /sys/kernel/dmabuf/buffers//exporter_name > > > /sys/kernel/dmabuf/buffers//size > > > /sys/kernel/dmabuf/buffers//attachments//device > > > /sys/kernel/dmabuf/buffers//attachments//map_counter > > > > > > The inode_number is unique for each DMA-BUF and was added earlier [1] > > > in order to allow userspace to track DMA-BUF usage across different > > > processes. > > > > > > Currently, this information is exposed in > > > /sys/kernel/debug/dma_buf/bufinfo. > > > However, since debugfs is considered unsafe to be mounted in production, > > > it is being duplicated in sysfs. > > > > > > This information will be used to derive DMA-BUF > > > per-exporter stats and per-device usage stats for Android Bug reports. > > > The corresponding userspace changes can be found at [2]. > > > Telemetry tools will also capture this information(along with other > > > memory metrics) periodically as well as on important events like a > > > foreground app kill (which might have been triggered by Low Memory > > > Killer). It will also contribute to provide a snapshot of the system > > > memory usage on other events such as OOM kills and Application Not > > > Responding events. > > > > > > A shell script that can be run on a classic Linux environment to read > > > out the DMA-BUF statistics can be found at [3](suggested by John > > > Stultz). > > > > > > The patch contains the following improvements over the previous version: > > > 1) Each attachment is represented by its own directory to allow creating > > > a symlink to the importing device and to also provide room for future > > > expansion. > > > 2) The number of distinct mappings of each attachment is exposed in a > > > separate file. > > > 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers > > > inorder to make the interface expandable in future. > > > > > > All of the improvements above are based on suggestions/feedback from > > > Daniel Vetter and Christian König. > > > > > > [1]: https://lore.kernel.org/patchwork/patch/1088791/ > > > [2]: > > > https://android-review.googlesource.com/q/topic:%22dmabuf-sysfs%22+(status:open%20OR%20status:merged) > > > [3]: > > > https://android-review.googlesource.com/c/platform/system/memory/libmeminfo/+/1549734 > > > > > > Signed-off-by: Hridya Valsaraju > > > --- > > > Changes in v2: > > > -Move statistics to /sys/kernel/dmabuf/buffers in oder to allow addition > > > of other DMA-BUF-related sysfs stats in future. Based on feedback from > > > Daniel Vetter. > > > -Each attachment has its own directory to represent attaching devices as > > > symlinks and to introduce map_count as a separate file. Based on > > > feedback from Daniel Vetter and Christian König. Thank you both! > > > -Commit messages updated to point to userspace code in AOSP that will > > > read the DMA-BUF sysfs stats. > > > > > > .../ABI/testing/sysfs-kernel-dmabuf-buffers | 52 > > > drivers/dma-buf/Kconfig | 11 + > > > drivers/dma-buf/Makefile | 1 + > > > drivers/dma-buf/dma-buf-sysfs-stats.c | 283 ++ > > > drivers/dma-buf/dma-buf-sysfs-stats.h | 62 > > > drivers/dma-buf/dma-buf.c | 37 +++ > > > include/linux/dma-buf.h | 20 ++ > > > 7 files changed, 466 insertions(+) > > > create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c > > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h > > > > > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > > > b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > > > new file mode 100644 > > > index ..6f7c65209f07 > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > > > @@ -0,0 +1
Re: [Linaro-mm-sig] [PATCH v2] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
On Wed, Jan 20, 2021 at 4:22 AM Christian König wrote: > > Am 19.01.21 um 23:57 schrieb Hridya Valsaraju: > > This patch allows statistics to be enabled for each DMA-BUF in > > sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. > > > > The following stats will be exposed by the interface: > > > > /sys/kernel/dmabuf/buffers//exporter_name > > /sys/kernel/dmabuf/buffers//size > > /sys/kernel/dmabuf/buffers//attachments//device > > /sys/kernel/dmabuf/buffers//attachments//map_counter > > > > The inode_number is unique for each DMA-BUF and was added earlier [1] > > in order to allow userspace to track DMA-BUF usage across different > > processes. > > > > Currently, this information is exposed in > > /sys/kernel/debug/dma_buf/bufinfo. > > However, since debugfs is considered unsafe to be mounted in production, > > it is being duplicated in sysfs. > > > > This information will be used to derive DMA-BUF > > per-exporter stats and per-device usage stats for Android Bug reports. > > The corresponding userspace changes can be found at [2]. > > Telemetry tools will also capture this information(along with other > > memory metrics) periodically as well as on important events like a > > foreground app kill (which might have been triggered by Low Memory > > Killer). It will also contribute to provide a snapshot of the system > > memory usage on other events such as OOM kills and Application Not > > Responding events. > > > > A shell script that can be run on a classic Linux environment to read > > out the DMA-BUF statistics can be found at [3](suggested by John > > Stultz). > > > > The patch contains the following improvements over the previous version: > > 1) Each attachment is represented by its own directory to allow creating > > a symlink to the importing device and to also provide room for future > > expansion. > > 2) The number of distinct mappings of each attachment is exposed in a > > separate file. > > 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers > > inorder to make the interface expandable in future. > > > > All of the improvements above are based on suggestions/feedback from > > Daniel Vetter and Christian König. > > > > [1]: https://lore.kernel.org/patchwork/patch/1088791/ > > [2]: > > https://android-review.googlesource.com/q/topic:%22dmabuf-sysfs%22+(status:open%20OR%20status:merged) > > [3]: > > https://android-review.googlesource.com/c/platform/system/memory/libmeminfo/+/1549734 > > > > Signed-off-by: Hridya Valsaraju > > --- > > Changes in v2: > > -Move statistics to /sys/kernel/dmabuf/buffers in oder to allow addition > > of other DMA-BUF-related sysfs stats in future. Based on feedback from > > Daniel Vetter. > > -Each attachment has its own directory to represent attaching devices as > > symlinks and to introduce map_count as a separate file. Based on > > feedback from Daniel Vetter and Christian König. Thank you both! > > -Commit messages updated to point to userspace code in AOSP that will > > read the DMA-BUF sysfs stats. > > > > .../ABI/testing/sysfs-kernel-dmabuf-buffers | 52 > > drivers/dma-buf/Kconfig | 11 + > > drivers/dma-buf/Makefile | 1 + > > drivers/dma-buf/dma-buf-sysfs-stats.c | 283 ++ > > drivers/dma-buf/dma-buf-sysfs-stats.h | 62 > > drivers/dma-buf/dma-buf.c | 37 +++ > > include/linux/dma-buf.h | 20 ++ > > 7 files changed, 466 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h > > > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > > b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > > new file mode 100644 > > index ..6f7c65209f07 > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > > @@ -0,0 +1,52 @@ > > +What:/sys/kernel/dmabuf/buffers > > +Date:January 2021 > > +KernelVersion: v5.12 > > +Contact: Hridya Valsaraju > > +Description: The /sys/kernel/dmabuf/buffers directory contains a > > + snapshot of the internal state of every DMA-BUF. > > + /sys/kernel/dmabuf/buffers/ will contain the > > + statistics for the DMA-BUF with the unique inode number
[PATCH v2] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
This patch allows statistics to be enabled for each DMA-BUF in sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. The following stats will be exposed by the interface: /sys/kernel/dmabuf/buffers//exporter_name /sys/kernel/dmabuf/buffers//size /sys/kernel/dmabuf/buffers//attachments//device /sys/kernel/dmabuf/buffers//attachments//map_counter The inode_number is unique for each DMA-BUF and was added earlier [1] in order to allow userspace to track DMA-BUF usage across different processes. Currently, this information is exposed in /sys/kernel/debug/dma_buf/bufinfo. However, since debugfs is considered unsafe to be mounted in production, it is being duplicated in sysfs. This information will be used to derive DMA-BUF per-exporter stats and per-device usage stats for Android Bug reports. The corresponding userspace changes can be found at [2]. Telemetry tools will also capture this information(along with other memory metrics) periodically as well as on important events like a foreground app kill (which might have been triggered by Low Memory Killer). It will also contribute to provide a snapshot of the system memory usage on other events such as OOM kills and Application Not Responding events. A shell script that can be run on a classic Linux environment to read out the DMA-BUF statistics can be found at [3](suggested by John Stultz). The patch contains the following improvements over the previous version: 1) Each attachment is represented by its own directory to allow creating a symlink to the importing device and to also provide room for future expansion. 2) The number of distinct mappings of each attachment is exposed in a separate file. 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers inorder to make the interface expandable in future. All of the improvements above are based on suggestions/feedback from Daniel Vetter and Christian König. [1]: https://lore.kernel.org/patchwork/patch/1088791/ [2]: https://android-review.googlesource.com/q/topic:%22dmabuf-sysfs%22+(status:open%20OR%20status:merged) [3]: https://android-review.googlesource.com/c/platform/system/memory/libmeminfo/+/1549734 Signed-off-by: Hridya Valsaraju --- Changes in v2: -Move statistics to /sys/kernel/dmabuf/buffers in oder to allow addition of other DMA-BUF-related sysfs stats in future. Based on feedback from Daniel Vetter. -Each attachment has its own directory to represent attaching devices as symlinks and to introduce map_count as a separate file. Based on feedback from Daniel Vetter and Christian König. Thank you both! -Commit messages updated to point to userspace code in AOSP that will read the DMA-BUF sysfs stats. .../ABI/testing/sysfs-kernel-dmabuf-buffers | 52 drivers/dma-buf/Kconfig | 11 + drivers/dma-buf/Makefile | 1 + drivers/dma-buf/dma-buf-sysfs-stats.c | 283 ++ drivers/dma-buf/dma-buf-sysfs-stats.h | 62 drivers/dma-buf/dma-buf.c | 37 +++ include/linux/dma-buf.h | 20 ++ 7 files changed, 466 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers new file mode 100644 index ..6f7c65209f07 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers @@ -0,0 +1,52 @@ +What: /sys/kernel/dmabuf/buffers +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: The /sys/kernel/dmabuf/buffers directory contains a + snapshot of the internal state of every DMA-BUF. + /sys/kernel/dmabuf/buffers/ will contain the + statistics for the DMA-BUF with the unique inode number + +Users: kernel memory tuning/debugging tools + +What: /sys/kernel/dmabuf/buffers//exporter_name +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: This file is read-only and contains the name of the exporter of + the DMA-BUF. + +What: /sys/kernel/dmabuf/buffers//size +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: This file is read-only and specifies the size of the DMA-BUF in + bytes. + +What: /sys/kernel/dmabuf/buffers//attachments +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: This directory will contain subdirectories representing every + attachment of the DMA-BUF. + +What: /sys/kernel/dmabuf/buffers//attachments/ +Date: January 2021 +KernelVersion: v5.12 +Contact: Hridya Valsaraju +Description: This directory will contain
Re: [PATCH v3 3/4] dt-bindings: reserved-memory: Make DMA-BUF CMA heap DT-configurable
On Thu, Jan 14, 2021 at 6:01 AM Rob Herring wrote:
>
> On Tue, Jan 12, 2021 at 05:21:42PM -0800, Minchan Kim wrote:
> > From: Hyesoo Yu
> >
> > Document devicetree binding for chunk cma heap on dma heap framework.
> >
> > The DMA chunk heap supports the bulk allocation of higher order pages.
>
> Why do we need this? What does this do that CMA doesn't?
>
> With a CMA area I can believe a carve out is a common, OS independent
> thing. This looks too closely tied to some Linux thing to go into DT.
Hello Rob,
Thank you for the review!
The chunk heap's allocator also allocates from the CMA area. It is,
however, optimized to perform bulk allocation of higher order pages in
an efficient manner. For this purpose, the heap needs an exclusive CMA
area that will only be used for allocation by the heap. This is the
reason why we need to use the DT to create and configure a reserved
memory region for use by the chunk CMA heap driver. Since all
allocation from DMA-BUF heaps happen from the user-space, there is no
other appropriate device-driver that we can use to register the chunk
CMA heap and configure the reserved memory region for its use.
We have been following your guidance in [1] to bind the chunk CMA heap
driver directly to the reserved_memory region it will allocate from.
Is there an alternative that we are missing Rob?
[1]:
https://lore.kernel.org/lkml/20191025225009.50305-2-john.stu...@linaro.org/T/#m3dc63acd33fea269a584f43bb799a876f0b2b45d
The use-case that we have for the heap currently will allocate memory
from it from userspace and use the allocated memory to optimize
4K/8K HDR video playback with a secure DRM HW pipeline.
Thank you for all the help and review :)
Regards,
Hridya
>
> >
> > Signed-off-by: Hyesoo Yu
> > Signed-off-by: Minchan Kim
> > Signed-off-by: Hridya Valsaraju
> > Change-Id: I8fb231e5a8360e2d8f65947e155b12aa664dde01
>
> Drop this.
>
> > ---
> > .../reserved-memory/dma_heap_chunk.yaml | 58 +++
> > 1 file changed, 58 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml
> > b/Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml
> > new file mode 100644
> > index ..3e7fed5fb006
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/reserved-memory/dma_heap_chunk.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Device tree binding for chunk heap on DMA HEAP FRAMEWORK
> > +
> > +description: |
> > + The DMA chunk heap is backed by the Contiguous Memory Allocator (CMA) and
> > + supports bulk allocation of fixed size pages.
> > +
> > +maintainers:
> > + - Hyesoo Yu
> > + - John Stultz
> > + - Minchan Kim
> > + - Hridya Valsaraju
>
> space ^
>
> > +
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - dma_heap,chunk
>
> The format is , and 'dma_heap' is not a vendor.
>
> > +
> > + chunk-order:
> > +description: |
> > +order of pages that will get allocated from the chunk DMA heap.
> > +maxItems: 1
> > +
> > + size:
> > +maxItems: 1
> > +
> > + alignment:
> > +maxItems: 1
> > +
> > +required:
> > + - compatible
> > + - size
> > + - alignment
> > + - chunk-order
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +reserved-memory {
> > +#address-cells = <2>;
> > +#size-cells = <1>;
> > +
> > +chunk_memory: chunk_memory {
> > +compatible = "dma_heap,chunk";
> > +size = <0x300>;
> > +alignment = <0x0 0x0001>;
> > +chunk-order = <4>;
> > +};
> > +};
> > +
> > +
> > --
> > 2.30.0.284.gd98b1dd5eaa7-goog
> >
Re: [PATCH v3 3/4] dt-bindings: reserved-memory: Make DMA-BUF CMA heap DT-configurable
On Wed, Jan 13, 2021 at 7:45 AM Rob Herring wrote: > > On Tue, 12 Jan 2021 17:21:42 -0800, Minchan Kim wrote: > > From: Hyesoo Yu > > > > Document devicetree binding for chunk cma heap on dma heap framework. > > > > The DMA chunk heap supports the bulk allocation of higher order pages. > > > > Signed-off-by: Hyesoo Yu > > Signed-off-by: Minchan Kim > > Signed-off-by: Hridya Valsaraju > > Change-Id: I8fb231e5a8360e2d8f65947e155b12aa664dde01 > > --- > > .../reserved-memory/dma_heap_chunk.yaml | 58 +++ > > 1 file changed, 58 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml > > > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > ./Documentation/devicetree/bindings/reserved-memory/dma_heap_chunk.yaml:58:1: > [warning] too many blank lines (2 > 1) (empty-lines) > > dtschema/dtc warnings/errors: > > See https://patchwork.ozlabs.org/patch/1425577 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. > Hi Rob, Sorry about that, I can see the warning after installing yamllint. Will fix it in the next version! Thanks, Hridya
Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
Thank you Christian!
On Fri, Dec 11, 2020 at 12:03 AM Christian König
wrote:
>
> Am 10.12.20 um 23:41 schrieb Hridya Valsaraju:
> > Thanks again for the reviews!
> >
> > On Thu, Dec 10, 2020 at 3:03 AM Christian König
> > wrote:
> >> Am 10.12.20 um 11:56 schrieb Greg KH:
> >>> On Thu, Dec 10, 2020 at 11:27:27AM +0100, Daniel Vetter wrote:
> >>>> On Thu, Dec 10, 2020 at 11:10:45AM +0100, Greg KH wrote:
> >>>>> On Thu, Dec 10, 2020 at 10:58:50AM +0100, Christian König wrote:
> >>>>>> In general a good idea, but I have a few concern/comments here.
> >>>>>>
> >>>>>> Am 10.12.20 um 05:43 schrieb Hridya Valsaraju:
> >>>>>>> This patch allows statistics to be enabled for each DMA-BUF in
> >>>>>>> sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS.
> >>>>>>>
> >>>>>>> The following stats will be exposed by the interface:
> >>>>>>>
> >>>>>>> /sys/kernel/dmabuf//exporter_name
> >>>>>>> /sys/kernel/dmabuf//size
> >>>>>>> /sys/kernel/dmabuf//dev_map_info
> >>>>>>>
> >>>>>>> The inode_number is unique for each DMA-BUF and was added earlier [1]
> >>>>>>> in order to allow userspace to track DMA-BUF usage across different
> >>>>>>> processes.
> >>>>>>>
> >>>>>>> Currently, this information is exposed in
> >>>>>>> /sys/kernel/debug/dma_buf/bufinfo.
> >>>>>>> However, since debugfs is considered unsafe to be mounted in
> >>>>>>> production,
> >>>>>>> it is being duplicated in sysfs.
> >>>>>> Mhm, this makes it part of the UAPI. What is the justification for
> >>>>>> this?
> >>>>>>
> >>>>>> In other words do we really need those debug information in a
> >>>>>> production
> >>>>>> environment?
> >>>>> Production environments seem to want to know who is using up memory :)
> >>>> This only shows shared memory, so it does smell a lot like
> >>>> $specific_issue
> >>>> and we're designing a narrow solution for that and then have to carry it
> >>>> forever.
> >>> I think the "issue" is that this was a feature from ion that people
> >>> "missed" in the dmabuf move. Taking away the ability to see what kind
> >>> of allocations were being made didn't make a lot of debugging tools
> >>> happy :(
> >> Yeah, that is certainly a very valid concern.
> >>
> >>> But Hridya knows more, she's been dealing with the transition for a long
> >>> time now.
> > Currently, telemetry tools capture this information(along with other
> > memory metrics) periodically as well as on important events like a
> > foreground app kill (which might have been triggered by an LMK). We
> > would also like to get a snapshot of the system memory usage on other
> > events such as OOM kills and ANRs.
>
> That userspace tools are going to use those files directly is the
> justification you need for the stable UAPI provided by sysfs.
>
> Otherwise debugfs would be the way to go even when that is often disabled.
>
> Please change the commit message to reflect that.
Sounds good, I will make sure to include it in the commit message of
the next version.
>
> >>>> E.g. why is the list of attachments not a sysfs link? That's how we
> >>>> usually expose struct device * pointers in sysfs to userspace, not as a
> >>>> list of things.
> >>> These aren't struct devices, so I don't understand the objection here.
> >>> Where else could these go in sysfs?
> >> Sure they are! Just take a look at an attachment:
> >>
> >> struct dma_buf_attachment {
> >>struct dma_buf *dmabuf;
> >>struct device *dev;
> >>
> >> This is the struct device which is importing the buffer and the patch in
> >> discussion is just printing the name of this device into sysfs.
> > I actually did not know that this is not ok to do. I will change it in
> > the next version of the patch to be sysfs links instead.
>
> Thanks, you need to restructure this patch a bit. But I agree with
> Daniel that links to the devices which are attached are more appropriate.
>
> I'm just not sure how we want to represent the map count for each
> attachment then, probably best to have that as separate file as well.
Yes, I can add the map count as a separate file. Thanks again!
Regards,
Hridya
>
> Regards,
> Christian.
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-team+unsubscr...@android.com.
>
Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
Thanks again for the reviews!
On Thu, Dec 10, 2020 at 3:03 AM Christian König
wrote:
>
> Am 10.12.20 um 11:56 schrieb Greg KH:
> > On Thu, Dec 10, 2020 at 11:27:27AM +0100, Daniel Vetter wrote:
> >> On Thu, Dec 10, 2020 at 11:10:45AM +0100, Greg KH wrote:
> >>> On Thu, Dec 10, 2020 at 10:58:50AM +0100, Christian König wrote:
> >>>> In general a good idea, but I have a few concern/comments here.
> >>>>
> >>>> Am 10.12.20 um 05:43 schrieb Hridya Valsaraju:
> >>>>> This patch allows statistics to be enabled for each DMA-BUF in
> >>>>> sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS.
> >>>>>
> >>>>> The following stats will be exposed by the interface:
> >>>>>
> >>>>> /sys/kernel/dmabuf//exporter_name
> >>>>> /sys/kernel/dmabuf//size
> >>>>> /sys/kernel/dmabuf//dev_map_info
> >>>>>
> >>>>> The inode_number is unique for each DMA-BUF and was added earlier [1]
> >>>>> in order to allow userspace to track DMA-BUF usage across different
> >>>>> processes.
> >>>>>
> >>>>> Currently, this information is exposed in
> >>>>> /sys/kernel/debug/dma_buf/bufinfo.
> >>>>> However, since debugfs is considered unsafe to be mounted in production,
> >>>>> it is being duplicated in sysfs.
> >>>> Mhm, this makes it part of the UAPI. What is the justification for this?
> >>>>
> >>>> In other words do we really need those debug information in a production
> >>>> environment?
> >>> Production environments seem to want to know who is using up memory :)
> >> This only shows shared memory, so it does smell a lot like $specific_issue
> >> and we're designing a narrow solution for that and then have to carry it
> >> forever.
> > I think the "issue" is that this was a feature from ion that people
> > "missed" in the dmabuf move. Taking away the ability to see what kind
> > of allocations were being made didn't make a lot of debugging tools
> > happy :(
>
> Yeah, that is certainly a very valid concern.
>
> > But Hridya knows more, she's been dealing with the transition for a long
> > time now.
Currently, telemetry tools capture this information(along with other
memory metrics) periodically as well as on important events like a
foreground app kill (which might have been triggered by an LMK). We
would also like to get a snapshot of the system memory usage on other
events such as OOM kills and ANRs.
> >
> >> E.g. why is the list of attachments not a sysfs link? That's how we
> >> usually expose struct device * pointers in sysfs to userspace, not as a
> >> list of things.
> > These aren't struct devices, so I don't understand the objection here.
> > Where else could these go in sysfs?
>
> Sure they are! Just take a look at an attachment:
>
> struct dma_buf_attachment {
> struct dma_buf *dmabuf;
> struct device *dev;
>
> This is the struct device which is importing the buffer and the patch in
> discussion is just printing the name of this device into sysfs.
I actually did not know that this is not ok to do. I will change it in
the next version of the patch to be sysfs links instead.
>
> >> Furthermore we don't have the exporter device covered anywhere, how is
> >> that tracked? Yes Android just uses ion for all shared buffers, but that's
> >> not how all of linux userspace works.
> > Do we have the exporter device link in the dmabuf interface? If so,
> > great, let's use that, but for some reason I didn't think it was there.
>
> Correct, since we don't really need a device as an exporter (it can just
> be a system heap as well) we only have a const char* as name for the
> exporter.
Yes, the file exporter_name prints out this information.
>
> >> Then I guess there's the mmaps, you can fish them out of procfs. A tool
> >> which collects all that information might be useful, just as demonstration
> >> of how this is all supposed to be used.
> > There's a script somewhere that does this today, again, Hridya knows
> > more.
That is correct, we do have a tool in AOSP that gathers the
per-process DMA-BUF map stats from procfs.
https://android.googlesource.com/platform/system/memory/libmeminfo/+/refs/heads/master/libdmabufinfo/tools/dmabuf_dump.cpp
When I send the next revision of the patch, I will also include links
to AOSP CLs that show the usage for the sysfs files.
> >
> >> There's also
Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
Thank you for the reviews Greg, Christian and Daniel! On Thu, Dec 10, 2020 at 1:59 AM Christian König wrote: > > In general a good idea, but I have a few concern/comments here. > > Am 10.12.20 um 05:43 schrieb Hridya Valsaraju: > > This patch allows statistics to be enabled for each DMA-BUF in > > sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. > > > > The following stats will be exposed by the interface: > > > > /sys/kernel/dmabuf//exporter_name > > /sys/kernel/dmabuf//size > > /sys/kernel/dmabuf//dev_map_info > > > > The inode_number is unique for each DMA-BUF and was added earlier [1] > > in order to allow userspace to track DMA-BUF usage across different > > processes. > > > > Currently, this information is exposed in > > /sys/kernel/debug/dma_buf/bufinfo. > > However, since debugfs is considered unsafe to be mounted in production, > > it is being duplicated in sysfs. > > Mhm, this makes it part of the UAPI. What is the justification for this? > > In other words do we really need those debug information in a production > environment? Yes, we currently collect this information on production devices as well. > > > > > This information is intended to help with root-causing > > low-memory kills and the debugging/analysis of other memory-related issues. > > > > It will also be used to derive DMA-BUF > > per-exporter stats and per-device usage stats for Android Bug reports. > > > > [1]: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1088791%2Fdata=04%7C01%7Cchristian.koenig%40amd.com%7C30a0e015502b4d20e18208d89cc63f1a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431722574983797%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=RdGMvj5VsFUwJcVOuSPaLuAr4eI3CR1YOaznupmpTqg%3Dreserved=0 > > > > Signed-off-by: Hridya Valsaraju > > --- > > Documentation/ABI/testing/sysfs-kernel-dmabuf | 32 > > drivers/dma-buf/Kconfig | 11 ++ > > drivers/dma-buf/Makefile | 1 + > > drivers/dma-buf/dma-buf-sysfs-stats.c | 162 ++ > > drivers/dma-buf/dma-buf-sysfs-stats.h | 37 > > drivers/dma-buf/dma-buf.c | 29 > > include/linux/dma-buf.h | 13 ++ > > 7 files changed, 285 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h > > > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf > > b/Documentation/ABI/testing/sysfs-kernel-dmabuf > > new file mode 100644 > > index ..02d407d57aaa > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf > > @@ -0,0 +1,32 @@ > > +What:/sys/kernel/dmabuf > > +Date:November 2020 > > +KernelVersion: v5.11 > > +Contact: Hridya Valsaraju > > +Description: The /sys/kernel/dmabuf directory contains a > > + snapshot of the internal state of every DMA-BUF. > > + /sys/kernel/dmabuf/ will contain the > > + statistics for the DMA-BUF with the unique inode number > > + > > +Users: kernel memory tuning/debugging tools > > + > > +What:/sys/kernel/dmabuf//exporter_name > > +Date:November 2020 > > +KernelVersion: v5.11 > > +Contact: Hridya Valsaraju > > +Description: This file is read-only and contains the name of the exporter > > of > > + the DMA-BUF. > > + > > +What:/sys/kernel/dmabuf//size > > +Dat: November 2020 > > +KernelVersion: v5.11 > > +Contact: Hridya Valsaraju > > +Description: This file is read-only and specifies the size of the DMA-BUF > > in > > + bytes. > > + > > +What:/sys/kernel/dmabuf//dev_map_info > > +Dat: November 2020 > > +KernelVersion: v5.11 > > +Contact: Hridya Valsaraju > > +Description: This file is read-only and lists the name of devices currently > > + mapping the DMA-BUF in a space-separated format. > > + > > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig > > index 4f8224a6ac95..2fed26f14548 100644 > > --- a/drivers/dma-buf/Kconfig > > +++ b/drivers/dma-buf/Kconfig > > @@ -64,6 +64,17 @@ menuconfig DMABUF
[PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
This patch allows statistics to be enabled for each DMA-BUF in
sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS.
The following stats will be exposed by the interface:
/sys/kernel/dmabuf//exporter_name
/sys/kernel/dmabuf//size
/sys/kernel/dmabuf//dev_map_info
The inode_number is unique for each DMA-BUF and was added earlier [1]
in order to allow userspace to track DMA-BUF usage across different
processes.
Currently, this information is exposed in
/sys/kernel/debug/dma_buf/bufinfo.
However, since debugfs is considered unsafe to be mounted in production,
it is being duplicated in sysfs.
This information is intended to help with root-causing
low-memory kills and the debugging/analysis of other memory-related issues.
It will also be used to derive DMA-BUF
per-exporter stats and per-device usage stats for Android Bug reports.
[1]: https://lore.kernel.org/patchwork/patch/1088791/
Signed-off-by: Hridya Valsaraju
---
Documentation/ABI/testing/sysfs-kernel-dmabuf | 32
drivers/dma-buf/Kconfig | 11 ++
drivers/dma-buf/Makefile | 1 +
drivers/dma-buf/dma-buf-sysfs-stats.c | 162 ++
drivers/dma-buf/dma-buf-sysfs-stats.h | 37
drivers/dma-buf/dma-buf.c | 29
include/linux/dma-buf.h | 13 ++
7 files changed, 285 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf
create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c
create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h
diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf
b/Documentation/ABI/testing/sysfs-kernel-dmabuf
new file mode 100644
index ..02d407d57aaa
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf
@@ -0,0 +1,32 @@
+What: /sys/kernel/dmabuf
+Date: November 2020
+KernelVersion: v5.11
+Contact: Hridya Valsaraju
+Description: The /sys/kernel/dmabuf directory contains a
+ snapshot of the internal state of every DMA-BUF.
+ /sys/kernel/dmabuf/ will contain the
+ statistics for the DMA-BUF with the unique inode number
+
+Users: kernel memory tuning/debugging tools
+
+What: /sys/kernel/dmabuf//exporter_name
+Date: November 2020
+KernelVersion: v5.11
+Contact: Hridya Valsaraju
+Description: This file is read-only and contains the name of the exporter of
+ the DMA-BUF.
+
+What: /sys/kernel/dmabuf//size
+Dat: November 2020
+KernelVersion: v5.11
+Contact: Hridya Valsaraju
+Description: This file is read-only and specifies the size of the DMA-BUF in
+ bytes.
+
+What: /sys/kernel/dmabuf//dev_map_info
+Dat: November 2020
+KernelVersion: v5.11
+Contact: Hridya Valsaraju
+Description: This file is read-only and lists the name of devices currently
+ mapping the DMA-BUF in a space-separated format.
+
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 4f8224a6ac95..2fed26f14548 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -64,6 +64,17 @@ menuconfig DMABUF_HEAPS
allows userspace to allocate dma-bufs that can be shared
between drivers.
+menuconfig DMABUF_SYSFS_STATS
+ bool "DMA-BUF sysfs statistics"
+ select DMA_SHARED_BUFFER
+ help
+ Choose this option to enable DMA-BUF sysfs statistics
+ in location /sys/kernel/dmabuf.
+
+ /sys/kernel/dmabuf/ will contain
+ statistics for the DMA-BUF with the unique inode number
+ .
+
source "drivers/dma-buf/heaps/Kconfig"
endmenu
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 995e05f609ff..40d81f23cacf 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_DMABUF_HEAPS) += heaps/
obj-$(CONFIG_SYNC_FILE)+= sync_file.o
obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o
obj-$(CONFIG_UDMABUF) += udmabuf.o
+obj-$(CONFIG_DMABUF_SYSFS_STATS) += dma-buf-sysfs-stats.o
dmabuf_selftests-y := \
selftest.o \
diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c
b/drivers/dma-buf/dma-buf-sysfs-stats.c
new file mode 100644
index ..bcbef81e0a5f
--- /dev/null
+++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define to_dma_buf_entry_from_kobj(x) container_of(x, struct
dma_buf_sysfs_entry, kobj)
+
+struct dma_buf_stats_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct dma_buf *dmabuf,
+ struct dma_buf_stats_attribute *attr, char *buf);
+};
+#define to_dma_buf_stats_attr(x) container_of(x, struct
dma_buf_stats_attribute, attr)
+
+static ssize_t dma_buf_stats_attribute_show(
Re: [PATCH] staging: ion: remove from the tree
On Thu, Aug 27, 2020 at 10:17 AM Greg Kroah-Hartman wrote: > > On Thu, Aug 27, 2020 at 10:31:41PM +0530, Amit Pundir wrote: > > On Thu, 27 Aug 2020 at 21:34, Greg Kroah-Hartman > > wrote: > > > > > > On Thu, Aug 27, 2020 at 09:31:27AM -0400, Laura Abbott wrote: > > > > On 8/27/20 8:36 AM, Greg Kroah-Hartman wrote: > > > > > The ION android code has long been marked to be removed, now that we > > > > > dma-buf support merged into the real part of the kernel. > > > > > > > > > > It was thought that we could wait to remove the ion kernel at a later > > > > > time, but as the out-of-tree Android fork of the ion code has diverged > > > > > quite a bit, and any Android device using the ion interface uses that > > > > > forked version and not this in-tree version, the in-tree copy of the > > > > > code is abandonded and not used by anyone. > > > > > > > > > > Combine this abandoned codebase with the need to make changes to it in > > > > > order to keep the kernel building properly, which then causes merge > > > > > issues when merging those changes into the out-of-tree Android code, > > > > > and > > > > > you end up with two different groups of people (the in-kernel-tree > > > > > developers, and the Android kernel developers) who are both annoyed at > > > > > the current situation. Because of this problem, just drop the > > > > > in-kernel > > > > > copy of the ion code now, as it's not used, and is only causing > > > > > problems > > > > > for everyone involved. > > > > > > > > > > Cc: "Arve Hjønnevåg" > > > > > Cc: "Christian König" > > > > > Cc: Christian Brauner > > > > > Cc: Christoph Hellwig > > > > > Cc: Hridya Valsaraju > > > > > Cc: Joel Fernandes > > > > > Cc: John Stultz > > > > > Cc: Laura Abbott > > > > > Cc: Martijn Coenen > > > > > Cc: Shuah Khan > > > > > Cc: Sumit Semwal > > > > > Cc: Suren Baghdasaryan > > > > > Cc: Todd Kjos > > > > > Cc: de...@driverdev.osuosl.org > > > > > Cc: dri-de...@lists.freedesktop.org > > > > > Cc: linaro-mm-...@lists.linaro.org > > > > > Signed-off-by: Greg Kroah-Hartman > > > > > > > > We discussed this at the Android MC on Monday and the plan was to > > > > remove it after the next LTS release. > > > > > > I know it was discussed, my point is that it is actually causing > > > problems now (with developers who want to change the internal kernel api > > > hitting issues, and newbies trying to clean up code in ways that isn't > > > exactly optimal wasting maintainer cycles), and that anyone who uses > > > this code, is not actually using this version of the code. Everyone who > > > relies on ion right now, is using the version that is in the Android > > > common kernel tree, which has diverged from this in-kernel way quite a > > > bit now for the reason that we didn't want to take any of those new > > > features in the in-kernel version. > > > > > > So this is a problem that we have caused by just wanting to wait, no one > > > is using this code, combined with it causing problems for the upstream > > > developers. > > > > > > There is nothing "magic" about the last kernel of the year that requires > > > this code to sit here until then. At that point in time, all users > > > will, again, be using the forked Android kernel version, and if we > > > delete this now here, that fork can remain just fine, with the added > > > benifit of it reducing developer workloads here in-kernel. > > > > > > So why wait? > > > > Hi, > > > > I don't know what is the right thing to do here. I just want to > > highlight that AOSP's audio (codec2) HAL depends on the ION system > > heap and it will break AOSP for people who boot mainline on their > > devices, even for just testing purpose like we do in Linaro. Right now > > we need only 1 (Android specific out-of-tree) patch to boot AOSP with > > mainline and Sumit is already trying to upstream that vma naming > > patch. Removal of in-kernel ION, will just add more to that delta. > > As AOSP will continue to rely on ION after December of this year, all > you are doing is postponing the inevitable a few more months. > > Push back on the Android team to fix up the code to not use ION, they > know this needs to happen. > Hi all, We are currently working with the codec2 team to transition codec2 to use libdmabufheap instead of libion. It will definitely happen during the Android S timeframe. Thanks, Hridya > thanks, > > greg k-h
Re: [PATCH] staging: ion: fix spelling mistake in function name "detatch" -> "detach"
On Wed, Aug 5, 2020 at 4:26 AM Colin King wrote:
>
> From: Colin Ian King
>
> There is a spelling mistake in the function name ion_dma_buf_detatch.
> Fix it by removing the extraneous t.
>
> Signed-off-by: Colin Ian King
> ---
Thanks Colin!
Acked-by: Hridya Valsaraju
> drivers/staging/android/ion/ion.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> index 3c9f09506ffa..e1fe03ceb7f1 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -205,8 +205,8 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf,
> return 0;
> }
>
> -static void ion_dma_buf_detatch(struct dma_buf *dmabuf,
> - struct dma_buf_attachment *attachment)
> +static void ion_dma_buf_detach(struct dma_buf *dmabuf,
> + struct dma_buf_attachment *attachment)
> {
> struct ion_dma_buf_attachment *a = attachment->priv;
> struct ion_buffer *buffer = dmabuf->priv;
> @@ -331,7 +331,7 @@ static const struct dma_buf_ops dma_buf_ops = {
> .mmap = ion_mmap,
> .release = ion_dma_buf_release,
> .attach = ion_dma_buf_attach,
> - .detach = ion_dma_buf_detatch,
> + .detach = ion_dma_buf_detach,
> .begin_cpu_access = ion_dma_buf_begin_cpu_access,
> .end_cpu_access = ion_dma_buf_end_cpu_access,
> };
> --
> 2.27.0
>
Re: [PATCH] binder: prevent transactions to context manager from its own process.
On Fri, Oct 11, 2019 at 3:11 PM Jann Horn wrote: > > On Fri, Oct 11, 2019 at 11:59 PM Jann Horn wrote: > > (I think you could also let A receive a handle > > to itself and then transact with itself, but I haven't tested that.) > > Ignore this sentence, that's obviously wrong because same-binder_proc > nodes will always show up as a binder, not a handle. Thank you for the email and steps to reproduce the issue Jann. I need some time to take a look at the same and I will get back to you once I understand it and hopefully have a fix. We do want to disallow same-process transactions. Here is a little bit more of context for the patch: https://lkml.org/lkml/2018/3/28/173
Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
On Tue, Oct 8, 2019 at 6:02 AM Christian Brauner
wrote:
>
> When a binder transaction is initiated on a binder device coming from a
> binderfs instance, a pointer to the name of the binder device is stashed
> in the binder_transaction_log_entry's context_name member. Later on it
> is used to print the name in print_binder_transaction_log_entry(). By
> the time print_binder_transaction_log_entry() accesses context_name
> binderfs_evict_inode() might have already freed the associated memory
> thereby causing a UAF. Do the simple thing and prevent this by copying
> the name of the binder device instead of stashing a pointer to it.
>
> Reported-by: Jann Horn
> Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> Link:
> https://lore.kernel.org/r/cag48ez14q0-f8lqsvcnbyr2o6gpw8shxsm4u5jmd9mpstem...@mail.gmail.com
> Cc: Joel Fernandes
> Cc: Todd Kjos
> Cc: Hridya Valsaraju
> Signed-off-by: Christian Brauner
Reviewed-by: Hridya Valsaraju
Thank you for sending out this fix Christian!
Regards,
Hridya
> ---
> drivers/android/binder.c | 4 +++-
> drivers/android/binder_internal.h | 2 +-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c0a491277aca..5b9ac2122e89 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -57,6 +57,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -66,6 +67,7 @@
> #include
>
> #include
> +#include
>
> #include
>
> @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> e->target_handle = tr->target.handle;
> e->data_size = tr->data_size;
> e->offsets_size = tr->offsets_size;
> - e->context_name = proc->context->name;
> + strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
>
> if (reply) {
> binder_inner_proc_lock(proc);
> diff --git a/drivers/android/binder_internal.h
> b/drivers/android/binder_internal.h
> index bd47f7f72075..ae991097d14d 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -130,7 +130,7 @@ struct binder_transaction_log_entry {
> int return_error_line;
> uint32_t return_error;
> uint32_t return_error_param;
> - const char *context_name;
> + char context_name[BINDERFS_MAX_NAME + 1];
> };
>
> struct binder_transaction_log {
> --
> 2.23.0
>
Re: UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels
Thank you for letting us know about the issue Jann. I will work on a
fix and send out the same for review once ready.
Regards,
Hridya
On Mon, Oct 7, 2019 at 2:04 PM Todd Kjos wrote:
>
> +Hridya Valsaraju
>
>
> On Mon, Oct 7, 2019 at 1:50 PM Jann Horn wrote:
> >
> > Hi!
> >
> > There is a use-after-free read in print_binder_transaction_log_entry()
> > on ANDROID_BINDERFS kernels because
> > print_binder_transaction_log_entry() prints the char* e->context_name
> > as string, and if the transaction occurred on a binder device from
> > binderfs, e->context_name belongs to the binder device and is freed
> > when the inode disappears.
> >
> > Luckily this shouldn't have security implications, since:
> >
> > a) reading the binder transaction log is already a pretty privileged
> > operation
> > b) I can't find any actual users of ANDROID_BINDERFS
> >
> > I guess there are three ways to fix it:
> > 1) Create a new shared global spinlock for binderfs_evict_inode() and
> > binder_transaction_log_show(), and let binderfs_evict_inode() scan the
> > transaction log for pointers to its name and replace them with
> > pointers to a statically-allocated string "{DELETED}" or something
> > like that.
> > 2) Let the transaction log contain non-reusable device identifiers
> > instead of name pointers, and let print_binder_transaction_log_entry()
> > look them up in something like a hashtable.
> > 3) Just copy the name into the transaction log every time.
> >
> > I'm not sure which one is better, or whether there's a nicer fourth
> > option, so I'm leaving writing a patch for this to y'all.
> >
> >
> > Trigger instructions (requires you to have some helpers that can
> > register a context manager and send some transaction to it):
> > ==
> > root@test:/home/user# mkdir /tmp/binder
> > root@test:/home/user# mount -t binder -o stats=global /dev/null /tmp/binder
> > root@test:/home/user# ls -l /tmp/binder
> > total 0
> > crw--- 1 root root 248, 1 Oct 7 20:34 binder
> > crw--- 1 root root 248, 0 Oct 7 20:34 binder-control
> > drwxr-xr-x 3 root root 0 Oct 7 20:34 binder_logs
> > crw--- 1 root root 248, 2 Oct 7 20:34 hwbinder
> > crw--- 1 root root 248, 3 Oct 7 20:34 vndbinder
> > root@test:/home/user# ln -s /tmp/binder/binder /dev/binder
> > [run some simple binder demo code to temporarily register a context
> > manager and send a binder transaction]
> > root@test:/home/user# rm /tmp/binder/binder
> > root@test:/home/user# cat /tmp/binder/binder_logs/transaction_log
> > 2: call from 2277:2277 to 2273:0 context @��� node 1 handle 0
> > size 24:8 ret 0/0 l=0
> > 5: call from 2273:2273 to 2277:2277 context @��� node 3 handle 1
> > size 0:0 ret 0/0 l=0
> > 6: reply from 2277:2277 to 2273:2273 context @��� node 0 handle 0
> > size 4:0 ret 0/0 l=0
> > 7: reply from 2273:2273 to 2277:2277 context @��� node 0 handle 0
> > size 4:0 ret 0/0 l=0
> > root@test:/home/user#
> > ==
> >
> > ASAN splat:
> > [ 333.300753]
> > ==
> > [ 333.303197] BUG: KASAN: use-after-free in string_nocheck+0x9d/0x160
> > [ 333.305081] Read of size 1 at addr 8880b0981258 by task cat/2279
> >
> > [ 333.307415] CPU: 1 PID: 2279 Comm: cat Not tainted 5.4.0-rc1+ #513
> > [ 333.309304] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > BIOS 1.12.0-1 04/01/2014
> > [ 333.310987] Call Trace:
> > [ 333.312032] dump_stack+0x7c/0xc0
> > [ 333.312581] ? string_nocheck+0x9d/0x160
> > [ 333.313157] print_address_description.constprop.7+0x36/0x50
> > [ 333.314030] ? string_nocheck+0x9d/0x160
> > [ 333.314603] ? string_nocheck+0x9d/0x160
> > [ 333.315236] __kasan_report.cold.10+0x1a/0x35
> > [ 333.315972] ? string_nocheck+0x9d/0x160
> > [ 333.316545] kasan_report+0xe/0x20
> > [ 333.317104] string_nocheck+0x9d/0x160
> > [ 333.317652] ? widen_string+0x160/0x160
> > [ 333.318270] ? string_nocheck+0x160/0x160
> > [ 333.318857] ? unwind_get_return_address+0x2a/0x40
> > [ 333.319636] ? profile_setup.cold.9+0x96/0x96
> > [ 333.320359] string+0xb6/0xc0
> > [ 333.320800] ? hex_string+0x280/0x280
> > [ 333.321398] vsnprintf+0x20c/0x780
> > [ 333.321898] ? num_to_str+0x180/0x180
> > [ 333.322503] ? __kasan_kmalloc.constprop.6+0xc1/0xd0
> > [ 333.323235] ? vfs_read+0xbc/0x1e0
> > [ 333.323814] ? ksys_read+0xb5/0x150
> > [ 333.324323]
Re: [PATCH v3 0/4] Add binder state and statistics to binderfs
On Fri, Sep 27, 2019 at 6:19 AM Christian Brauner wrote: > > On Tue, Sep 03, 2019 at 09:16:51AM -0700, Hridya Valsaraju wrote: > > Currently, the only way to access binder state and > > statistics is through debugfs. We need a way to > > access the same even when debugfs is not mounted. > > These patches add a mount option to make this > > information available in binderfs without affecting > > its presence in debugfs. The following debugfs nodes > > will be made available in a binderfs instance when > > mounted with the mount option 'stats=global' or 'stats=local'. > > > > /sys/kernel/debug/binder/failed_transaction_log > > /sys/kernel/debug/binder/proc > > /sys/kernel/debug/binder/state > > /sys/kernel/debug/binder/stats > > /sys/kernel/debug/binder/transaction_log > > /sys/kernel/debug/binder/transactions > > I'm sitting in a talk from Jonathan about kernel documentation and what > I realized is that we forgot to update the documentation I wrote for > binderfs in Documentation/admin-guide/binderfs.rst to reflect the new > stats=global mount option. Would be great if we could add that after rc1 > is out. Would you have time to do that, Hridya? Thanks for catching that Christian, sorry about the miss! I will send out a patch ASAP to add the documentation. Regards, Hridya > > Should just be a new entry under: > > Options > --- > max > binderfs instances can be mounted with a limit on the number of binder > devices that can be allocated. The ``max=`` mount option serves as > a per-instance limit. If ``max=`` is set then only number > of binder devices can be allocated in this binderfs instance. > stats > > > Thanks! > Christian
Re: [PATCH v3 0/4] Add binder state and statistics to binderfs
On Wed, Sep 4, 2019 at 7:20 AM Joel Fernandes wrote: > > On September 4, 2019 7:19:35 AM EDT, Christian Brauner > wrote: > >On Tue, Sep 03, 2019 at 09:16:51AM -0700, Hridya Valsaraju wrote: > >> Currently, the only way to access binder state and > >> statistics is through debugfs. We need a way to > >> access the same even when debugfs is not mounted. > >> These patches add a mount option to make this > >> information available in binderfs without affecting > >> its presence in debugfs. The following debugfs nodes > >> will be made available in a binderfs instance when > >> mounted with the mount option 'stats=global' or 'stats=local'. > >> > >> /sys/kernel/debug/binder/failed_transaction_log > >> /sys/kernel/debug/binder/proc > >> /sys/kernel/debug/binder/state > >> /sys/kernel/debug/binder/stats > >> /sys/kernel/debug/binder/transaction_log > >> /sys/kernel/debug/binder/transactions > > > >Acked-by: Christian Brauner > > > >Btw, I think your counting is off-by-one. :) We usually count the > >initial send of a series as 0 and the first rework of that series as > >v1. > >I think you counted your initial send as v1 and the first rework as v2. > > Which is fine. I have done it both ways. Is this a rule written somewhere? > > >:) > > > > If I am not mistaken, this is Hridya's first set of kernel patches. > Congrats on landing it upstream and to everyone for reviews! (assuming > nothing falls apart on the way to Linus tree). I really hope so! Thank you Joel and everyone else for the reviews ! > > thanks, > > - Joel > > [TLDR] > My first kernel patch was 10 years ago to a WiFi driver when I was an > intern at University. I was thrilled to have fixed a bug in network > bridging code in the 802.11s stack. This is always a special moment so > congrats again! ;-) > > > > > > >Christian > > > >> > >> Hridya Valsaraju (4): > >> binder: add a mount option to show global stats > >> binder: Add stats, state and transactions files > >> binder: Make transaction_log available in binderfs > >> binder: Add binder_proc logging to binderfs > >> > >> drivers/android/binder.c | 95 ++- > >> drivers/android/binder_internal.h | 84 ++ > >> drivers/android/binderfs.c| 255 > >++ > >> 3 files changed, 362 insertions(+), 72 deletions(-) > >> > >> -- > >> 2.23.0.187.g17f5b7556c-goog > >>
Re: [PATCH v3 0/4] Add binder state and statistics to binderfs
On Wed, Sep 4, 2019 at 8:12 AM Christian Brauner wrote: > > On Wed, Sep 04, 2019 at 04:49:03PM +0200, Greg Kroah-Hartman wrote: > > On Wed, Sep 04, 2019 at 10:20:32AM -0400, Joel Fernandes wrote: > > > On September 4, 2019 7:19:35 AM EDT, Christian Brauner > > > wrote: > > > >On Tue, Sep 03, 2019 at 09:16:51AM -0700, Hridya Valsaraju wrote: > > > >> Currently, the only way to access binder state and > > > >> statistics is through debugfs. We need a way to > > > >> access the same even when debugfs is not mounted. > > > >> These patches add a mount option to make this > > > >> information available in binderfs without affecting > > > >> its presence in debugfs. The following debugfs nodes > > > >> will be made available in a binderfs instance when > > > >> mounted with the mount option 'stats=global' or 'stats=local'. > > > >> > > > >> /sys/kernel/debug/binder/failed_transaction_log > > > >> /sys/kernel/debug/binder/proc > > > >> /sys/kernel/debug/binder/state > > > >> /sys/kernel/debug/binder/stats > > > >> /sys/kernel/debug/binder/transaction_log > > > >> /sys/kernel/debug/binder/transactions > > > > > > > >Acked-by: Christian Brauner > > > > > > > >Btw, I think your counting is off-by-one. :) We usually count the > > > >initial send of a series as 0 and the first rework of that series as > > > >v1. > > > >I think you counted your initial send as v1 and the first rework as v2. > > > > > > Which is fine. I have done it both ways. Is this a rule written somewhere? > > > > No where, I can count both ways, it's not a big deal :) > > It isn't documented (as many things we still do are) and it's not a big > deal. But most people seem to be counting revisions starting from 0 it > seems. I went looking for previous version to link to in the patch cover > letter and was confused because I was missing a v1. :) > > Anyway, I'm happy that Hridya landed this! It was fun helping her the > last couple of weeks on- and off-list. Thanks for getting this done! I > hope we'll see even more patches in the future. :) Thank you so much Christian and Todd for the guidance and thorough reviews on the many patches I sent your way both on and off-list :) I really appreciate it! > > Christian
[PATCH v3 2/4] binder: Add stats, state and transactions files
The following binder stat files currently live in debugfs.
/sys/kernel/debug/binder/state
/sys/kernel/debug/binder/stats
/sys/kernel/debug/binder/transactions
This patch makes these files available in a binderfs instance
mounted with the mount option 'stats=global'. For example, if a binderfs
instance is mounted at path /dev/binderfs, the above files will be
available at the following locations:
/dev/binderfs/binder_logs/state
/dev/binderfs/binder_logs/stats
/dev/binderfs/binder_logs/transactions
This provides a way to access them even when debugfs is not mounted.
Acked-by: Christian Brauner
Signed-off-by: Hridya Valsaraju
---
Changes in v3:
- Use set_nlink() instead of inc_nlink() in binderfs_create_dir() as
per Christian Brauner.
- Replace parent->d_inode usage with d_inode(parent) in
binderfs_create_file() for consistency as per Christian Brauner.
Changes in v2:
- Consistently name variables across functions as per Christian
Brauner.
- Improve check for binderfs device in binderfs_evict_inode()
as per Christian Brauner.
drivers/android/binder.c | 15 ++--
drivers/android/binder_internal.h | 8 ++
drivers/android/binderfs.c| 140 +-
3 files changed, 153 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ca6b21a53321..de795bd229c4 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6055,7 +6055,7 @@ static void print_binder_proc_stats(struct seq_file *m,
}
-static int state_show(struct seq_file *m, void *unused)
+int binder_state_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
struct binder_node *node;
@@ -6094,7 +6094,7 @@ static int state_show(struct seq_file *m, void *unused)
return 0;
}
-static int stats_show(struct seq_file *m, void *unused)
+int binder_stats_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
@@ -6110,7 +6110,7 @@ static int stats_show(struct seq_file *m, void *unused)
return 0;
}
-static int transactions_show(struct seq_file *m, void *unused)
+int binder_transactions_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
@@ -6198,9 +6198,6 @@ const struct file_operations binder_fops = {
.release = binder_release,
};
-DEFINE_SHOW_ATTRIBUTE(state);
-DEFINE_SHOW_ATTRIBUTE(stats);
-DEFINE_SHOW_ATTRIBUTE(transactions);
DEFINE_SHOW_ATTRIBUTE(transaction_log);
static int __init init_binder_device(const char *name)
@@ -6256,17 +6253,17 @@ static int __init binder_init(void)
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _state_fops);
debugfs_create_file("stats",
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _stats_fops);
debugfs_create_file("transactions",
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _transactions_fops);
debugfs_create_file("transaction_log",
0444,
binder_debugfs_dir_entry_root,
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index fe8c745dc8e0..12ef96f256c6 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -57,4 +57,12 @@ static inline int __init init_binderfs(void)
}
#endif
+int binder_stats_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_stats);
+
+int binder_state_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_state);
+
+int binder_transactions_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_transactions);
#endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7045bfe5b52b..01c1db463053 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -280,7 +280,7 @@ static void binderfs_evict_inode(struct inode *inode)
clear_inode(inode);
- if (!device)
+ if (!S_ISCHR(inode->i_mode) || !device)
return;
mutex_lock(_minors_mutex);
@@ -502,6 +502,141 @@ static const struct inode_operations
binderfs_dir_inode_operations = {
.unlink = binderfs_unlink,
};
+static struct inode *binderfs_make_inode(struct super_block *sb, int mode)
+{
+ struct inode *ret;
+
+ ret = new_inode(sb);
+ if (ret) {
+
[PATCH v3 3/4] binder: Make transaction_log available in binderfs
Currently, the binder transaction log files 'transaction_log'
and 'failed_transaction_log' live in debugfs at the following locations:
/sys/kernel/debug/binder/failed_transaction_log
/sys/kernel/debug/binder/transaction_log
This patch makes these files also available in a binderfs instance
mounted with the mount option "stats=global".
It does not affect the presence of these files in debugfs.
If a binderfs instance is mounted at path /dev/binderfs, the location of
these files will be as follows:
/dev/binderfs/binder_logs/failed_transaction_log
/dev/binderfs/binder_logs/transaction_log
This change provides an alternate option to access these files when
debugfs is not mounted.
Acked-by: Christian Brauner
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
-Consistent variable naming accross functions as per Christian Brauner.
drivers/android/binder.c | 34 +--
drivers/android/binder_internal.h | 30 +++
drivers/android/binderfs.c| 18
3 files changed, 53 insertions(+), 29 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index de795bd229c4..bed217310197 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -197,30 +197,8 @@ static inline void binder_stats_created(enum
binder_stat_types type)
atomic_inc(_stats.obj_created[type]);
}
-struct binder_transaction_log_entry {
- int debug_id;
- int debug_id_done;
- int call_type;
- int from_proc;
- int from_thread;
- int target_handle;
- int to_proc;
- int to_thread;
- int to_node;
- int data_size;
- int offsets_size;
- int return_error_line;
- uint32_t return_error;
- uint32_t return_error_param;
- const char *context_name;
-};
-struct binder_transaction_log {
- atomic_t cur;
- bool full;
- struct binder_transaction_log_entry entry[32];
-};
-static struct binder_transaction_log binder_transaction_log;
-static struct binder_transaction_log binder_transaction_log_failed;
+struct binder_transaction_log binder_transaction_log;
+struct binder_transaction_log binder_transaction_log_failed;
static struct binder_transaction_log_entry *binder_transaction_log_add(
struct binder_transaction_log *log)
@@ -6166,7 +6144,7 @@ static void print_binder_transaction_log_entry(struct
seq_file *m,
"\n" : " (incomplete)\n");
}
-static int transaction_log_show(struct seq_file *m, void *unused)
+int binder_transaction_log_show(struct seq_file *m, void *unused)
{
struct binder_transaction_log *log = m->private;
unsigned int log_cur = atomic_read(>cur);
@@ -6198,8 +6176,6 @@ const struct file_operations binder_fops = {
.release = binder_release,
};
-DEFINE_SHOW_ATTRIBUTE(transaction_log);
-
static int __init init_binder_device(const char *name)
{
int ret;
@@ -6268,12 +6244,12 @@ static int __init binder_init(void)
0444,
binder_debugfs_dir_entry_root,
_transaction_log,
- _log_fops);
+ _transaction_log_fops);
debugfs_create_file("failed_transaction_log",
0444,
binder_debugfs_dir_entry_root,
_transaction_log_failed,
- _log_fops);
+ _transaction_log_fops);
}
if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 12ef96f256c6..b9be42d9464c 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -65,4 +65,34 @@ DEFINE_SHOW_ATTRIBUTE(binder_state);
int binder_transactions_show(struct seq_file *m, void *unused);
DEFINE_SHOW_ATTRIBUTE(binder_transactions);
+
+int binder_transaction_log_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_transaction_log);
+
+struct binder_transaction_log_entry {
+ int debug_id;
+ int debug_id_done;
+ int call_type;
+ int from_proc;
+ int from_thread;
+ int target_handle;
+ int to_proc;
+ int to_thread;
+ int to_node;
+ int data_size;
+ int offsets_size;
+ int return_error_line;
+ uint32_t return_error;
+ uint32_t return_error_param;
+ const char *context_name;
+};
+
+struct binder_transaction_log {
+ atomic_t cur;
+ bool full;
+ struct binder_transaction_log_entry entry[32];
+};
+
+extern struct binder_transaction_log binder_transaction_log;
+extern struct binder_transaction_log binder_transaction_log_failed;
#endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/d
[PATCH v3 4/4] binder: Add binder_proc logging to binderfs
Currently /sys/kernel/debug/binder/proc contains
the debug data for every binder_proc instance.
This patch makes this information also available
in a binderfs instance mounted with a mount option
"stats=global" in addition to debugfs. The patch does
not affect the presence of the file in debugfs.
If a binderfs instance is mounted at path /dev/binderfs,
this file would be present at /dev/binderfs/binder_logs/proc.
This change provides an alternate way to access this file when debugfs
is not mounted.
Acked-by: Christian Brauner
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
- Consistent variable naming across functions as per Christian
Brauner.
- As suggested by Todd Kjos, log a failure to create a
process-specific binderfs log file if the error code is not EEXIST
since an error code of EEXIST is expected if
multiple contexts of the same process try to create the file during
binder_open().
drivers/android/binder.c | 46 -
drivers/android/binder_internal.h | 46 +
drivers/android/binderfs.c| 68 ++-
3 files changed, 121 insertions(+), 39 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index bed217310197..ee610ea48309 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -481,6 +481,7 @@ struct binder_priority {
* @inner_lock: can nest under outer_lock and/or node lock
* @outer_lock: no nesting under innor or node lock
*Lock order: 1) outer, 2) node, 3) inner
+ * @binderfs_entry: process-specific binderfs log file
*
* Bookkeeping structure for binder processes
*/
@@ -510,6 +511,7 @@ struct binder_proc {
struct binder_context *context;
spinlock_t inner_lock;
spinlock_t outer_lock;
+ struct dentry *binderfs_entry;
};
enum {
@@ -5347,6 +5349,8 @@ static int binder_open(struct inode *nodp, struct file
*filp)
{
struct binder_proc *proc;
struct binder_device *binder_dev;
+ struct binderfs_info *info;
+ struct dentry *binder_binderfs_dir_entry_proc = NULL;
binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d:%d\n", __func__,
current->group_leader->pid, current->pid);
@@ -5368,11 +5372,14 @@ static int binder_open(struct inode *nodp, struct file
*filp)
}
/* binderfs stashes devices in i_private */
- if (is_binderfs_device(nodp))
+ if (is_binderfs_device(nodp)) {
binder_dev = nodp->i_private;
- else
+ info = nodp->i_sb->s_fs_info;
+ binder_binderfs_dir_entry_proc = info->proc_log_dir;
+ } else {
binder_dev = container_of(filp->private_data,
struct binder_device, miscdev);
+ }
proc->context = _dev->context;
binder_alloc_init(>alloc);
@@ -5403,6 +5410,35 @@ static int binder_open(struct inode *nodp, struct file
*filp)
_fops);
}
+ if (binder_binderfs_dir_entry_proc) {
+ char strbuf[11];
+ struct dentry *binderfs_entry;
+
+ snprintf(strbuf, sizeof(strbuf), "%u", proc->pid);
+ /*
+* Similar to debugfs, the process specific log file is shared
+* between contexts. If the file has already been created for a
+* process, the following binderfs_create_file() call will
+* fail with error code EEXIST if another context of the same
+* process invoked binder_open(). This is ok since same as
+* debugfs, the log file will contain information on all
+* contexts of a given PID.
+*/
+ binderfs_entry =
binderfs_create_file(binder_binderfs_dir_entry_proc,
+ strbuf, _fops, (void *)(unsigned long)proc->pid);
+ if (!IS_ERR(binderfs_entry)) {
+ proc->binderfs_entry = binderfs_entry;
+ } else {
+ int error;
+
+ error = PTR_ERR(binderfs_entry);
+ if (error != -EEXIST) {
+ pr_warn("Unable to create file %s in binderfs
(error %d)\n",
+ strbuf, error);
+ }
+ }
+ }
+
return 0;
}
@@ -5442,6 +5478,12 @@ static int binder_release(struct inode *nodp, struct
file *filp)
struct binder_proc *proc = filp->private_data;
debugfs_remove(proc->debugfs_entry);
+
+ if (proc->binderfs_entry) {
+ binderfs_remove_file(proc->binderfs_entry);
+ proc->binderfs_entry = NULL;
+ }
+
binder_defer_work(proc, BINDER_DEFERRED_RELEASE);
re
[PATCH v3 1/4] binder: add a mount option to show global stats
Currently, all binder state and statistics live in debugfs.
We need this information even when debugfs is not mounted.
This patch adds the mount option 'stats' to enable a binderfs
instance to have binder debug information present in the same.
'stats=global' will enable the global binder statistics. In
the future, 'stats=local' will enable binder statistics local
to the binderfs instance. The two modes 'global' and 'local'
will be mutually exclusive. 'stats=global' option is only available
for a binderfs instance mounted in the initial user namespace.
An attempt to use the option to mount a binderfs instance in
another user namespace will return an EPERM error.
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
- Improve error check in binderfs_parse_mount_opts() as per Greg
Kroah-Hartman.
- Change pr_info() log before failure to pr_err() as per Greg
Kroah-Hartman.
drivers/android/binderfs.c | 45 --
1 file changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index cc2e71576396..7045bfe5b52b 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -51,18 +51,27 @@ static DEFINE_IDA(binderfs_minors);
/**
* binderfs_mount_opts - mount options for binderfs
* @max: maximum number of allocatable binderfs binder devices
+ * @stats_mode: enable binder stats in binderfs.
*/
struct binderfs_mount_opts {
int max;
+ int stats_mode;
};
enum {
Opt_max,
+ Opt_stats_mode,
Opt_err
};
+enum binderfs_stats_mode {
+ STATS_NONE,
+ STATS_GLOBAL,
+};
+
static const match_table_t tokens = {
{ Opt_max, "max=%d" },
+ { Opt_stats_mode, "stats=%s" },
{ Opt_err, NULL }
};
@@ -290,8 +299,9 @@ static void binderfs_evict_inode(struct inode *inode)
static int binderfs_parse_mount_opts(char *data,
struct binderfs_mount_opts *opts)
{
- char *p;
+ char *p, *stats;
opts->max = BINDERFS_MAX_MINOR;
+ opts->stats_mode = STATS_NONE;
while ((p = strsep(, ",")) != NULL) {
substring_t args[MAX_OPT_ARGS];
@@ -311,6 +321,22 @@ static int binderfs_parse_mount_opts(char *data,
opts->max = max_devices;
break;
+ case Opt_stats_mode:
+ if (!capable(CAP_SYS_ADMIN))
+ return -EINVAL;
+
+ stats = match_strdup([0]);
+ if (!stats)
+ return -ENOMEM;
+
+ if (strcmp(stats, "global") != 0) {
+ kfree(stats);
+ return -EINVAL;
+ }
+
+ opts->stats_mode = STATS_GLOBAL;
+ kfree(stats);
+ break;
default:
pr_err("Invalid mount options\n");
return -EINVAL;
@@ -322,8 +348,21 @@ static int binderfs_parse_mount_opts(char *data,
static int binderfs_remount(struct super_block *sb, int *flags, char *data)
{
+ int prev_stats_mode, ret;
struct binderfs_info *info = sb->s_fs_info;
- return binderfs_parse_mount_opts(data, >mount_opts);
+
+ prev_stats_mode = info->mount_opts.stats_mode;
+ ret = binderfs_parse_mount_opts(data, >mount_opts);
+ if (ret)
+ return ret;
+
+ if (prev_stats_mode != info->mount_opts.stats_mode) {
+ pr_err("Binderfs stats mode cannot be changed during a
remount\n");
+ info->mount_opts.stats_mode = prev_stats_mode;
+ return -EINVAL;
+ }
+
+ return 0;
}
static int binderfs_show_mount_opts(struct seq_file *seq, struct dentry *root)
@@ -333,6 +372,8 @@ static int binderfs_show_mount_opts(struct seq_file *seq,
struct dentry *root)
info = root->d_sb->s_fs_info;
if (info->mount_opts.max <= BINDERFS_MAX_MINOR)
seq_printf(seq, ",max=%d", info->mount_opts.max);
+ if (info->mount_opts.stats_mode == STATS_GLOBAL)
+ seq_printf(seq, ",stats=global");
return 0;
}
--
2.23.0.187.g17f5b7556c-goog
[PATCH v3 0/4] Add binder state and statistics to binderfs
Currently, the only way to access binder state and statistics is through debugfs. We need a way to access the same even when debugfs is not mounted. These patches add a mount option to make this information available in binderfs without affecting its presence in debugfs. The following debugfs nodes will be made available in a binderfs instance when mounted with the mount option 'stats=global' or 'stats=local'. /sys/kernel/debug/binder/failed_transaction_log /sys/kernel/debug/binder/proc /sys/kernel/debug/binder/state /sys/kernel/debug/binder/stats /sys/kernel/debug/binder/transaction_log /sys/kernel/debug/binder/transactions Hridya Valsaraju (4): binder: add a mount option to show global stats binder: Add stats, state and transactions files binder: Make transaction_log available in binderfs binder: Add binder_proc logging to binderfs drivers/android/binder.c | 95 ++- drivers/android/binder_internal.h | 84 ++ drivers/android/binderfs.c| 255 ++ 3 files changed, 362 insertions(+), 72 deletions(-) -- 2.23.0.187.g17f5b7556c-goog
Re: [PATCH v2 3/4] binder: Make transaction_log available in binderfs
On Fri, Aug 30, 2019 at 4:34 AM Christian Brauner
wrote:
>
> On Thu, Aug 29, 2019 at 02:18:11PM -0700, Hridya Valsaraju wrote:
> > Currently, the binder transaction log files 'transaction_log'
> > and 'failed_transaction_log' live in debugfs at the following locations:
> >
> > /sys/kernel/debug/binder/failed_transaction_log
> > /sys/kernel/debug/binder/transaction_log
> >
> > This patch makes these files also available in a binderfs instance
> > mounted with the mount option "stats=global".
> > It does not affect the presence of these files in debugfs.
> > If a binderfs instance is mounted at path /dev/binderfs, the location of
> > these files will be as follows:
> >
> > /dev/binderfs/binder_logs/failed_transaction_log
> > /dev/binderfs/binder_logs/transaction_log
> >
> > This change provides an alternate option to access these files when
> > debugfs is not mounted.
> >
> > Signed-off-by: Hridya Valsaraju
>
> (If you don't change this patch in the next version, please just keep my:
>
> Acked-by: Christian Brauner
>
> when sending it out. :)
Will do! Thank you Christian!
>
> > ---
> >
> > Changes in v2:
> > -Consistent variable naming accross functions as per Christian Brauner.
> >
> > drivers/android/binder.c | 34 +--
> > drivers/android/binder_internal.h | 30 +++
> > drivers/android/binderfs.c| 18
> > 3 files changed, 53 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index de795bd229c4..bed217310197 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -197,30 +197,8 @@ static inline void binder_stats_created(enum
> > binder_stat_types type)
> > atomic_inc(_stats.obj_created[type]);
> > }
> >
> > -struct binder_transaction_log_entry {
> > - int debug_id;
> > - int debug_id_done;
> > - int call_type;
> > - int from_proc;
> > - int from_thread;
> > - int target_handle;
> > - int to_proc;
> > - int to_thread;
> > - int to_node;
> > - int data_size;
> > - int offsets_size;
> > - int return_error_line;
> > - uint32_t return_error;
> > - uint32_t return_error_param;
> > - const char *context_name;
> > -};
> > -struct binder_transaction_log {
> > - atomic_t cur;
> > - bool full;
> > - struct binder_transaction_log_entry entry[32];
> > -};
> > -static struct binder_transaction_log binder_transaction_log;
> > -static struct binder_transaction_log binder_transaction_log_failed;
> > +struct binder_transaction_log binder_transaction_log;
> > +struct binder_transaction_log binder_transaction_log_failed;
> >
> > static struct binder_transaction_log_entry *binder_transaction_log_add(
> > struct binder_transaction_log *log)
> > @@ -6166,7 +6144,7 @@ static void print_binder_transaction_log_entry(struct
> > seq_file *m,
> > "\n" : " (incomplete)\n");
> > }
> >
> > -static int transaction_log_show(struct seq_file *m, void *unused)
> > +int binder_transaction_log_show(struct seq_file *m, void *unused)
> > {
> > struct binder_transaction_log *log = m->private;
> > unsigned int log_cur = atomic_read(>cur);
> > @@ -6198,8 +6176,6 @@ const struct file_operations binder_fops = {
> > .release = binder_release,
> > };
> >
> > -DEFINE_SHOW_ATTRIBUTE(transaction_log);
> > -
> > static int __init init_binder_device(const char *name)
> > {
> > int ret;
> > @@ -6268,12 +6244,12 @@ static int __init binder_init(void)
> > 0444,
> > binder_debugfs_dir_entry_root,
> > _transaction_log,
> > - _log_fops);
> > + _transaction_log_fops);
> > debugfs_create_file("failed_transaction_log",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > _transaction_log_failed,
> > - _log_fops);
> > + _transaction_log_fops);
> > }
> >
> > if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
> > diff --git a/dri
Re: [PATCH v2 2/4] binder: Add stats, state and transactions files
On Fri, Aug 30, 2019 at 4:32 AM Christian Brauner
wrote:
>
> On Thu, Aug 29, 2019 at 02:18:10PM -0700, Hridya Valsaraju wrote:
> > The following binder stat files currently live in debugfs.
> >
> > /sys/kernel/debug/binder/state
> > /sys/kernel/debug/binder/stats
> > /sys/kernel/debug/binder/transactions
> >
> > This patch makes these files available in a binderfs instance
> > mounted with the mount option 'stats=global'. For example, if a binderfs
> > instance is mounted at path /dev/binderfs, the above files will be
> > available at the following locations:
> >
> > /dev/binderfs/binder_logs/state
> > /dev/binderfs/binder_logs/stats
> > /dev/binderfs/binder_logs/transactions
> >
> > This provides a way to access them even when debugfs is not mounted.
> >
> > Signed-off-by: Hridya Valsaraju
>
> Just two comments below. If you have addressed them you can add my:
>
> Acked-by: Christian Brauner
Thank you for taking another look Christian, will address both
comments and send out v3 soon :)
>
> > ---
> >
> > Changes in v2:
> > - Consistently name variables across functions as per Christian
> >Brauner.
> > - Improve check for binderfs device in binderfs_evict_inode()
> >as per Christian Brauner.
> >
> > drivers/android/binder.c | 15 ++--
> > drivers/android/binder_internal.h | 8 ++
> > drivers/android/binderfs.c| 140 +-
> > 3 files changed, 153 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index ca6b21a53321..de795bd229c4 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -6055,7 +6055,7 @@ static void print_binder_proc_stats(struct seq_file
> > *m,
> > }
> >
> >
> > -static int state_show(struct seq_file *m, void *unused)
> > +int binder_state_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> > struct binder_node *node;
> > @@ -6094,7 +6094,7 @@ static int state_show(struct seq_file *m, void
> > *unused)
> > return 0;
> > }
> >
> > -static int stats_show(struct seq_file *m, void *unused)
> > +int binder_stats_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> >
> > @@ -6110,7 +6110,7 @@ static int stats_show(struct seq_file *m, void
> > *unused)
> > return 0;
> > }
> >
> > -static int transactions_show(struct seq_file *m, void *unused)
> > +int binder_transactions_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> >
> > @@ -6198,9 +6198,6 @@ const struct file_operations binder_fops = {
> > .release = binder_release,
> > };
> >
> > -DEFINE_SHOW_ATTRIBUTE(state);
> > -DEFINE_SHOW_ATTRIBUTE(stats);
> > -DEFINE_SHOW_ATTRIBUTE(transactions);
> > DEFINE_SHOW_ATTRIBUTE(transaction_log);
> >
> > static int __init init_binder_device(const char *name)
> > @@ -6256,17 +6253,17 @@ static int __init binder_init(void)
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - _fops);
> > + _state_fops);
> > debugfs_create_file("stats",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - _fops);
> > + _stats_fops);
> > debugfs_create_file("transactions",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - _fops);
> > + _transactions_fops);
> > debugfs_create_file("transaction_log",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > diff --git a/drivers/android/binder_internal.h
> > b/drivers/android/binder_internal.h
> > index fe8c745dc8e0..12ef96f256c6 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -57,4 +57,12 @@ static inline int __init init_binderfs(void)
> > }
> > #endif
> >
[PATCH v2 4/4] binder: Add binder_proc logging to binderfs
Currently /sys/kernel/debug/binder/proc contains
the debug data for every binder_proc instance.
This patch makes this information also available
in a binderfs instance mounted with a mount option
"stats=global" in addition to debugfs. The patch does
not affect the presence of the file in debugfs.
If a binderfs instance is mounted at path /dev/binderfs,
this file would be present at /dev/binderfs/binder_logs/proc.
This change provides an alternate way to access this file when debugfs
is not mounted.
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
- Consistent variable naming across functions as per Christian
Brauner.
- As suggested by Todd Kjos, log a failure to create a
process-specific binderfs log file if the error code is not EEXIST
since an error code of EEXIST is expected if
multiple contexts of the same process try to create the file during
binder_open().
drivers/android/binder.c | 46 -
drivers/android/binder_internal.h | 46 +
drivers/android/binderfs.c| 68 ++-
3 files changed, 121 insertions(+), 39 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index bed217310197..ee610ea48309 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -481,6 +481,7 @@ struct binder_priority {
* @inner_lock: can nest under outer_lock and/or node lock
* @outer_lock: no nesting under innor or node lock
*Lock order: 1) outer, 2) node, 3) inner
+ * @binderfs_entry: process-specific binderfs log file
*
* Bookkeeping structure for binder processes
*/
@@ -510,6 +511,7 @@ struct binder_proc {
struct binder_context *context;
spinlock_t inner_lock;
spinlock_t outer_lock;
+ struct dentry *binderfs_entry;
};
enum {
@@ -5347,6 +5349,8 @@ static int binder_open(struct inode *nodp, struct file
*filp)
{
struct binder_proc *proc;
struct binder_device *binder_dev;
+ struct binderfs_info *info;
+ struct dentry *binder_binderfs_dir_entry_proc = NULL;
binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d:%d\n", __func__,
current->group_leader->pid, current->pid);
@@ -5368,11 +5372,14 @@ static int binder_open(struct inode *nodp, struct file
*filp)
}
/* binderfs stashes devices in i_private */
- if (is_binderfs_device(nodp))
+ if (is_binderfs_device(nodp)) {
binder_dev = nodp->i_private;
- else
+ info = nodp->i_sb->s_fs_info;
+ binder_binderfs_dir_entry_proc = info->proc_log_dir;
+ } else {
binder_dev = container_of(filp->private_data,
struct binder_device, miscdev);
+ }
proc->context = _dev->context;
binder_alloc_init(>alloc);
@@ -5403,6 +5410,35 @@ static int binder_open(struct inode *nodp, struct file
*filp)
_fops);
}
+ if (binder_binderfs_dir_entry_proc) {
+ char strbuf[11];
+ struct dentry *binderfs_entry;
+
+ snprintf(strbuf, sizeof(strbuf), "%u", proc->pid);
+ /*
+* Similar to debugfs, the process specific log file is shared
+* between contexts. If the file has already been created for a
+* process, the following binderfs_create_file() call will
+* fail with error code EEXIST if another context of the same
+* process invoked binder_open(). This is ok since same as
+* debugfs, the log file will contain information on all
+* contexts of a given PID.
+*/
+ binderfs_entry =
binderfs_create_file(binder_binderfs_dir_entry_proc,
+ strbuf, _fops, (void *)(unsigned long)proc->pid);
+ if (!IS_ERR(binderfs_entry)) {
+ proc->binderfs_entry = binderfs_entry;
+ } else {
+ int error;
+
+ error = PTR_ERR(binderfs_entry);
+ if (error != -EEXIST) {
+ pr_warn("Unable to create file %s in binderfs
(error %d)\n",
+ strbuf, error);
+ }
+ }
+ }
+
return 0;
}
@@ -5442,6 +5478,12 @@ static int binder_release(struct inode *nodp, struct
file *filp)
struct binder_proc *proc = filp->private_data;
debugfs_remove(proc->debugfs_entry);
+
+ if (proc->binderfs_entry) {
+ binderfs_remove_file(proc->binderfs_entry);
+ proc->binderfs_entry = NULL;
+ }
+
binder_defer_work(proc, BINDER_DEFERRED_RELEASE);
return 0;
diff --git a/drivers/andro
[PATCH v2 1/4] binder: add a mount option to show global stats
Currently, all binder state and statistics live in debugfs.
We need this information even when debugfs is not mounted.
This patch adds the mount option 'stats' to enable a binderfs
instance to have binder debug information present in the same.
'stats=global' will enable the global binder statistics. In
the future, 'stats=local' will enable binder statistics local
to the binderfs instance. The two modes 'global' and 'local'
will be mutually exclusive. 'stats=global' option is only available
for a binderfs instance mounted in the initial user namespace.
An attempt to use the option to mount a binderfs instance in
another user namespace will return an EPERM error.
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
- Improve error check in binderfs_parse_mount_opts() as per Greg
Kroah-Hartman.
- Change pr_info() log before failure to pr_err() as per Greg
Kroah-Hartman.
drivers/android/binderfs.c | 45 --
1 file changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index cc2e71576396..7045bfe5b52b 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -51,18 +51,27 @@ static DEFINE_IDA(binderfs_minors);
/**
* binderfs_mount_opts - mount options for binderfs
* @max: maximum number of allocatable binderfs binder devices
+ * @stats_mode: enable binder stats in binderfs.
*/
struct binderfs_mount_opts {
int max;
+ int stats_mode;
};
enum {
Opt_max,
+ Opt_stats_mode,
Opt_err
};
+enum binderfs_stats_mode {
+ STATS_NONE,
+ STATS_GLOBAL,
+};
+
static const match_table_t tokens = {
{ Opt_max, "max=%d" },
+ { Opt_stats_mode, "stats=%s" },
{ Opt_err, NULL }
};
@@ -290,8 +299,9 @@ static void binderfs_evict_inode(struct inode *inode)
static int binderfs_parse_mount_opts(char *data,
struct binderfs_mount_opts *opts)
{
- char *p;
+ char *p, *stats;
opts->max = BINDERFS_MAX_MINOR;
+ opts->stats_mode = STATS_NONE;
while ((p = strsep(, ",")) != NULL) {
substring_t args[MAX_OPT_ARGS];
@@ -311,6 +321,22 @@ static int binderfs_parse_mount_opts(char *data,
opts->max = max_devices;
break;
+ case Opt_stats_mode:
+ if (!capable(CAP_SYS_ADMIN))
+ return -EINVAL;
+
+ stats = match_strdup([0]);
+ if (!stats)
+ return -ENOMEM;
+
+ if (strcmp(stats, "global") != 0) {
+ kfree(stats);
+ return -EINVAL;
+ }
+
+ opts->stats_mode = STATS_GLOBAL;
+ kfree(stats);
+ break;
default:
pr_err("Invalid mount options\n");
return -EINVAL;
@@ -322,8 +348,21 @@ static int binderfs_parse_mount_opts(char *data,
static int binderfs_remount(struct super_block *sb, int *flags, char *data)
{
+ int prev_stats_mode, ret;
struct binderfs_info *info = sb->s_fs_info;
- return binderfs_parse_mount_opts(data, >mount_opts);
+
+ prev_stats_mode = info->mount_opts.stats_mode;
+ ret = binderfs_parse_mount_opts(data, >mount_opts);
+ if (ret)
+ return ret;
+
+ if (prev_stats_mode != info->mount_opts.stats_mode) {
+ pr_err("Binderfs stats mode cannot be changed during a
remount\n");
+ info->mount_opts.stats_mode = prev_stats_mode;
+ return -EINVAL;
+ }
+
+ return 0;
}
static int binderfs_show_mount_opts(struct seq_file *seq, struct dentry *root)
@@ -333,6 +372,8 @@ static int binderfs_show_mount_opts(struct seq_file *seq,
struct dentry *root)
info = root->d_sb->s_fs_info;
if (info->mount_opts.max <= BINDERFS_MAX_MINOR)
seq_printf(seq, ",max=%d", info->mount_opts.max);
+ if (info->mount_opts.stats_mode == STATS_GLOBAL)
+ seq_printf(seq, ",stats=global");
return 0;
}
--
2.23.0.187.g17f5b7556c-goog
[PATCH v2 2/4] binder: Add stats, state and transactions files
The following binder stat files currently live in debugfs.
/sys/kernel/debug/binder/state
/sys/kernel/debug/binder/stats
/sys/kernel/debug/binder/transactions
This patch makes these files available in a binderfs instance
mounted with the mount option 'stats=global'. For example, if a binderfs
instance is mounted at path /dev/binderfs, the above files will be
available at the following locations:
/dev/binderfs/binder_logs/state
/dev/binderfs/binder_logs/stats
/dev/binderfs/binder_logs/transactions
This provides a way to access them even when debugfs is not mounted.
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
- Consistently name variables across functions as per Christian
Brauner.
- Improve check for binderfs device in binderfs_evict_inode()
as per Christian Brauner.
drivers/android/binder.c | 15 ++--
drivers/android/binder_internal.h | 8 ++
drivers/android/binderfs.c| 140 +-
3 files changed, 153 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ca6b21a53321..de795bd229c4 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6055,7 +6055,7 @@ static void print_binder_proc_stats(struct seq_file *m,
}
-static int state_show(struct seq_file *m, void *unused)
+int binder_state_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
struct binder_node *node;
@@ -6094,7 +6094,7 @@ static int state_show(struct seq_file *m, void *unused)
return 0;
}
-static int stats_show(struct seq_file *m, void *unused)
+int binder_stats_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
@@ -6110,7 +6110,7 @@ static int stats_show(struct seq_file *m, void *unused)
return 0;
}
-static int transactions_show(struct seq_file *m, void *unused)
+int binder_transactions_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
@@ -6198,9 +6198,6 @@ const struct file_operations binder_fops = {
.release = binder_release,
};
-DEFINE_SHOW_ATTRIBUTE(state);
-DEFINE_SHOW_ATTRIBUTE(stats);
-DEFINE_SHOW_ATTRIBUTE(transactions);
DEFINE_SHOW_ATTRIBUTE(transaction_log);
static int __init init_binder_device(const char *name)
@@ -6256,17 +6253,17 @@ static int __init binder_init(void)
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _state_fops);
debugfs_create_file("stats",
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _stats_fops);
debugfs_create_file("transactions",
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _transactions_fops);
debugfs_create_file("transaction_log",
0444,
binder_debugfs_dir_entry_root,
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index fe8c745dc8e0..12ef96f256c6 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -57,4 +57,12 @@ static inline int __init init_binderfs(void)
}
#endif
+int binder_stats_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_stats);
+
+int binder_state_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_state);
+
+int binder_transactions_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_transactions);
#endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7045bfe5b52b..0e1e7c87cd33 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -280,7 +280,7 @@ static void binderfs_evict_inode(struct inode *inode)
clear_inode(inode);
- if (!device)
+ if (!S_ISCHR(inode->i_mode) || !device)
return;
mutex_lock(_minors_mutex);
@@ -502,6 +502,141 @@ static const struct inode_operations
binderfs_dir_inode_operations = {
.unlink = binderfs_unlink,
};
+static struct inode *binderfs_make_inode(struct super_block *sb, int mode)
+{
+ struct inode *ret;
+
+ ret = new_inode(sb);
+ if (ret) {
+ ret->i_ino = iunique(sb, BINDERFS_MAX_MINOR + INODE_OFFSET);
+ ret->i_mode = mode;
+ ret->i_atime = ret->i_mtime = ret->i_ctime = current_time(ret);
+ }
+ return ret;
+}
+
+static struct dent
[PATCH v2 0/4] Add binder state and statistics to binderfs
Currently, the only way to access binder state and statistics is through debugfs. We need a way to access the same even when debugfs is not mounted. These patches add a mount option to make this information available in binderfs without affecting its presence in debugfs. The following debugfs nodes will be made available in a binderfs instance when mounted with the mount option 'stats=global' or 'stats=local'. /sys/kernel/debug/binder/failed_transaction_log /sys/kernel/debug/binder/proc /sys/kernel/debug/binder/state /sys/kernel/debug/binder/stats /sys/kernel/debug/binder/transaction_log /sys/kernel/debug/binder/transactions Hridya Valsaraju (4): binder: add a mount option to show global stats binder: Add stats, state and transactions files binder: Make transaction_log available in binderfs binder: Add binder_proc logging to binderfs drivers/android/binder.c | 95 ++- drivers/android/binder_internal.h | 84 ++ drivers/android/binderfs.c| 255 ++ 3 files changed, 362 insertions(+), 72 deletions(-) -- 2.23.0.187.g17f5b7556c-goog
[PATCH v2 3/4] binder: Make transaction_log available in binderfs
Currently, the binder transaction log files 'transaction_log'
and 'failed_transaction_log' live in debugfs at the following locations:
/sys/kernel/debug/binder/failed_transaction_log
/sys/kernel/debug/binder/transaction_log
This patch makes these files also available in a binderfs instance
mounted with the mount option "stats=global".
It does not affect the presence of these files in debugfs.
If a binderfs instance is mounted at path /dev/binderfs, the location of
these files will be as follows:
/dev/binderfs/binder_logs/failed_transaction_log
/dev/binderfs/binder_logs/transaction_log
This change provides an alternate option to access these files when
debugfs is not mounted.
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
-Consistent variable naming accross functions as per Christian Brauner.
drivers/android/binder.c | 34 +--
drivers/android/binder_internal.h | 30 +++
drivers/android/binderfs.c| 18
3 files changed, 53 insertions(+), 29 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index de795bd229c4..bed217310197 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -197,30 +197,8 @@ static inline void binder_stats_created(enum
binder_stat_types type)
atomic_inc(_stats.obj_created[type]);
}
-struct binder_transaction_log_entry {
- int debug_id;
- int debug_id_done;
- int call_type;
- int from_proc;
- int from_thread;
- int target_handle;
- int to_proc;
- int to_thread;
- int to_node;
- int data_size;
- int offsets_size;
- int return_error_line;
- uint32_t return_error;
- uint32_t return_error_param;
- const char *context_name;
-};
-struct binder_transaction_log {
- atomic_t cur;
- bool full;
- struct binder_transaction_log_entry entry[32];
-};
-static struct binder_transaction_log binder_transaction_log;
-static struct binder_transaction_log binder_transaction_log_failed;
+struct binder_transaction_log binder_transaction_log;
+struct binder_transaction_log binder_transaction_log_failed;
static struct binder_transaction_log_entry *binder_transaction_log_add(
struct binder_transaction_log *log)
@@ -6166,7 +6144,7 @@ static void print_binder_transaction_log_entry(struct
seq_file *m,
"\n" : " (incomplete)\n");
}
-static int transaction_log_show(struct seq_file *m, void *unused)
+int binder_transaction_log_show(struct seq_file *m, void *unused)
{
struct binder_transaction_log *log = m->private;
unsigned int log_cur = atomic_read(>cur);
@@ -6198,8 +6176,6 @@ const struct file_operations binder_fops = {
.release = binder_release,
};
-DEFINE_SHOW_ATTRIBUTE(transaction_log);
-
static int __init init_binder_device(const char *name)
{
int ret;
@@ -6268,12 +6244,12 @@ static int __init binder_init(void)
0444,
binder_debugfs_dir_entry_root,
_transaction_log,
- _log_fops);
+ _transaction_log_fops);
debugfs_create_file("failed_transaction_log",
0444,
binder_debugfs_dir_entry_root,
_transaction_log_failed,
- _log_fops);
+ _transaction_log_fops);
}
if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 12ef96f256c6..b9be42d9464c 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -65,4 +65,34 @@ DEFINE_SHOW_ATTRIBUTE(binder_state);
int binder_transactions_show(struct seq_file *m, void *unused);
DEFINE_SHOW_ATTRIBUTE(binder_transactions);
+
+int binder_transaction_log_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_transaction_log);
+
+struct binder_transaction_log_entry {
+ int debug_id;
+ int debug_id_done;
+ int call_type;
+ int from_proc;
+ int from_thread;
+ int target_handle;
+ int to_proc;
+ int to_thread;
+ int to_node;
+ int data_size;
+ int offsets_size;
+ int return_error_line;
+ uint32_t return_error;
+ uint32_t return_error_param;
+ const char *context_name;
+};
+
+struct binder_transaction_log {
+ atomic_t cur;
+ bool full;
+ struct binder_transaction_log_entry entry[32];
+};
+
+extern struct binder_transaction_log binder_transaction_log;
+extern struct binder_transaction_log binder_transaction_log_failed;
#endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/bind
Re: [PATCH 4/4] binder: Add binder_proc logging to binderfs
On Wed, Aug 28, 2019 at 9:21 AM Todd Kjos wrote:
>
> On Wed, Aug 28, 2019 at 6:08 AM Christian Brauner
> wrote:
> >
> > On Tue, Aug 27, 2019 at 01:41:52PM -0700, Hridya Valsaraju wrote:
> > > Currently /sys/kernel/debug/binder/proc contains
> > > the debug data for every binder_proc instance.
> > > This patch makes this information also available
> > > in a binderfs instance mounted with a mount option
> > > "stats=global" in addition to debugfs. The patch does
> > > not affect the presence of the file in debugfs.
> > >
> > > If a binderfs instance is mounted at path /dev/binderfs,
> > > this file would be present at /dev/binderfs/binder_logs/proc.
> > > This change provides an alternate way to access this file when debugfs
> > > is not mounted.
> > >
> > > Signed-off-by: Hridya Valsaraju
> >
> > I'm still wondering whether there's a nicer way to create those debuf
> > files per-process without doing it in binder_open() but it has worked
> > fine for a long time with debugfs.
> >
> > Also, one minor question below. Otherwise
> >
> > Acked-by: Christian Brauner
>
> Acked-by: Todd Kjos
>
> >
> > > ---
> > > drivers/android/binder.c | 38 ++-
> > > drivers/android/binder_internal.h | 46 ++
> > > drivers/android/binderfs.c| 63 ++-
> > > 3 files changed, 111 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index bed217310197..37189838f32a 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -481,6 +481,7 @@ struct binder_priority {
> > > * @inner_lock: can nest under outer_lock and/or node lock
> > > * @outer_lock: no nesting under innor or node lock
> > > *Lock order: 1) outer, 2) node, 3) inner
> > > + * @binderfs_entry: process-specific binderfs log file
> > > *
> > > * Bookkeeping structure for binder processes
> > > */
> > > @@ -510,6 +511,7 @@ struct binder_proc {
> > > struct binder_context *context;
> > > spinlock_t inner_lock;
> > > spinlock_t outer_lock;
> > > + struct dentry *binderfs_entry;
> > > };
> > >
> > > enum {
> > > @@ -5347,6 +5349,8 @@ static int binder_open(struct inode *nodp, struct
> > > file *filp)
> > > {
> > > struct binder_proc *proc;
> > > struct binder_device *binder_dev;
> > > + struct binderfs_info *info;
> > > + struct dentry *binder_binderfs_dir_entry_proc = NULL;
> > >
> > > binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d:%d\n", __func__,
> > >current->group_leader->pid, current->pid);
> > > @@ -5368,11 +5372,14 @@ static int binder_open(struct inode *nodp, struct
> > > file *filp)
> > > }
> > >
> > > /* binderfs stashes devices in i_private */
> > > - if (is_binderfs_device(nodp))
> > > + if (is_binderfs_device(nodp)) {
> > > binder_dev = nodp->i_private;
> > > - else
> > > + info = nodp->i_sb->s_fs_info;
> > > + binder_binderfs_dir_entry_proc = info->proc_log_dir;
> > > + } else {
> > > binder_dev = container_of(filp->private_data,
> > > struct binder_device, miscdev);
> > > + }
> > > proc->context = _dev->context;
> > > binder_alloc_init(>alloc);
> > >
> > > @@ -5403,6 +5410,27 @@ static int binder_open(struct inode *nodp, struct
> > > file *filp)
> > > _fops);
> > > }
> > >
> > > + if (binder_binderfs_dir_entry_proc) {
> > > + char strbuf[11];
> > > + struct dentry *binderfs_entry;
> > > +
> > > + snprintf(strbuf, sizeof(strbuf), "%u", proc->pid);
> > > + /*
> > > + * Similar to debugfs, the process specific log file is
> > > shared
> > > + * between contexts. If the file has already been created
> > > for a
> > > + * process, the following binderfs_create_file() call will
> > > + * fail if anothe
Re: [PATCH 2/4] binder: Add stats, state and transactions files
On Wed, Aug 28, 2019 at 5:58 AM Christian Brauner
wrote:
>
> On Tue, Aug 27, 2019 at 01:41:50PM -0700, Hridya Valsaraju wrote:
> > The following binder stat files currently live in debugfs.
> >
> > /sys/kernel/debug/binder/state
> > /sys/kernel/debug/binder/stats
> > /sys/kernel/debug/binder/transactions
> >
> > This patch makes these files available in a binderfs instance
> > mounted with the mount option 'stats=global'. For example, if a binderfs
> > instance is mounted at path /dev/binderfs, the above files will be
> > available at the following locations:
> >
> > /dev/binderfs/binder_logs/state
> > /dev/binderfs/binder_logs/stats
> > /dev/binderfs/binder_logs/transactions
> >
> > This provides a way to access them even when debugfs is not mounted.
> >
> > Signed-off-by: Hridya Valsaraju
> > ---
> > drivers/android/binder.c | 15 ++--
> > drivers/android/binder_internal.h | 8 ++
> > drivers/android/binderfs.c| 137 +-
> > 3 files changed, 150 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index ca6b21a53321..de795bd229c4 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -6055,7 +6055,7 @@ static void print_binder_proc_stats(struct seq_file
> > *m,
> > }
> >
> >
> > -static int state_show(struct seq_file *m, void *unused)
> > +int binder_state_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> > struct binder_node *node;
> > @@ -6094,7 +6094,7 @@ static int state_show(struct seq_file *m, void
> > *unused)
> > return 0;
> > }
> >
> > -static int stats_show(struct seq_file *m, void *unused)
> > +int binder_stats_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> >
> > @@ -6110,7 +6110,7 @@ static int stats_show(struct seq_file *m, void
> > *unused)
> > return 0;
> > }
> >
> > -static int transactions_show(struct seq_file *m, void *unused)
> > +int binder_transactions_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> >
> > @@ -6198,9 +6198,6 @@ const struct file_operations binder_fops = {
> > .release = binder_release,
> > };
> >
> > -DEFINE_SHOW_ATTRIBUTE(state);
> > -DEFINE_SHOW_ATTRIBUTE(stats);
> > -DEFINE_SHOW_ATTRIBUTE(transactions);
> > DEFINE_SHOW_ATTRIBUTE(transaction_log);
> >
> > static int __init init_binder_device(const char *name)
> > @@ -6256,17 +6253,17 @@ static int __init binder_init(void)
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - _fops);
> > + _state_fops);
> > debugfs_create_file("stats",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - _fops);
> > + _stats_fops);
> > debugfs_create_file("transactions",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - _fops);
> > + _transactions_fops);
> > debugfs_create_file("transaction_log",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > diff --git a/drivers/android/binder_internal.h
> > b/drivers/android/binder_internal.h
> > index fe8c745dc8e0..12ef96f256c6 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -57,4 +57,12 @@ static inline int __init init_binderfs(void)
> > }
> > #endif
> >
> > +int binder_stats_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_stats);
> > +
> > +int binder_state_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_state);
> > +
> > +int binder_transactions_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_transactions);
> > #endif /* _LINUX_BINDER_INTERNAL_H */
> > diff --git a/drivers/android/binderfs.c b/dri
Re: [PATCH 1/4] binder: add a mount option to show global stats
On Wed, Aug 28, 2019 at 5:39 AM Christian Brauner
wrote:
>
> On Wed, Aug 28, 2019 at 11:22:37AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 27, 2019 at 01:41:49PM -0700, Hridya Valsaraju wrote:
> > > Currently, all binder state and statistics live in debugfs.
> > > We need this information even when debugfs is not mounted.
> > > This patch adds the mount option 'stats' to enable a binderfs
> > > instance to have binder debug information present in the same.
> > > 'stats=global' will enable the global binder statistics. In
> > > the future, 'stats=local' will enable binder statistics local
> > > to the binderfs instance. The two modes 'global' and 'local'
> > > will be mutually exclusive. 'stats=global' option is only available
> > > for a binderfs instance mounted in the initial user namespace.
> > > An attempt to use the option to mount a binderfs instance in
> > > another user namespace will return an EPERM error.
> > >
> > > Signed-off-by: Hridya Valsaraju
> > > ---
> > > drivers/android/binderfs.c | 47 --
> > > 1 file changed, 45 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > > index cc2e71576396..d95d179aec58 100644
> > > --- a/drivers/android/binderfs.c
> > > +++ b/drivers/android/binderfs.c
> > > @@ -51,18 +51,27 @@ static DEFINE_IDA(binderfs_minors);
> > > /**
> > > * binderfs_mount_opts - mount options for binderfs
> > > * @max: maximum number of allocatable binderfs binder devices
> > > + * @stats_mode: enable binder stats in binderfs.
> > > */
> > > struct binderfs_mount_opts {
> > > int max;
> > > + int stats_mode;
> > > };
> > >
> > > enum {
> > > Opt_max,
> > > + Opt_stats_mode,
> > > Opt_err
> > > };
> > >
> > > +enum binderfs_stats_mode {
> > > + STATS_NONE,
> > > + STATS_GLOBAL,
> > > +};
> > > +
> > > static const match_table_t tokens = {
> > > { Opt_max, "max=%d" },
> > > + { Opt_stats_mode, "stats=%s" },
> > > { Opt_err, NULL }
> > > };
> > >
> > > @@ -290,8 +299,9 @@ static void binderfs_evict_inode(struct inode *inode)
> > > static int binderfs_parse_mount_opts(char *data,
> > > struct binderfs_mount_opts *opts)
> > > {
> > > - char *p;
> > > + char *p, *stats;
> > > opts->max = BINDERFS_MAX_MINOR;
> > > + opts->stats_mode = STATS_NONE;
> > >
> > > while ((p = strsep(, ",")) != NULL) {
> > > substring_t args[MAX_OPT_ARGS];
> > > @@ -311,6 +321,24 @@ static int binderfs_parse_mount_opts(char *data,
> > >
> > > opts->max = max_devices;
> > > break;
> > > + case Opt_stats_mode:
> > > + stats = match_strdup([0]);
> > > + if (!stats)
> > > + return -ENOMEM;
> > > +
> > > + if (strcmp(stats, "global") != 0) {
> > > + kfree(stats);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (!capable(CAP_SYS_ADMIN)) {
> > > + kfree(stats);
> > > + return -EINVAL;
> >
> > Can a non-CAP_SYS_ADMIN task even call this function? Anyway, if it
>
> It can. A task that has CAP_SYS_ADMIN in the userns the corresponding
> binderfs mount has been created in can change the max= mount option.
> Only stats=global currently requires capable(CAP_SYS_ADMIN) aka
> CAP_SYS_ADMIN in the initial userns to prevent non-initial userns from
> snooping at global statistics.
>
> > can, put the check at the top of the case, and just return early before
> > doing any extra work like checking values or allocating memory.
Thank you Greg and Christian for reviewing the patch! That makes
sense, I will make the change and send out v2 soon.
> >
> > > + }
> > > +
> > > + opts->stats_mode = STATS_GLOBAL;
> > > + kfree(stats);
> > > + break;
> > > default:
> > > pr_err("Invalid mount options\n");
> > > return -EINVAL;
> > > @@ -322,8 +350,21 @@ static int binderfs_parse_mount_opts(char *data,
> > >
> > > static int binderfs_remount(struct super_block *sb, int *flags, char
> > > *data)
> > > {
> > > + int prev_stats_mode, ret;
> > > struct binderfs_info *info = sb->s_fs_info;
> > > - return binderfs_parse_mount_opts(data, >mount_opts);
> > > +
> > > + prev_stats_mode = info->mount_opts.stats_mode;
> > > + ret = binderfs_parse_mount_opts(data, >mount_opts);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (prev_stats_mode != info->mount_opts.stats_mode) {
> > > + pr_info("Binderfs stats mode cannot be changed during a
> > > remount\n");
> >
> > pr_err()?
> >
> > thanks,
> >
> > greg k-h
[PATCH 3/4] binder: Make transaction_log available in binderfs
Currently, the binder transaction log files 'transaction_log'
and 'failed_transaction_log' live in debugfs at the following locations:
/sys/kernel/debug/binder/failed_transaction_log
/sys/kernel/debug/binder/transaction_log
This patch makes these files also available in a binderfs instance
mounted with the mount option "stats=global".
It does not affect the presence of these files in debugfs.
If a binderfs instance is mounted at path /dev/binderfs, the location of
these files will be as follows:
/dev/binderfs/binder_logs/failed_transaction_log
/dev/binderfs/binder_logs/transaction_log
This change provides an alternate option to access these files when
debugfs is not mounted.
Signed-off-by: Hridya Valsaraju
---
drivers/android/binder.c | 34 +--
drivers/android/binder_internal.h | 30 +++
drivers/android/binderfs.c| 19 +
3 files changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index de795bd229c4..bed217310197 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -197,30 +197,8 @@ static inline void binder_stats_created(enum
binder_stat_types type)
atomic_inc(_stats.obj_created[type]);
}
-struct binder_transaction_log_entry {
- int debug_id;
- int debug_id_done;
- int call_type;
- int from_proc;
- int from_thread;
- int target_handle;
- int to_proc;
- int to_thread;
- int to_node;
- int data_size;
- int offsets_size;
- int return_error_line;
- uint32_t return_error;
- uint32_t return_error_param;
- const char *context_name;
-};
-struct binder_transaction_log {
- atomic_t cur;
- bool full;
- struct binder_transaction_log_entry entry[32];
-};
-static struct binder_transaction_log binder_transaction_log;
-static struct binder_transaction_log binder_transaction_log_failed;
+struct binder_transaction_log binder_transaction_log;
+struct binder_transaction_log binder_transaction_log_failed;
static struct binder_transaction_log_entry *binder_transaction_log_add(
struct binder_transaction_log *log)
@@ -6166,7 +6144,7 @@ static void print_binder_transaction_log_entry(struct
seq_file *m,
"\n" : " (incomplete)\n");
}
-static int transaction_log_show(struct seq_file *m, void *unused)
+int binder_transaction_log_show(struct seq_file *m, void *unused)
{
struct binder_transaction_log *log = m->private;
unsigned int log_cur = atomic_read(>cur);
@@ -6198,8 +6176,6 @@ const struct file_operations binder_fops = {
.release = binder_release,
};
-DEFINE_SHOW_ATTRIBUTE(transaction_log);
-
static int __init init_binder_device(const char *name)
{
int ret;
@@ -6268,12 +6244,12 @@ static int __init binder_init(void)
0444,
binder_debugfs_dir_entry_root,
_transaction_log,
- _log_fops);
+ _transaction_log_fops);
debugfs_create_file("failed_transaction_log",
0444,
binder_debugfs_dir_entry_root,
_transaction_log_failed,
- _log_fops);
+ _transaction_log_fops);
}
if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 12ef96f256c6..b9be42d9464c 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -65,4 +65,34 @@ DEFINE_SHOW_ATTRIBUTE(binder_state);
int binder_transactions_show(struct seq_file *m, void *unused);
DEFINE_SHOW_ATTRIBUTE(binder_transactions);
+
+int binder_transaction_log_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_transaction_log);
+
+struct binder_transaction_log_entry {
+ int debug_id;
+ int debug_id_done;
+ int call_type;
+ int from_proc;
+ int from_thread;
+ int target_handle;
+ int to_proc;
+ int to_thread;
+ int to_node;
+ int data_size;
+ int offsets_size;
+ int return_error_line;
+ uint32_t return_error;
+ uint32_t return_error_param;
+ const char *context_name;
+};
+
+struct binder_transaction_log {
+ atomic_t cur;
+ bool full;
+ struct binder_transaction_log_entry entry[32];
+};
+
+extern struct binder_transaction_log binder_transaction_log;
+extern struct binder_transaction_log binder_transaction_log_failed;
#endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index d542f9b8d8ab..dc25a7d759c9 100644
--- a/drivers/android
[PATCH 4/4] binder: Add binder_proc logging to binderfs
Currently /sys/kernel/debug/binder/proc contains
the debug data for every binder_proc instance.
This patch makes this information also available
in a binderfs instance mounted with a mount option
"stats=global" in addition to debugfs. The patch does
not affect the presence of the file in debugfs.
If a binderfs instance is mounted at path /dev/binderfs,
this file would be present at /dev/binderfs/binder_logs/proc.
This change provides an alternate way to access this file when debugfs
is not mounted.
Signed-off-by: Hridya Valsaraju
---
drivers/android/binder.c | 38 ++-
drivers/android/binder_internal.h | 46 ++
drivers/android/binderfs.c| 63 ++-
3 files changed, 111 insertions(+), 36 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index bed217310197..37189838f32a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -481,6 +481,7 @@ struct binder_priority {
* @inner_lock: can nest under outer_lock and/or node lock
* @outer_lock: no nesting under innor or node lock
*Lock order: 1) outer, 2) node, 3) inner
+ * @binderfs_entry: process-specific binderfs log file
*
* Bookkeeping structure for binder processes
*/
@@ -510,6 +511,7 @@ struct binder_proc {
struct binder_context *context;
spinlock_t inner_lock;
spinlock_t outer_lock;
+ struct dentry *binderfs_entry;
};
enum {
@@ -5347,6 +5349,8 @@ static int binder_open(struct inode *nodp, struct file
*filp)
{
struct binder_proc *proc;
struct binder_device *binder_dev;
+ struct binderfs_info *info;
+ struct dentry *binder_binderfs_dir_entry_proc = NULL;
binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d:%d\n", __func__,
current->group_leader->pid, current->pid);
@@ -5368,11 +5372,14 @@ static int binder_open(struct inode *nodp, struct file
*filp)
}
/* binderfs stashes devices in i_private */
- if (is_binderfs_device(nodp))
+ if (is_binderfs_device(nodp)) {
binder_dev = nodp->i_private;
- else
+ info = nodp->i_sb->s_fs_info;
+ binder_binderfs_dir_entry_proc = info->proc_log_dir;
+ } else {
binder_dev = container_of(filp->private_data,
struct binder_device, miscdev);
+ }
proc->context = _dev->context;
binder_alloc_init(>alloc);
@@ -5403,6 +5410,27 @@ static int binder_open(struct inode *nodp, struct file
*filp)
_fops);
}
+ if (binder_binderfs_dir_entry_proc) {
+ char strbuf[11];
+ struct dentry *binderfs_entry;
+
+ snprintf(strbuf, sizeof(strbuf), "%u", proc->pid);
+ /*
+* Similar to debugfs, the process specific log file is shared
+* between contexts. If the file has already been created for a
+* process, the following binderfs_create_file() call will
+* fail if another context of the same process invoked
+* binder_open(). This is ok since same as debugfs,
+* the log file will contain information on all contexts of a
+* given PID.
+*/
+ binderfs_entry =
binderfs_create_file(binder_binderfs_dir_entry_proc,
+ strbuf, _fops, (void *)(unsigned long)proc->pid);
+ if (!IS_ERR(binderfs_entry))
+ proc->binderfs_entry = binderfs_entry;
+
+ }
+
return 0;
}
@@ -5442,6 +5470,12 @@ static int binder_release(struct inode *nodp, struct
file *filp)
struct binder_proc *proc = filp->private_data;
debugfs_remove(proc->debugfs_entry);
+
+ if (proc->binderfs_entry) {
+ binderfs_remove_file(proc->binderfs_entry);
+ proc->binderfs_entry = NULL;
+ }
+
binder_defer_work(proc, BINDER_DEFERRED_RELEASE);
return 0;
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index b9be42d9464c..bd47f7f72075 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -35,17 +35,63 @@ struct binder_device {
struct inode *binderfs_inode;
};
+/**
+ * binderfs_mount_opts - mount options for binderfs
+ * @max: maximum number of allocatable binderfs binder devices
+ * @stats_mode: enable binder stats in binderfs.
+ */
+struct binderfs_mount_opts {
+ int max;
+ int stats_mode;
+};
+
+/**
+ * binderfs_info - information about a binderfs mount
+ * @ipc_ns: The ipc namespace the binderfs mount belongs to.
+ * @control_dentry: This records the dentry of this binderfs mount
+ * binder-
[PATCH 2/4] binder: Add stats, state and transactions files
The following binder stat files currently live in debugfs.
/sys/kernel/debug/binder/state
/sys/kernel/debug/binder/stats
/sys/kernel/debug/binder/transactions
This patch makes these files available in a binderfs instance
mounted with the mount option 'stats=global'. For example, if a binderfs
instance is mounted at path /dev/binderfs, the above files will be
available at the following locations:
/dev/binderfs/binder_logs/state
/dev/binderfs/binder_logs/stats
/dev/binderfs/binder_logs/transactions
This provides a way to access them even when debugfs is not mounted.
Signed-off-by: Hridya Valsaraju
---
drivers/android/binder.c | 15 ++--
drivers/android/binder_internal.h | 8 ++
drivers/android/binderfs.c| 137 +-
3 files changed, 150 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ca6b21a53321..de795bd229c4 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6055,7 +6055,7 @@ static void print_binder_proc_stats(struct seq_file *m,
}
-static int state_show(struct seq_file *m, void *unused)
+int binder_state_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
struct binder_node *node;
@@ -6094,7 +6094,7 @@ static int state_show(struct seq_file *m, void *unused)
return 0;
}
-static int stats_show(struct seq_file *m, void *unused)
+int binder_stats_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
@@ -6110,7 +6110,7 @@ static int stats_show(struct seq_file *m, void *unused)
return 0;
}
-static int transactions_show(struct seq_file *m, void *unused)
+int binder_transactions_show(struct seq_file *m, void *unused)
{
struct binder_proc *proc;
@@ -6198,9 +6198,6 @@ const struct file_operations binder_fops = {
.release = binder_release,
};
-DEFINE_SHOW_ATTRIBUTE(state);
-DEFINE_SHOW_ATTRIBUTE(stats);
-DEFINE_SHOW_ATTRIBUTE(transactions);
DEFINE_SHOW_ATTRIBUTE(transaction_log);
static int __init init_binder_device(const char *name)
@@ -6256,17 +6253,17 @@ static int __init binder_init(void)
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _state_fops);
debugfs_create_file("stats",
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _stats_fops);
debugfs_create_file("transactions",
0444,
binder_debugfs_dir_entry_root,
NULL,
- _fops);
+ _transactions_fops);
debugfs_create_file("transaction_log",
0444,
binder_debugfs_dir_entry_root,
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index fe8c745dc8e0..12ef96f256c6 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -57,4 +57,12 @@ static inline int __init init_binderfs(void)
}
#endif
+int binder_stats_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_stats);
+
+int binder_state_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_state);
+
+int binder_transactions_show(struct seq_file *m, void *unused);
+DEFINE_SHOW_ATTRIBUTE(binder_transactions);
#endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index d95d179aec58..d542f9b8d8ab 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -280,7 +280,7 @@ static void binderfs_evict_inode(struct inode *inode)
clear_inode(inode);
- if (!device)
+ if (!device || S_ISREG(inode->i_mode))
return;
mutex_lock(_minors_mutex);
@@ -504,6 +504,138 @@ static const struct inode_operations
binderfs_dir_inode_operations = {
.unlink = binderfs_unlink,
};
+static struct inode *binderfs_make_inode(struct super_block *sb, int mode)
+{
+ struct inode *ret;
+
+ ret = new_inode(sb);
+ if (ret) {
+ ret->i_ino = iunique(sb, BINDERFS_MAX_MINOR + INODE_OFFSET);
+ ret->i_mode = mode;
+ ret->i_atime = ret->i_mtime = ret->i_ctime = current_time(ret);
+ }
+ return ret;
+}
+
+static struct dentry *binderfs_create_dentry(struct dentry *dir,
+const char *name)
+{
+ struct dentry *dentry;
+
+ dentry = lookup_one_len(name, dir, strlen(name));
+ i
[PATCH 1/4] binder: add a mount option to show global stats
Currently, all binder state and statistics live in debugfs.
We need this information even when debugfs is not mounted.
This patch adds the mount option 'stats' to enable a binderfs
instance to have binder debug information present in the same.
'stats=global' will enable the global binder statistics. In
the future, 'stats=local' will enable binder statistics local
to the binderfs instance. The two modes 'global' and 'local'
will be mutually exclusive. 'stats=global' option is only available
for a binderfs instance mounted in the initial user namespace.
An attempt to use the option to mount a binderfs instance in
another user namespace will return an EPERM error.
Signed-off-by: Hridya Valsaraju
---
drivers/android/binderfs.c | 47 --
1 file changed, 45 insertions(+), 2 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index cc2e71576396..d95d179aec58 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -51,18 +51,27 @@ static DEFINE_IDA(binderfs_minors);
/**
* binderfs_mount_opts - mount options for binderfs
* @max: maximum number of allocatable binderfs binder devices
+ * @stats_mode: enable binder stats in binderfs.
*/
struct binderfs_mount_opts {
int max;
+ int stats_mode;
};
enum {
Opt_max,
+ Opt_stats_mode,
Opt_err
};
+enum binderfs_stats_mode {
+ STATS_NONE,
+ STATS_GLOBAL,
+};
+
static const match_table_t tokens = {
{ Opt_max, "max=%d" },
+ { Opt_stats_mode, "stats=%s" },
{ Opt_err, NULL }
};
@@ -290,8 +299,9 @@ static void binderfs_evict_inode(struct inode *inode)
static int binderfs_parse_mount_opts(char *data,
struct binderfs_mount_opts *opts)
{
- char *p;
+ char *p, *stats;
opts->max = BINDERFS_MAX_MINOR;
+ opts->stats_mode = STATS_NONE;
while ((p = strsep(, ",")) != NULL) {
substring_t args[MAX_OPT_ARGS];
@@ -311,6 +321,24 @@ static int binderfs_parse_mount_opts(char *data,
opts->max = max_devices;
break;
+ case Opt_stats_mode:
+ stats = match_strdup([0]);
+ if (!stats)
+ return -ENOMEM;
+
+ if (strcmp(stats, "global") != 0) {
+ kfree(stats);
+ return -EINVAL;
+ }
+
+ if (!capable(CAP_SYS_ADMIN)) {
+ kfree(stats);
+ return -EINVAL;
+ }
+
+ opts->stats_mode = STATS_GLOBAL;
+ kfree(stats);
+ break;
default:
pr_err("Invalid mount options\n");
return -EINVAL;
@@ -322,8 +350,21 @@ static int binderfs_parse_mount_opts(char *data,
static int binderfs_remount(struct super_block *sb, int *flags, char *data)
{
+ int prev_stats_mode, ret;
struct binderfs_info *info = sb->s_fs_info;
- return binderfs_parse_mount_opts(data, >mount_opts);
+
+ prev_stats_mode = info->mount_opts.stats_mode;
+ ret = binderfs_parse_mount_opts(data, >mount_opts);
+ if (ret)
+ return ret;
+
+ if (prev_stats_mode != info->mount_opts.stats_mode) {
+ pr_info("Binderfs stats mode cannot be changed during a
remount\n");
+ info->mount_opts.stats_mode = prev_stats_mode;
+ return -EINVAL;
+ }
+
+ return 0;
}
static int binderfs_show_mount_opts(struct seq_file *seq, struct dentry *root)
@@ -333,6 +374,8 @@ static int binderfs_show_mount_opts(struct seq_file *seq,
struct dentry *root)
info = root->d_sb->s_fs_info;
if (info->mount_opts.max <= BINDERFS_MAX_MINOR)
seq_printf(seq, ",max=%d", info->mount_opts.max);
+ if (info->mount_opts.stats_mode == STATS_GLOBAL)
+ seq_printf(seq, ",stats=global");
return 0;
}
--
2.23.0.187.g17f5b7556c-goog
[PATCH 0/4] Add binder state and statistics to binderfs
Currently, the only way to access binder state and statistics is through debugfs. We need a way to access the same even when debugfs is not mounted. These patches add a mount option to make this information available in binderfs without affecting its presence in debugfs. The following debugfs nodes will be made available in a binderfs instance when mounted with the mount option 'stats=global' or 'stats=local'. /sys/kernel/debug/binder/failed_transaction_log /sys/kernel/debug/binder/proc /sys/kernel/debug/binder/state /sys/kernel/debug/binder/stats /sys/kernel/debug/binder/transaction_log /sys/kernel/debug/binder/transactions Hridya Valsaraju (4): binder: add a mount option to show global stats binder: Add stats, state and transactions files binder: Make transaction_log available in binderfs binder: Add binder_proc logging to binderfs drivers/android/binder.c | 87 +- drivers/android/binder_internal.h | 84 ++ drivers/android/binderfs.c| 256 ++ 3 files changed, 355 insertions(+), 72 deletions(-) -- 2.23.0.187.g17f5b7556c-goog
Re: [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
On Fri, Aug 9, 2019 at 11:22 AM Christian Brauner wrote: > > On Fri, Aug 09, 2019 at 04:50:16PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Aug 08, 2019 at 03:27:25PM -0700, Hridya Valsaraju wrote: > > > Currently, since each binderfs instance needs its own > > > private binder devices, every time a binderfs instance is > > > mounted, all the default binder devices need to be created > > > via the BINDER_CTL_ADD IOCTL. > > > > Wasn't that a design goal of binderfs? > > Sure, but if you solely rely binderfs to be used to provide binder > devices having them pre-created on each mount makes quite some sense, > imho. > > > > > > This patch aims to > > > add a solution to automatically create the default binder > > > devices for each binderfs instance that gets mounted. > > > To achieve this goal, when CONFIG_ANDROID_BINDERFS is set, > > > the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES > > > are created in each binderfs instance instead of global devices > > > being created by the binder driver. > > > > This is going to change how things work today, what is going to break > > because of this change? > > > > I don't object to this, except for the worry of changing the default > > behavior. > > This is something that Hridya and Todd can speak better to given that > they suggested this change. :) > From my perspective, binderfs binder devices and the regular binder > driver are mostly used mutually exclusive in practice atm so that this > change has little chance of breaking anyone. As Christian says, we do not anticipate the change to break any existing use cases since binderfs binder devices and regular binder devices are generally not used simultaneously. Hopefully, there are not a lot of unusual use cases since binderfs itself is relatively new as well :) > > Now I really need to go back to vacation time - which I suck at > apparently. :) > > Christian
Re: [PATCH v3 2/2] binder: Validate the default binderfs device names.
On Fri, Aug 9, 2019 at 11:14 AM Christian Brauner
wrote:
>
> On Fri, Aug 09, 2019 at 04:55:08PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 08, 2019 at 03:27:26PM -0700, Hridya Valsaraju wrote:
> > > Length of a binderfs device name cannot exceed BINDERFS_MAX_NAME.
> > > This patch adds a check in binderfs_init() to ensure the same
> > > for the default binder devices that will be created in every
> > > binderfs instance.
> > >
> > > Co-developed-by: Christian Brauner
> > > Signed-off-by: Christian Brauner
> > > Signed-off-by: Hridya Valsaraju
> > > ---
> > > drivers/android/binderfs.c | 12
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > > index aee46dd1be91..55c5adb87585 100644
> > > --- a/drivers/android/binderfs.c
> > > +++ b/drivers/android/binderfs.c
> > > @@ -570,6 +570,18 @@ static struct file_system_type binder_fs_type = {
> > > int __init init_binderfs(void)
> > > {
> > > int ret;
> > > + const char *name;
> > > + size_t len;
> > > +
> > > + /* Verify that the default binderfs device names are valid. */
> >
> > And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?
> >
> > > + name = binder_devices_param;
> > > + for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> > > + if (len > BINDERFS_MAX_NAME)
> > > + return -E2BIG;
> > > + name += len;
> > > + if (*name == ',')
> > > + name++;
> > > + }
> >
> > We already tokenize the binderfs device names in binder_init(), why not
> > check this there instead? Parsing the same string over and over isn't
> > the nicest.
>
> non-binderfs binder devices do not have their limit set to
> BINDERFS_NAME_MAX. That's why the check has likely been made specific to
> binderfs binder devices which do have that limit.
Thank you Greg and Christian, for taking another look. Yes,
non-binderfs binder devices not having this limitation is the reason
why the check was made specific to binderfs devices. Also, when
CONFIG_ANDROID_BINDERFS is set, patch 1/2 disabled the same string
being parsed in binder_init().
>
> But, in practice, 255 is the standard path-part limit that no-one really
> exceeds especially not for stuff such as device nodes which usually have
> rather standard naming schemes (e.g. binder, vndbinder, hwbinder, etc.).
> So yes, we can move that check before both the binderfs binder device
> and non-binderfs binder device parsing code and treat it as a generic
> check.
> Then we can also backport that check as you requested in the other mail.
> Unless Hridya or Todd have objections, of course.
I do not have any objections to adding a generic check in binder_init() instead.
>
> Christian
[PATCH v3 0/2] Add default binderfs devices
Binderfs was created to help provide private binder devices to containers in their own IPC namespace. Currently, every time a new binderfs instance is mounted, its private binder devices need to be created via IOCTL calls. This patch series eliminates the effort for creating the default binder devices for each binderfs instance by creating them automatically. Hridya Valsaraju (2): binder: Add default binder devices through binderfs when configured binder: Validate the default binderfs device names. drivers/android/binder.c | 5 +++-- drivers/android/binder_internal.h | 2 ++ drivers/android/binderfs.c| 35 --- 3 files changed, 37 insertions(+), 5 deletions(-) -- 2.22.0.770.g0f2c4a37fd-goog
[PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
Currently, since each binderfs instance needs its own
private binder devices, every time a binderfs instance is
mounted, all the default binder devices need to be created
via the BINDER_CTL_ADD IOCTL. This patch aims to
add a solution to automatically create the default binder
devices for each binderfs instance that gets mounted.
To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
are created in each binderfs instance instead of global devices
being created by the binder driver.
Co-developed-by: Christian Brauner
Signed-off-by: Christian Brauner
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
- Updated commit message as per Greg Kroah-Hartman.
- Removed new module parameter creation as per Greg
Kroah-Hartman/Christian Brauner.
- Refactored device name length check into a new patch as per Greg
Kroah-Hartman.
Changes in v3:
-Removed unnecessary empty lines as per Dan Carpenter.
drivers/android/binder.c | 5 +++--
drivers/android/binder_internal.h | 2 ++
drivers/android/binderfs.c| 23 ---
3 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 466b6a7f8ab7..ca6b21a53321 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -123,7 +123,7 @@ static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR
|
BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
module_param_named(debug_mask, binder_debug_mask, uint, 0644);
-static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
+char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
module_param_named(devices, binder_devices_param, charp, 0444);
static DECLARE_WAIT_QUEUE_HEAD(binder_user_error_wait);
@@ -6279,7 +6279,8 @@ static int __init binder_init(void)
_log_fops);
}
- if (strcmp(binder_devices_param, "") != 0) {
+ if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
+ strcmp(binder_devices_param, "") != 0) {
/*
* Copy the module_parameter string, because we don't want to
* tokenize it in-place.
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 045b3e42d98b..fe8c745dc8e0 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -37,6 +37,8 @@ struct binder_device {
extern const struct file_operations binder_fops;
+extern char *binder_devices_param;
+
#ifdef CONFIG_ANDROID_BINDERFS
extern bool is_binderfs_device(const struct inode *inode);
#else
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e773f45d19d9..aee46dd1be91 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -186,8 +186,7 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
req->major = MAJOR(binderfs_dev);
req->minor = minor;
- ret = copy_to_user(userp, req, sizeof(*req));
- if (ret) {
+ if (userp && copy_to_user(userp, req, sizeof(*req))) {
ret = -EFAULT;
goto err;
}
@@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void
*data, int silent)
int ret;
struct binderfs_info *info;
struct inode *inode = NULL;
+ struct binderfs_device device_info = { 0 };
+ const char *name;
+ size_t len;
sb->s_blocksize = PAGE_SIZE;
sb->s_blocksize_bits = PAGE_SHIFT;
@@ -521,7 +523,22 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
if (!sb->s_root)
return -ENOMEM;
- return binderfs_binder_ctl_create(sb);
+ ret = binderfs_binder_ctl_create(sb);
+ if (ret)
+ return ret;
+
+ name = binder_devices_param;
+ for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+ strscpy(device_info.name, name, len + 1);
+ ret = binderfs_binder_device_create(inode, NULL, _info);
+ if (ret)
+ return ret;
+ name += len;
+ if (*name == ',')
+ name++;
+ }
+
+ return 0;
}
static struct dentry *binderfs_mount(struct file_system_type *fs_type,
--
2.22.0.770.g0f2c4a37fd-goog
[PATCH v3 2/2] binder: Validate the default binderfs device names.
Length of a binderfs device name cannot exceed BINDERFS_MAX_NAME.
This patch adds a check in binderfs_init() to ensure the same
for the default binder devices that will be created in every
binderfs instance.
Co-developed-by: Christian Brauner
Signed-off-by: Christian Brauner
Signed-off-by: Hridya Valsaraju
---
drivers/android/binderfs.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index aee46dd1be91..55c5adb87585 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -570,6 +570,18 @@ static struct file_system_type binder_fs_type = {
int __init init_binderfs(void)
{
int ret;
+ const char *name;
+ size_t len;
+
+ /* Verify that the default binderfs device names are valid. */
+ name = binder_devices_param;
+ for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+ if (len > BINDERFS_MAX_NAME)
+ return -E2BIG;
+ name += len;
+ if (*name == ',')
+ name++;
+ }
/* Allocate new major number for binderfs. */
ret = alloc_chrdev_region(_dev, 0, BINDERFS_MAX_MINOR,
--
2.22.0.770.g0f2c4a37fd-goog
Re: [PATCH v2 1/2] binder: Add default binder devices through binderfs when configured
On Wed, Aug 7, 2019 at 4:02 AM Dan Carpenter wrote:
>
> On Tue, Aug 06, 2019 at 11:40:05AM -0700, Hridya Valsaraju wrote:
> > @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb,
> > void *data, int silent)
> > int ret;
> > struct binderfs_info *info;
> > struct inode *inode = NULL;
> > + struct binderfs_device device_info = { 0 };
> > + const char *name;
> > + size_t len;
> >
> > sb->s_blocksize = PAGE_SIZE;
> > sb->s_blocksize_bits = PAGE_SHIFT;
> > @@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block *sb,
> > void *data, int silent)
> > if (!sb->s_root)
> > return -ENOMEM;
> >
> > - return binderfs_binder_ctl_create(sb);
> > + ret = binderfs_binder_ctl_create(sb);
> > + if (ret)
> > + return ret;
> > +
> > + name = binder_devices_param;
> > + for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> > + strscpy(device_info.name, name, len + 1);
> > + ret = binderfs_binder_device_create(inode, NULL,
> > _info);
> > + if (ret)
> > + return ret;
>
> We should probably clean up before returning... The error handling code
> would probably be tricky to write though and it's not super common.
Thank you for taking a look Dan. Did you mean cleaning up the default
devices that were already created? They will actually be cleaned up by
binderfs_evict_inode() during the super block's cleanup since the
mount operation will fail due to an error here.
>
> > + name += len;
> > + if (*name == ',')
> > + name++;
> > +
> > + }
> > +
> > + return 0;
> > +
>
> Remove this extra blank line.
Will do in v3, thanks for catching this Dan!
>
> > }
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-team+unsubscr...@android.com.
>
[PATCH v2 1/2] binder: Add default binder devices through binderfs when configured
Currently, since each binderfs instance needs its own
private binder devices, every time a binderfs instance is
mounted, all the default binder devices need to be created
via the BINDER_CTL_ADD IOCTL. This patch aims to
add a solution to automatically create the default binder
devices for each binderfs instance that gets mounted.
To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
are created in each binderfs instance instead of global devices
being created by the binder driver.
Co-developed-by: Christian Brauner
Signed-off-by: Christian Brauner
Signed-off-by: Hridya Valsaraju
---
Changes in v2:
- Updated commit message as per Greg Kroah-Hartman.
- Removed new module parameter creation as per Greg
Kroah-Hartman/Christian Brauner.
- Refactored device name length check into a new patch as per Greg
Kroah-Hartman.
drivers/android/binder.c | 5 +++--
drivers/android/binder_internal.h | 2 ++
drivers/android/binderfs.c| 25 ++---
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 466b6a7f8ab7..ca6b21a53321 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -123,7 +123,7 @@ static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR
|
BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
module_param_named(debug_mask, binder_debug_mask, uint, 0644);
-static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
+char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
module_param_named(devices, binder_devices_param, charp, 0444);
static DECLARE_WAIT_QUEUE_HEAD(binder_user_error_wait);
@@ -6279,7 +6279,8 @@ static int __init binder_init(void)
_log_fops);
}
- if (strcmp(binder_devices_param, "") != 0) {
+ if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
+ strcmp(binder_devices_param, "") != 0) {
/*
* Copy the module_parameter string, because we don't want to
* tokenize it in-place.
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 045b3e42d98b..fe8c745dc8e0 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -37,6 +37,8 @@ struct binder_device {
extern const struct file_operations binder_fops;
+extern char *binder_devices_param;
+
#ifdef CONFIG_ANDROID_BINDERFS
extern bool is_binderfs_device(const struct inode *inode);
#else
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e773f45d19d9..886b4e0f482f 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -186,8 +186,7 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
req->major = MAJOR(binderfs_dev);
req->minor = minor;
- ret = copy_to_user(userp, req, sizeof(*req));
- if (ret) {
+ if (userp && copy_to_user(userp, req, sizeof(*req))) {
ret = -EFAULT;
goto err;
}
@@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void
*data, int silent)
int ret;
struct binderfs_info *info;
struct inode *inode = NULL;
+ struct binderfs_device device_info = { 0 };
+ const char *name;
+ size_t len;
sb->s_blocksize = PAGE_SIZE;
sb->s_blocksize_bits = PAGE_SHIFT;
@@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
if (!sb->s_root)
return -ENOMEM;
- return binderfs_binder_ctl_create(sb);
+ ret = binderfs_binder_ctl_create(sb);
+ if (ret)
+ return ret;
+
+ name = binder_devices_param;
+ for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+ strscpy(device_info.name, name, len + 1);
+ ret = binderfs_binder_device_create(inode, NULL, _info);
+ if (ret)
+ return ret;
+ name += len;
+ if (*name == ',')
+ name++;
+
+ }
+
+ return 0;
+
}
static struct dentry *binderfs_mount(struct file_system_type *fs_type,
--
2.22.0.770.g0f2c4a37fd-goog
[PATCH v2 0/2] Add default binderfs devices
Binderfs was created to help provide private binder devices to containers in their own IPC namespace. Currently, every time a new binderfs instance is mounted, its private binder devices need to be created via IOCTL calls. This patch series eliminates the effort for creating the default binder devices for each binderfs instance by creating them automatically. Hridya Valsaraju (2): binder: Add default binder devices through binderfs when configured binder: Validate the default binderfs device names. drivers/android/binder.c | 5 +++-- drivers/android/binder_internal.h | 2 ++ drivers/android/binderfs.c| 37 --- 3 files changed, 39 insertions(+), 5 deletions(-) -- 2.22.0.770.g0f2c4a37fd-goog
[PATCH v2 2/2] binder: Validate the default binderfs device names.
Length of a binderfs device name cannot exceed BINDERFS_MAX_NAME.
This patch adds a check in binderfs_init() to ensure the same
for the default binder devices that will be created in every
binderfs instance.
Co-developed-by: Christian Brauner
Signed-off-by: Christian Brauner
Signed-off-by: Hridya Valsaraju
---
drivers/android/binderfs.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 886b4e0f482f..52c8bd361906 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -572,6 +572,18 @@ static struct file_system_type binder_fs_type = {
int __init init_binderfs(void)
{
int ret;
+ const char *name;
+ size_t len;
+
+ /* Verify that the default binderfs device names are valid. */
+ name = binder_devices_param;
+ for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+ if (len > BINDERFS_MAX_NAME)
+ return -E2BIG;
+ name += len;
+ if (*name == ',')
+ name++;
+ }
/* Allocate new major number for binderfs. */
ret = alloc_chrdev_region(_dev, 0, BINDERFS_MAX_MINOR,
--
2.22.0.770.g0f2c4a37fd-goog
Re: [PATCH] Add default binder devices through binderfs when configured
On Fri, Aug 2, 2019 at 8:06 AM Christian Brauner wrote:
>
> On Fri, Aug 02, 2019 at 08:18:38AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 01, 2019 at 03:35:56PM -0700, Hridya Valsaraju wrote:
> > > If CONFIG_ANDROID_BINDERFS is set, the default binder devices
> > > specified by CONFIG_ANDROID_BINDER_DEVICES are created in each
> > > binderfs instance instead of global devices being created by
> > > the binder driver.
> > >
> > > Co-developed-by: Christian Brauner
> > > Signed-off-by: Christian Brauner
> > > Signed-off-by: Hridya Valsaraju
> > > ---
> > > drivers/android/binder.c | 3 ++-
> > > drivers/android/binderfs.c | 46 ++
> > > 2 files changed, 44 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index 466b6a7f8ab7..65a99ac26711 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -6279,7 +6279,8 @@ static int __init binder_init(void)
> > > _log_fops);
> > > }
> > >
> > > - if (strcmp(binder_devices_param, "") != 0) {
> > > + if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
> > > + strcmp(binder_devices_param, "") != 0) {
> > > /*
> > > * Copy the module_parameter string, because we don't want to
> > > * tokenize it in-place.
> > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > > index e773f45d19d9..9f5ed50ffd70 100644
> > > --- a/drivers/android/binderfs.c
> > > +++ b/drivers/android/binderfs.c
> > > @@ -48,6 +48,10 @@ static dev_t binderfs_dev;
> > > static DEFINE_MUTEX(binderfs_minors_mutex);
> > > static DEFINE_IDA(binderfs_minors);
> > >
> > > +static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
> > > +module_param_named(devices, binder_devices_param, charp, 0444);
> > > +MODULE_PARM_DESC(devices, "Binder devices to be created by default");
> > > +
> >
> > Why are you creating a module parameter? That was not in your changelog
> > :(
>
> Yeah, you don't need an additional module parameter. You can just move
>
> static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
> module_param_named(devices, binder_devices_param, charp, 0444);
>
> from binder.c to binder_internal.h and expose it to binder.c and
> binderfs.c this way. This will work just fine since binderfs.c doesn't
> modify the parameter and binder.c makes a copy of it before doing so.
>
> >
> >
> >
> > > /**
> > > * binderfs_mount_opts - mount options for binderfs
> > > * @max: maximum number of allocatable binderfs binder devices
> > > @@ -135,7 +139,6 @@ static int binderfs_binder_device_create(struct inode
> > > *ref_inode,
> > > #else
> > > bool use_reserve = true;
> > > #endif
> > > -
> > > /* Reserve new minor number for the new device. */
> > > mutex_lock(_minors_mutex);
> > > if (++info->device_count <= info->mount_opts.max)
> > > @@ -186,8 +189,7 @@ static int binderfs_binder_device_create(struct inode
> > > *ref_inode,
> > > req->major = MAJOR(binderfs_dev);
> > > req->minor = minor;
> > >
> > > - ret = copy_to_user(userp, req, sizeof(*req));
> > > - if (ret) {
> > > + if (userp && copy_to_user(userp, req, sizeof(*req))) {
> > > ret = -EFAULT;
> > > goto err;
> > > }
> > > @@ -467,6 +469,9 @@ static int binderfs_fill_super(struct super_block
> > > *sb, void *data, int silent)
> > > int ret;
> > > struct binderfs_info *info;
> > > struct inode *inode = NULL;
> > > + struct binderfs_device device_info = { 0 };
> > > + const char *name;
> > > + size_t len;
> > >
> > > sb->s_blocksize = PAGE_SIZE;
> > > sb->s_blocksize_bits = PAGE_SHIFT;
> > > @@ -521,7 +526,28 @@ static int binderfs_fill_super(struct super_block
> > > *sb, void *data, int silent)
> > > if (!sb->s_root)
> > > return -ENOMEM;
> > >
> > > - return binderfs_binder_ctl_create(sb);
> > > + ret = binderfs_binder_ctl_create(sb);
> > > + if (ret)
> > > + return ret;
> > > +
&
[PATCH] Add default binder devices through binderfs when configured
If CONFIG_ANDROID_BINDERFS is set, the default binder devices
specified by CONFIG_ANDROID_BINDER_DEVICES are created in each
binderfs instance instead of global devices being created by
the binder driver.
Co-developed-by: Christian Brauner
Signed-off-by: Christian Brauner
Signed-off-by: Hridya Valsaraju
---
drivers/android/binder.c | 3 ++-
drivers/android/binderfs.c | 46 ++
2 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 466b6a7f8ab7..65a99ac26711 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6279,7 +6279,8 @@ static int __init binder_init(void)
_log_fops);
}
- if (strcmp(binder_devices_param, "") != 0) {
+ if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
+ strcmp(binder_devices_param, "") != 0) {
/*
* Copy the module_parameter string, because we don't want to
* tokenize it in-place.
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e773f45d19d9..9f5ed50ffd70 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -48,6 +48,10 @@ static dev_t binderfs_dev;
static DEFINE_MUTEX(binderfs_minors_mutex);
static DEFINE_IDA(binderfs_minors);
+static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
+module_param_named(devices, binder_devices_param, charp, 0444);
+MODULE_PARM_DESC(devices, "Binder devices to be created by default");
+
/**
* binderfs_mount_opts - mount options for binderfs
* @max: maximum number of allocatable binderfs binder devices
@@ -135,7 +139,6 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
#else
bool use_reserve = true;
#endif
-
/* Reserve new minor number for the new device. */
mutex_lock(_minors_mutex);
if (++info->device_count <= info->mount_opts.max)
@@ -186,8 +189,7 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
req->major = MAJOR(binderfs_dev);
req->minor = minor;
- ret = copy_to_user(userp, req, sizeof(*req));
- if (ret) {
+ if (userp && copy_to_user(userp, req, sizeof(*req))) {
ret = -EFAULT;
goto err;
}
@@ -467,6 +469,9 @@ static int binderfs_fill_super(struct super_block *sb, void
*data, int silent)
int ret;
struct binderfs_info *info;
struct inode *inode = NULL;
+ struct binderfs_device device_info = { 0 };
+ const char *name;
+ size_t len;
sb->s_blocksize = PAGE_SIZE;
sb->s_blocksize_bits = PAGE_SHIFT;
@@ -521,7 +526,28 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
if (!sb->s_root)
return -ENOMEM;
- return binderfs_binder_ctl_create(sb);
+ ret = binderfs_binder_ctl_create(sb);
+ if (ret)
+ return ret;
+
+ name = binder_devices_param;
+ for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+ /*
+* init_binderfs() has already checked that the length of
+* device_name_entry->name is not greater than device_info.name.
+*/
+ strscpy(device_info.name, name, len + 1);
+ ret = binderfs_binder_device_create(inode, NULL, _info);
+ if (ret)
+ return ret;
+ name += len;
+ if (*name == ',')
+ name++;
+
+ }
+
+ return 0;
+
}
static struct dentry *binderfs_mount(struct file_system_type *fs_type,
@@ -553,6 +579,18 @@ static struct file_system_type binder_fs_type = {
int __init init_binderfs(void)
{
int ret;
+ const char *name;
+ size_t len;
+
+ /* Verify that the default binderfs device names are valid. */
+ name = binder_devices_param;
+ for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+ if (len > BINDERFS_MAX_NAME)
+ return -E2BIG;
+ name += len;
+ if (*name == ',')
+ name++;
+ }
/* Allocate new major number for binderfs. */
ret = alloc_chrdev_region(_dev, 0, BINDERFS_MAX_MINOR,
--
2.22.0.770.g0f2c4a37fd-goog
[PATCH] binder: prevent transactions to context manager from its own process.
Currently, a transaction to context manager from its own process
is prevented by checking if its binder_proc struct is the same as
that of the sender. However, this would not catch cases where the
process opens the binder device again and uses the new fd to send
a transaction to the context manager.
Reported-by: syzbot+8b3c354d33c4ac78b...@syzkaller.appspotmail.com
Signed-off-by: Hridya Valsaraju
---
drivers/android/binder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index e4d25ebec5be..89b9cedae088 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3138,7 +3138,7 @@ static void binder_transaction(struct binder_proc *proc,
else
return_error = BR_DEAD_REPLY;
mutex_unlock(>context_mgr_node_lock);
- if (target_node && target_proc == proc) {
+ if (target_node && target_proc->pid == proc->pid) {
binder_user_error("%d:%d got transaction to
context manager from process owning it\n",
proc->pid, thread->pid);
return_error = BR_FAILED_REPLY;
--
2.22.0.510.g264f2c817a-goog
