linux-kernel - Re: [PATCH] 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: <039e1acc-8688-2e06-1b2a-1acbe813b91e@amd.com>
Date:   Tue, 10 May 2022 13:35:41 +0200
From:   Christian König <christian.koenig@....com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Charan Teja Kalla <quic_charante@...cinc.com>
Cc:     sumit.semwal@...aro.org, daniel.vetter@...ll.ch,
        tjmercier@...gle.com, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dmabuf: ensure unique directory name for dmabuf stats

Am 10.05.22 um 13:00 schrieb Greg KH:
> On Tue, May 10, 2022 at 03:53:32PM +0530, Charan Teja Kalla 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.
> Then maybe we should not fail the creation path of the kobject fails to
> be created?  It's just for debugging, it should be fine if the creation
> of it isn't there.

Well if it's just for debugging then it should be under debugfs and not 
sysfs.

>> 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 inode creation time to
>> the inode. With this change the stats directory entries will be in the
>> format of: /sys/kernel/dmabuf/buffers/<inode no>-<inode creation time in
>> secs>.
> As you are changing the format here, shouldn't the Documentation/ABI/
> entry for this also be changed?

As far as I can see that is even an UAPI break, not sure if we can allow 
that.

> And what's to keep the seconds field from also being the same?

Well exporting two DMA-bufs with the same ino in the same nanosecond 
should be basically impossible, but I would rather opt for using a 64bit 
atomic in that function.

This should be 100% UAPI compatible and even if we manage to create one 
buffer ever ns we need ~500years to wrap around.

Regards,
Christian.

>
> thanks,
>
> greg k-h