linux-kernel - Re: [Linaro-mm-sig] 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] [day] [month] [year] [list]

Date:   Tue, 10 May 2022 14:52:54 +0200
From:   Christian König <ckoenig.leichtzumerken@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Christian König <christian.koenig@....com>
Cc:     Charan Teja Kalla <quic_charante@...cinc.com>,
        sumit.semwal@...aro.org, 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: [Linaro-mm-sig] Re: [PATCH] dmabuf: ensure unique directory name
 for dmabuf stats

Am 10.05.22 um 14:10 schrieb Greg KH:
> On Tue, May 10, 2022 at 01:35:41PM +0200, Christian König wrote:
>> 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.
> I'll note that the original patch series for this described why this was
> moved from debugfs to 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.
> Why?  Device names change all the time and should never be static.  A
> buffer name should just be a unique identifier in that directory, that's
> all.  No rules on the formatting of it unless for some reason the name
> being the inode number was somehow being used in userspace for that
> number?

My impression was that we documented that should have been a number, but 
I might be wrong on this. And if it's not documented to be a number, I 
think it should be.

The background is that you probably need to associate the DMA-buf with 
some userspace structure for accounting and that becomes easier when you 
can just put them into a radix.

Regards,
Christian.

>
> thanks,
>
> greg k-h
> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@...ts.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@...ts.linaro.org