linux-kernel - Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 10 May 2022 10:11:08 -0700
From:   "T.J. Mercier" <tjmercier@...gle.com>
To:     Charan Teja Kalla <quic_charante@...cinc.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Christian König <christian.koenig@....com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Hridya Valsaraju <hridya@...gle.com>, daniel.vetter@...ll.ch,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH V2] dmabuf: ensure unique directory name for dmabuf stats

On Tue, May 10, 2022 at 7:07 AM Charan Teja Kalla
<quic_charante@...cinc.com> wrote:
>
> The dmabuf file uses get_next_ino()(through dma_buf_getfile() ->
> alloc_anon_inode()) to get an inode number and uses the same as a
> directory name under /sys/kernel/dmabuf/buffers/<ino>. This directory is
> used to collect the dmabuf stats and it is created through
> dma_buf_stats_setup(). At current, failure to create this directory
> entry can make the dma_buf_export() to fail.
>
> Now, as the get_next_ino() can definitely give a repetitive inode no
> causing the directory entry creation to fail with -EEXIST. This is a
> problem on the systems where dmabuf stats functionality is enabled on
> the production builds can make the dma_buf_export(), though the dmabuf
> memory is allocated successfully, to fail just because it couldn't
> create stats entry.
>
> This issue we are able to see on the snapdragon system within 13 days
> where there already exists a directory with inode no "122602" so
> dma_buf_stats_setup() failed with -EEXIST as it is trying to create
> the same directory entry.
>
> To make the directory entry as unique, append the unique_id for every
> inode. With this change the stats directory entries will be in the
> format of: /sys/kernel/dmabuf/buffers/<inode_number-unique_id>.
>
> Signed-off-by: Charan Teja Kalla <quic_charante@...cinc.com>
> ---
> Changes in V2:
>   -- Used the atomic64_t variable to generate a unique_id to be appended to inode
>      to have an unique directory with name <inode_number-unique_id> -- Suggested by christian
>   -- Updated the ABI documentation -- Identified by Greg.
>   -- Massaged the commit log.
>
> Changes in V1:
>   -- Used the inode->i_ctime->tv_secs as an id appended to inode to create the
>      unique directory with name <inode_number-time_in_secs>.
>   -- https://lore.kernel.org/all/1652178212-22383-1-git-send-email-quic_charante@quicinc.com/
>
>  Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers | 10 +++++-----
>  drivers/dma-buf/Kconfig                               |  6 +++---
>  drivers/dma-buf/dma-buf-sysfs-stats.c                 |  8 +++++---
>  3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers
> index 5d3bc99..9fffbd3 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers
> +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers
> @@ -4,19 +4,19 @@ KernelVersion:        v5.13
>  Contact:       Hridya Valsaraju <hridya@...gle.com>
>  Description:   The /sys/kernel/dmabuf/buffers directory contains a
>                 snapshot of the internal state of every DMA-BUF.
> -               /sys/kernel/dmabuf/buffers/<inode_number> will contain the
> -               statistics for the DMA-BUF with the unique inode number
> -               <inode_number>
> +               /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will
> +               contain the statistics for the DMA-BUF with the unique
> +               pair <inode_number-unique_id>

Android userspace does have a dependency on this being an inode
number. Or at least, a single unsigned int. Not the end of the world,
but still... this will break.
https://cs.android.com/android/platform/superproject/+/master:system/memory/libmeminfo/libdmabufinfo/dmabuf_sysfs_stats.cpp;l=76-77;drc=6951984bbefb96423970b82005ae381065e36704

>  Users:         kernel memory tuning/debugging tools
>
> -What:          /sys/kernel/dmabuf/buffers/<inode_number>/exporter_name
> +What:          /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name
>  Date:          May 2021
>  KernelVersion: v5.13
>  Contact:       Hridya Valsaraju <hridya@...gle.com>
>  Description:   This file is read-only and contains the name of the exporter of
>                 the DMA-BUF.
>
> -What:          /sys/kernel/dmabuf/buffers/<inode_number>/size
> +What:          /sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size
>  Date:          May 2021
>  KernelVersion: v5.13
>  Contact:       Hridya Valsaraju <hridya@...gle.com>
> diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
> index 541efe0..5bcbdb1 100644
> --- a/drivers/dma-buf/Kconfig
> +++ b/drivers/dma-buf/Kconfig
> @@ -81,9 +81,9 @@ menuconfig DMABUF_SYSFS_STATS
>            Choose this option to enable DMA-BUF sysfs statistics
>            in location /sys/kernel/dmabuf/buffers.
>
> -          /sys/kernel/dmabuf/buffers/<inode_number> will contain
> -          statistics for the DMA-BUF with the unique inode number
> -          <inode_number>.
> +          /sys/kernel/dmabuf/buffers/<inode_number-unique_id> will contain
> +          statistics for the DMA-BUF with the unique pair
> +          <inode_number-unique_id>.
>
>  source "drivers/dma-buf/heaps/Kconfig"
>
> diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c
> index 2bba0ba..29e9e23 100644
> --- a/drivers/dma-buf/dma-buf-sysfs-stats.c
> +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
> @@ -38,8 +38,8 @@
>   *
>   * The following stats are exposed by the interface:
>   *
> - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/exporter_name``
> - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/size``
> + * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/exporter_name``
> + * * ``/sys/kernel/dmabuf/buffers/<inode_number-unique_id>/size``
>   *
>   * The information in the interface can also be used to derive per-exporter
>   * statistics. The data from the interface can be gathered on error conditions
> @@ -172,6 +172,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf)
>  {
>         struct dma_buf_sysfs_entry *sysfs_entry;
>         int ret;
> +       static atomic64_t unique_id = ATOMIC_INIT(0);
>
>         if (!dmabuf || !dmabuf->file)
>                 return -EINVAL;
> @@ -192,7 +193,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf)
>
>         /* create the directory for buffer stats */
>         ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL,
> -                                  "%lu", file_inode(dmabuf->file)->i_ino);
> +                                  "%lu-%lu", file_inode(dmabuf->file)->i_ino,
> +                                  atomic64_add_return(1, &unique_id));
>         if (ret)
>                 goto err_sysfs_dmabuf;
>
> --
> 2.7.4
>