diff mbox series

[V3] dma-buf: ensure unique directory name for dmabuf stats

Message ID 1652434689-6203-1-git-send-email-quic_charante@quicinc.com (mailing list archive)
State Not Applicable
Headers
Series [V3] dma-buf: ensure unique directory name for dmabuf stats |

Commit Message

Charan Teja Kalla May 13, 2022, 9:38 a.m. UTC 
  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 dentry name as unique, use the dmabuf fs specific inode
which is based on the simple atomic variable increment. There is tmpfs
subsystem too which relies on its own inode generation rather than
relying on the get_next_ino() for the same reason of avoiding the
duplicate inodes[1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/patch/?id=e809d5f0b5c912fe981dce738f3283b2010665f0

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
---
Changes in V3:
  -- Used the atomic64 variable to have dmabuf files its own inodes.
  -- Ensured no UAPI breakage as suggested by Christian.

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.
  -- https://lore.kernel.org/all/1652191562-18700-1-git-send-email-quic_charante@quicinc.com/

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/

 drivers/dma-buf/dma-buf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Greg KH May 13, 2022, 10:11 a.m. UTC | #1 
On Fri, May 13, 2022 at 03:08:09PM +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.
> 
> 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 dentry name as unique, use the dmabuf fs specific inode
> which is based on the simple atomic variable increment. There is tmpfs
> subsystem too which relies on its own inode generation rather than
> relying on the get_next_ino() for the same reason of avoiding the
> duplicate inodes[1].
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/patch/?id=e809d5f0b5c912fe981dce738f3283b2010665f0
> 
> Reported-by: kernel test robot <lkp@intel.com>

The trest robot did not say that the dmabuf stat name was being
duplicated, did it?

> Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
> ---
> Changes in V3:
>   -- Used the atomic64 variable to have dmabuf files its own inodes.
>   -- Ensured no UAPI breakage as suggested by Christian.
> 
> 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.
>   -- https://lore.kernel.org/all/1652191562-18700-1-git-send-email-quic_charante@quicinc.com/
> 
> 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/
> 
>  drivers/dma-buf/dma-buf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index a6fc96e..0ad5039 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -407,6 +407,7 @@ static inline int is_dma_buf_file(struct file *file)
>  
>  static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
>  {
> +	static atomic64_t dmabuf_inode = ATOMIC64_INIT(0);
>  	struct file *file;
>  	struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
>  
> @@ -416,6 +417,13 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
>  	inode->i_size = dmabuf->size;
>  	inode_set_bytes(inode, dmabuf->size);
>  
> +	/*
> +	 * The ->i_ino acquired from get_next_ino() is not unique thus
> +	 * not suitable for using it as dentry name by dmabuf stats.
> +	 * Override ->i_ino with the unique and dmabuffs specific
> +	 * value.
> +	 */
> +	inode->i_ino = atomic64_add_return(1, &dmabuf_inode);
>  	file = alloc_file_pseudo(inode, dma_buf_mnt, "dmabuf",
>  				 flags, &dma_buf_fops);
>  	if (IS_ERR(file))
> -- 
> 2.7.4
> 

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Charan Teja Kalla May 13, 2022, 10:18 a.m. UTC | #2 
On 5/13/2022 3:41 PM, Greg KH wrote:
>> Reported-by: kernel test robot <lkp@intel.com>
> The trest robot did not say that the dmabuf stat name was being
> duplicated, did it?
>

It reported a printk warning on V2[1]. Should we remove this on V3?

@Christian: Could you please drop this tag while merging?

[1] https://lore.kernel.org/all/202205110511.E0d8TXXC-lkp@intel.com/


>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index a6fc96e..0ad5039 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -407,6 +407,7 @@ static inline int is_dma_buf_file(struct file *file)
>>  
>>  static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
>>  {
>> +	static atomic64_t dmabuf_inode = ATOMIC64_INIT(0);
>>  	struct file *file;
>>  	struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
>>  
>> @@ -416,6 +417,13 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
>>  	inode->i_size = dmabuf->size;
>>  	inode_set_bytes(inode, dmabuf->size);
>>  
>> +	/*
>> +	 * The ->i_ino acquired from get_next_ino() is not unique thus
>> +	 * not suitable for using it as dentry name by dmabuf stats.
>> +	 * Override ->i_ino with the unique and dmabuffs specific
>> +	 * value.
>> +	 */
>> +	inode->i_ino = atomic64_add_return(1, &dmabuf_inode);
>>  	file = alloc_file_pseudo(inode, dma_buf_mnt, "dmabuf",
>>  				 flags, &dma_buf_fops);
>>  	if (IS_ERR(file))
>> -- 
>> 2.7.4
>>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for the ACK.

--Charan
Greg KH May 13, 2022, 10:26 a.m. UTC | #3 
On Fri, May 13, 2022 at 03:48:23PM +0530, Charan Teja Kalla wrote:
> 
> On 5/13/2022 3:41 PM, Greg KH wrote:
> >> Reported-by: kernel test robot <lkp@intel.com>
> > The trest robot did not say that the dmabuf stat name was being
> > duplicated, did it?
> >
> 
> It reported a printk warning on V2[1]. Should we remove this on V3?

Yes, that's different.

> @Christian: Could you please drop this tag while merging?
> 
> [1] https://lore.kernel.org/all/202205110511.E0d8TXXC-lkp@intel.com/

Never ask a maintainer to hand-edit a patch, it increases their workload
:(

thanks,

greg k-h
Christian König May 13, 2022, 10:29 a.m. UTC | #4 
Am 13.05.22 um 12:18 schrieb Charan Teja Kalla:
> On 5/13/2022 3:41 PM, Greg KH wrote:
>>> Reported-by: kernel test robot <lkp@intel.com>
>> The trest robot did not say that the dmabuf stat name was being
>> duplicated, did it?
>>
> It reported a printk warning on V2[1]. Should we remove this on V3?

We only add the kernel test robot is report when it found the underlying 
problem and not just noted some warning on an intermediate patch version.

> @Christian: Could you please drop this tag while merging?

Sure, I don't have much on my plate at the moment. But don't let it 
become a habit.

Going to push it upstream through drm-misc-fixes now.

Regards,
Christian.

>
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F202205110511.E0d8TXXC-lkp%40intel.com%2F&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C00e38dfb74fb4fe48b8408da34c9ee77%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637880339170888363%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=yH%2FBg2Fjq2ohEFKLgw2CcYEeHiUfgPLlsJaRnt9cKLo%3D&amp;reserved=0
>
>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index a6fc96e..0ad5039 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -407,6 +407,7 @@ static inline int is_dma_buf_file(struct file *file)
>>>   
>>>   static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
>>>   {
>>> +	static atomic64_t dmabuf_inode = ATOMIC64_INIT(0);
>>>   	struct file *file;
>>>   	struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
>>>   
>>> @@ -416,6 +417,13 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
>>>   	inode->i_size = dmabuf->size;
>>>   	inode_set_bytes(inode, dmabuf->size);
>>>   
>>> +	/*
>>> +	 * The ->i_ino acquired from get_next_ino() is not unique thus
>>> +	 * not suitable for using it as dentry name by dmabuf stats.
>>> +	 * Override ->i_ino with the unique and dmabuffs specific
>>> +	 * value.
>>> +	 */
>>> +	inode->i_ino = atomic64_add_return(1, &dmabuf_inode);
>>>   	file = alloc_file_pseudo(inode, dma_buf_mnt, "dmabuf",
>>>   				 flags, &dma_buf_fops);
>>>   	if (IS_ERR(file))
>>> -- 
>>> 2.7.4
>>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Thanks for the ACK.
>
> --Charan
Charan Teja Kalla May 13, 2022, 10:38 a.m. UTC | #5 
On 5/13/2022 3:59 PM, Christian König wrote:
> Am 13.05.22 um 12:18 schrieb Charan Teja Kalla:
>> On 5/13/2022 3:41 PM, Greg KH wrote:
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>> The trest robot did not say that the dmabuf stat name was being
>>> duplicated, did it?
>>>
>> It reported a printk warning on V2[1]. Should we remove this on V3?
> 
> We only add the kernel test robot is report when it found the underlying
> problem and not just noted some warning on an intermediate patch version.

Noted. Thanks!!
> 
>> @Christian: Could you please drop this tag while merging?
> 
> Sure, I don't have much on my plate at the moment. But don't let it
> become a habit.
> 

Sure. I am also thinking If it is worth to add stable tag? Though it is
not crashing the kernel but definitely making the dma_buf_export to fail
for no reason.

If yes, I can resend the patch with all these tags.

> Going to push it upstream through drm-misc-fixes now.
Christian König May 13, 2022, 10:42 a.m. UTC | #6 
Am 13.05.22 um 12:38 schrieb Charan Teja Kalla:
> On 5/13/2022 3:59 PM, Christian König wrote:
>> Am 13.05.22 um 12:18 schrieb Charan Teja Kalla:
>>> On 5/13/2022 3:41 PM, Greg KH wrote:
>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> The trest robot did not say that the dmabuf stat name was being
>>>> duplicated, did it?
>>>>
>>> It reported a printk warning on V2[1]. Should we remove this on V3?
>> We only add the kernel test robot is report when it found the underlying
>> problem and not just noted some warning on an intermediate patch version.
> Noted. Thanks!!
>>> @Christian: Could you please drop this tag while merging?
>> Sure, I don't have much on my plate at the moment. But don't let it
>> become a habit.
>>
> Sure. I am also thinking If it is worth to add stable tag? Though it is
> not crashing the kernel but definitely making the dma_buf_export to fail
> for no reason.
>
> If yes, I can resend the patch with all these tags.

Yeah, sure.

Christian.

>
>> Going to push it upstream through drm-misc-fixes now.
diff mbox series

Patch

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index a6fc96e..0ad5039 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -407,6 +407,7 @@  static inline int is_dma_buf_file(struct file *file)
 
 static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
 {
+	static atomic64_t dmabuf_inode = ATOMIC64_INIT(0);
 	struct file *file;
 	struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
 
@@ -416,6 +417,13 @@  static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
 	inode->i_size = dmabuf->size;
 	inode_set_bytes(inode, dmabuf->size);
 
+	/*
+	 * The ->i_ino acquired from get_next_ino() is not unique thus
+	 * not suitable for using it as dentry name by dmabuf stats.
+	 * Override ->i_ino with the unique and dmabuffs specific
+	 * value.
+	 */
+	inode->i_ino = atomic64_add_return(1, &dmabuf_inode);
 	file = alloc_file_pseudo(inode, dma_buf_mnt, "dmabuf",
 				 flags, &dma_buf_fops);
 	if (IS_ERR(file))