| 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
| ||
|
Date: Tue, 10 May 2022 17:12:07 +0200
From: Christian König <christian.koenig@....com>
To: Charan Teja Kalla <quic_charante@...cinc.com>,
gregkh@...uxfoundation.org, sumit.semwal@...aro.org,
hridya@...gle.com, daniel.vetter@...ll.ch, tjmercier@...gle.com
Cc: 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
Am 10.05.22 um 16:06 schrieb Charan Teja Kalla:
> 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>
> 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);
Please move that to the beginning of the declarations.
>
> 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,
Why not just use the unique value here? Or is the inode number necessary
for something?
Regards,
Christian.
> + atomic64_add_return(1, &unique_id));
> if (ret)
> goto err_sysfs_dmabuf;
>
Powered by blists - more mailing lists