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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4ac55be2-7d55-2c3b-0d5e-f61c02c62792@amd.com>
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;
>