[PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Nirmoy Das]

Debugfs core APIs will throw -EPERM when user disables debugfs
using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
see that as an error. Also validate drm root dentry before creating
amdgpu debugfs files.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 6611b3c7c149..d786072e918b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 	struct dentry *ent;
 	int r, i;
 
+	if (IS_ERR(root)) {
+		/* When debugfs is disabled we get -EPERM which is not an
+		 * error as this is user controllable.
+		 */
+		if (PTR_ERR(root) == -EPERM)
+			return 0;
+
+		return PTR_ERR(ent);
+	}
+
 	ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
 				  &fops_ib_preempt);
 	if (IS_ERR(ent)) {
-- 
2.32.0

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Christian König]

Am 05.10.21 um 15:11 schrieb Nirmoy Das:
> Debugfs core APIs will throw -EPERM when user disables debugfs
> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
> see that as an error. Also validate drm root dentry before creating
> amdgpu debugfs files.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index 6611b3c7c149..d786072e918b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   	struct dentry *ent;
>   	int r, i;
>   
> +	if (IS_ERR(root)) {
> +		/* When debugfs is disabled we get -EPERM which is not an
> +		 * error as this is user controllable.
> +		 */

Well setting primary->debugfs_root to an error code is probably not a 
good idea to begin with.

When debugfs is disabled that should most likely be NULL.

Regards,
Christian.

> +		if (PTR_ERR(root) == -EPERM)
> +			return 0;
> +
> +		return PTR_ERR(ent);
> +	}
> +
>   	ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>   				  &fops_ib_preempt);
>   	if (IS_ERR(ent)) {

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Das, Nirmoy]

On 10/5/2021 3:22 PM, Christian König wrote:
>
>
> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>> Debugfs core APIs will throw -EPERM when user disables debugfs
>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>> see that as an error. Also validate drm root dentry before creating
>> amdgpu debugfs files.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> index 6611b3c7c149..d786072e918b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>> *adev)
>>       struct dentry *ent;
>>       int r, i;
>>   +    if (IS_ERR(root)) {
>> +        /* When debugfs is disabled we get -EPERM which is not an
>> +         * error as this is user controllable.
>> +         */
>
> Well setting primary->debugfs_root to an error code is probably not a 
> good idea to begin with.
>
> When debugfs is disabled that should most likely be NULL.


If we set primary->debugfs_root to  NULL then we need to add bunch of 
NULL checks everywhere before creating any debugfs files

because debugfs_create_{file|dir}() with NULL root is still valid.  I am 
assuming a hypothetical case when debugfs_root dir creation fails  even 
with debugfs enabled

but further calls are successful.  This wont be a problem if we 
propagate the error code.


Regards,

Nirmoy

>
> Regards,
> Christian.
>
>> +        if (PTR_ERR(root) == -EPERM)
>> +            return 0;
>> +
>> +        return PTR_ERR(ent);
>> +    }
>> +
>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>>                     &fops_ib_preempt);
>>       if (IS_ERR(ent)) {
>

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Lazar, Lijo]

On 10/5/2021 7:19 PM, Das, Nirmoy wrote:
> 
> On 10/5/2021 3:22 PM, Christian König wrote:
>>
>>
>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>>> see that as an error. Also validate drm root dentry before creating
>>> amdgpu debugfs files.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index 6611b3c7c149..d786072e918b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>> *adev)
>>>       struct dentry *ent;
>>>       int r, i;
>>>   +    if (IS_ERR(root)) {
>>> +        /* When debugfs is disabled we get -EPERM which is not an
>>> +         * error as this is user controllable.
>>> +         */
>>
>> Well setting primary->debugfs_root to an error code is probably not a 
>> good idea to begin with.
>>
>> When debugfs is disabled that should most likely be NULL.
> 
> 
> If we set primary->debugfs_root to  NULL then we need to add bunch of 
> NULL checks everywhere before creating any debugfs files
> 
> because debugfs_create_{file|dir}() with NULL root is still valid.  I am 
> assuming a hypothetical case when debugfs_root dir creation fails  even 
> with debugfs enabled
> 
> but further calls are successful.  This wont be a problem if we 
> propagate the error code.
> 
> 

drm does a blind assignment.

	minor->debugfs_root = debugfs_create_dir(name, root);

As per the latest documentation, the API doesn't return NULL
	If an error occurs, ERR_PTR(-ERROR) will be returned.

In short, root won't be NULL once the debugfs API is called.

Thanks,
Lijo

> Regards,
> 
> Nirmoy
> 
>>
>> Regards,
>> Christian.
>>
>>> +        if (PTR_ERR(root) == -EPERM)
>>> +            return 0;
>>> +
>>> +        return PTR_ERR(ent);
>>> +    }
>>> +
>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>>>                     &fops_ib_preempt);
>>>       if (IS_ERR(ent)) {
>>

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Christian König]

Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>
> On 10/5/2021 3:22 PM, Christian König wrote:
>>
>>
>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>>> see that as an error. Also validate drm root dentry before creating
>>> amdgpu debugfs files.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index 6611b3c7c149..d786072e918b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>> *adev)
>>>       struct dentry *ent;
>>>       int r, i;
>>>   +    if (IS_ERR(root)) {
>>> +        /* When debugfs is disabled we get -EPERM which is not an
>>> +         * error as this is user controllable.
>>> +         */
>>
>> Well setting primary->debugfs_root to an error code is probably not a 
>> good idea to begin with.
>>
>> When debugfs is disabled that should most likely be NULL.
>
>
> If we set primary->debugfs_root to  NULL then we need to add bunch of 
> NULL checks everywhere before creating any debugfs files
>
> because debugfs_create_{file|dir}() with NULL root is still valid.  I 
> am assuming a hypothetical case when debugfs_root dir creation fails  
> even with debugfs enabled
>
> but further calls are successful.  This wont be a problem if we 
> propagate the error code.

Yeah, but an error code in members is ugly like hell and potentially 
causes crashes instead.

I strongly suggest to fix this so that root is NULL when debugfs isn't 
available and we add proper checks for that instead.

Regards,
Christian.

>
>
> Regards,
>
> Nirmoy
>
>>
>> Regards,
>> Christian.
>>
>>> +        if (PTR_ERR(root) == -EPERM)
>>> +            return 0;
>>> +
>>> +        return PTR_ERR(ent);
>>> +    }
>>> +
>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>>>                     &fops_ib_preempt);
>>>       if (IS_ERR(ent)) {
>>

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Lazar, Lijo]

On 10/5/2021 10:15 PM, Christian König wrote:
> Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>>
>> On 10/5/2021 3:22 PM, Christian König wrote:
>>>
>>>
>>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>>>> see that as an error. Also validate drm root dentry before creating
>>>> amdgpu debugfs files.
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> index 6611b3c7c149..d786072e918b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>>> *adev)
>>>>       struct dentry *ent;
>>>>       int r, i;
>>>>   +    if (IS_ERR(root)) {
>>>> +        /* When debugfs is disabled we get -EPERM which is not an
>>>> +         * error as this is user controllable.
>>>> +         */
>>>
>>> Well setting primary->debugfs_root to an error code is probably not a 
>>> good idea to begin with.
>>>
>>> When debugfs is disabled that should most likely be NULL.
>>
>>
>> If we set primary->debugfs_root to  NULL then we need to add bunch of 
>> NULL checks everywhere before creating any debugfs files
>>
>> because debugfs_create_{file|dir}() with NULL root is still valid.  I 
>> am assuming a hypothetical case when debugfs_root dir creation fails 
>> even with debugfs enabled
>>
>> but further calls are successful.  This wont be a problem if we 
>> propagate the error code.
> 
> Yeah, but an error code in members is ugly like hell and potentially 
> causes crashes instead.
> 
> I strongly suggest to fix this so that root is NULL when debugfs isn't 
> available and we add proper checks for that instead.

This shouldn't be done. A NULL is a valid parent for debugfs API. An 
invalid parent is always checked like this
	      if (IS_ERR(parent))
                 return parent;

Instead of adding redundant work like NULL checks, let the API do its 
work and don't break the API contract. For ex: usage of sample client, 
you may look at the drm usage; it does the same.

Thanks,
Lijo

> 
> Regards,
> Christian.
> 
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>> +        if (PTR_ERR(root) == -EPERM)
>>>> +            return 0;
>>>> +
>>>> +        return PTR_ERR(ent);
>>>> +    }
>>>> +
>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>>>>                     &fops_ib_preempt);
>>>>       if (IS_ERR(ent)) {
>>>
> 

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Christian König]

Am 06.10.21 um 06:51 schrieb Lazar, Lijo:
>
>
> On 10/5/2021 10:15 PM, Christian König wrote:
>> Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>>>
>>> On 10/5/2021 3:22 PM, Christian König wrote:
>>>>
>>>>
>>>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>>>>> see that as an error. Also validate drm root dentry before creating
>>>>> amdgpu debugfs files.
>>>>>
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>>>   1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> index 6611b3c7c149..d786072e918b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct 
>>>>> amdgpu_device *adev)
>>>>>       struct dentry *ent;
>>>>>       int r, i;
>>>>>   +    if (IS_ERR(root)) {
>>>>> +        /* When debugfs is disabled we get -EPERM which is not an
>>>>> +         * error as this is user controllable.
>>>>> +         */
>>>>
>>>> Well setting primary->debugfs_root to an error code is probably not 
>>>> a good idea to begin with.
>>>>
>>>> When debugfs is disabled that should most likely be NULL.
>>>
>>>
>>> If we set primary->debugfs_root to  NULL then we need to add bunch 
>>> of NULL checks everywhere before creating any debugfs files
>>>
>>> because debugfs_create_{file|dir}() with NULL root is still valid.  
>>> I am assuming a hypothetical case when debugfs_root dir creation 
>>> fails even with debugfs enabled
>>>
>>> but further calls are successful.  This wont be a problem if we 
>>> propagate the error code.
>>
>> Yeah, but an error code in members is ugly like hell and potentially 
>> causes crashes instead.
>>
>> I strongly suggest to fix this so that root is NULL when debugfs 
>> isn't available and we add proper checks for that instead.
>
> This shouldn't be done. A NULL is a valid parent for debugfs API. An 
> invalid parent is always checked like this
>           if (IS_ERR(parent))
>                 return parent;
>
> Instead of adding redundant work like NULL checks, let the API do its 
> work and don't break the API contract. For ex: usage of sample client, 
> you may look at the drm usage; it does the same.

Yeah, but that is horrible API design and should be avoided.

ERR_PTR(), PTR_ERR(), IS_ERR() and similar are supposed to be used as 
alternative to signaling errors as return values from functions and 
should *never* ever be used to signal errors in pointer members.

Regards,
Christian.

>
> Thanks,
> Lijo
>
>>
>> Regards,
>> Christian.
>>
>>>
>>>
>>> Regards,
>>>
>>> Nirmoy
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> +        if (PTR_ERR(root) == -EPERM)
>>>>> +            return 0;
>>>>> +
>>>>> +        return PTR_ERR(ent);
>>>>> +    }
>>>>> +
>>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, 
>>>>> adev,
>>>>>                     &fops_ib_preempt);
>>>>>       if (IS_ERR(ent)) {
>>>>
>>

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Lazar, Lijo]

On 10/6/2021 11:49 AM, Christian König wrote:
> Am 06.10.21 um 06:51 schrieb Lazar, Lijo:
>>
>>
>> On 10/5/2021 10:15 PM, Christian König wrote:
>>> Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>>>>
>>>> On 10/5/2021 3:22 PM, Christian König wrote:
>>>>>
>>>>>
>>>>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>>>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>>>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>>>>>> see that as an error. Also validate drm root dentry before creating
>>>>>> amdgpu debugfs files.
>>>>>>
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>>>>   1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>> index 6611b3c7c149..d786072e918b 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct 
>>>>>> amdgpu_device *adev)
>>>>>>       struct dentry *ent;
>>>>>>       int r, i;
>>>>>>   +    if (IS_ERR(root)) {
>>>>>> +        /* When debugfs is disabled we get -EPERM which is not an
>>>>>> +         * error as this is user controllable.
>>>>>> +         */
>>>>>
>>>>> Well setting primary->debugfs_root to an error code is probably not 
>>>>> a good idea to begin with.
>>>>>
>>>>> When debugfs is disabled that should most likely be NULL.
>>>>
>>>>
>>>> If we set primary->debugfs_root to  NULL then we need to add bunch 
>>>> of NULL checks everywhere before creating any debugfs files
>>>>
>>>> because debugfs_create_{file|dir}() with NULL root is still valid. I 
>>>> am assuming a hypothetical case when debugfs_root dir creation fails 
>>>> even with debugfs enabled
>>>>
>>>> but further calls are successful.  This wont be a problem if we 
>>>> propagate the error code.
>>>
>>> Yeah, but an error code in members is ugly like hell and potentially 
>>> causes crashes instead.
>>>
>>> I strongly suggest to fix this so that root is NULL when debugfs 
>>> isn't available and we add proper checks for that instead.
>>
>> This shouldn't be done. A NULL is a valid parent for debugfs API. An 
>> invalid parent is always checked like this
>>           if (IS_ERR(parent))
>>                 return parent;
>>
>> Instead of adding redundant work like NULL checks, let the API do its 
>> work and don't break the API contract. For ex: usage of sample client, 
>> you may look at the drm usage; it does the same.
> 
> Yeah, but that is horrible API design and should be avoided.
> 
> ERR_PTR(), PTR_ERR(), IS_ERR() and similar are supposed to be used as 
> alternative to signaling errors as return values from functions and 
> should *never* ever be used to signal errors in pointer members.
> 

One escape route may be - add another export from debugfs like 
debugfs_is_valid_node() which adheres to the current logic in debugfs 
API and use that in client code. Whenever debugfs changes to a different 
logic from IS_ERR, let that be changed.

Thanks,
Lijo

> Regards,
> Christian.
> 
>>
>> Thanks,
>> Lijo
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Nirmoy
>>>>
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> +        if (PTR_ERR(root) == -EPERM)
>>>>>> +            return 0;
>>>>>> +
>>>>>> +        return PTR_ERR(ent);
>>>>>> +    }
>>>>>> +
>>>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, 
>>>>>> adev,
>>>>>>                     &fops_ib_preempt);
>>>>>>       if (IS_ERR(ent)) {
>>>>>
>>>
> 

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Christian König]

Am 06.10.21 um 08:32 schrieb Lazar, Lijo:
>
>
> On 10/6/2021 11:49 AM, Christian König wrote:
>> Am 06.10.21 um 06:51 schrieb Lazar, Lijo:
>>>
>>>
>>> On 10/5/2021 10:15 PM, Christian König wrote:
>>>> Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>>>>>
>>>>> On 10/5/2021 3:22 PM, Christian König wrote:
>>>>>>
>>>>>>
>>>>>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>>>>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>>>>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>>>>>>> see that as an error. Also validate drm root dentry before creating
>>>>>>> amdgpu debugfs files.
>>>>>>>
>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>>>>>   1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>> index 6611b3c7c149..d786072e918b 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct 
>>>>>>> amdgpu_device *adev)
>>>>>>>       struct dentry *ent;
>>>>>>>       int r, i;
>>>>>>>   +    if (IS_ERR(root)) {
>>>>>>> +        /* When debugfs is disabled we get -EPERM which is not an
>>>>>>> +         * error as this is user controllable.
>>>>>>> +         */
>>>>>>
>>>>>> Well setting primary->debugfs_root to an error code is probably 
>>>>>> not a good idea to begin with.
>>>>>>
>>>>>> When debugfs is disabled that should most likely be NULL.
>>>>>
>>>>>
>>>>> If we set primary->debugfs_root to  NULL then we need to add bunch 
>>>>> of NULL checks everywhere before creating any debugfs files
>>>>>
>>>>> because debugfs_create_{file|dir}() with NULL root is still valid. 
>>>>> I am assuming a hypothetical case when debugfs_root dir creation 
>>>>> fails even with debugfs enabled
>>>>>
>>>>> but further calls are successful.  This wont be a problem if we 
>>>>> propagate the error code.
>>>>
>>>> Yeah, but an error code in members is ugly like hell and 
>>>> potentially causes crashes instead.
>>>>
>>>> I strongly suggest to fix this so that root is NULL when debugfs 
>>>> isn't available and we add proper checks for that instead.
>>>
>>> This shouldn't be done. A NULL is a valid parent for debugfs API. An 
>>> invalid parent is always checked like this
>>>           if (IS_ERR(parent))
>>>                 return parent;
>>>
>>> Instead of adding redundant work like NULL checks, let the API do 
>>> its work and don't break the API contract. For ex: usage of sample 
>>> client, you may look at the drm usage; it does the same.
>>
>> Yeah, but that is horrible API design and should be avoided.
>>
>> ERR_PTR(), PTR_ERR(), IS_ERR() and similar are supposed to be used as 
>> alternative to signaling errors as return values from functions and 
>> should *never* ever be used to signal errors in pointer members.
>>
>
> One escape route may be - add another export from debugfs like 
> debugfs_is_valid_node() which adheres to the current logic in debugfs 
> API and use that in client code. Whenever debugfs changes to a 
> different logic from IS_ERR, let that be changed.

Well that would then rather be drm_is_debugfs_enabled(), because that we 
separate debugfs handling into a drm core and individual drivers is drm 
specific.

Christian.

>
> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks,
>>> Lijo
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Nirmoy
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> +        if (PTR_ERR(root) == -EPERM)
>>>>>>> +            return 0;
>>>>>>> +
>>>>>>> +        return PTR_ERR(ent);
>>>>>>> +    }
>>>>>>> +
>>>>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, 
>>>>>>> adev,
>>>>>>>                     &fops_ib_preempt);
>>>>>>>       if (IS_ERR(ent)) {
>>>>>>
>>>>
>>

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Lazar, Lijo]

On 10/6/2021 12:05 PM, Christian König wrote:
> Am 06.10.21 um 08:32 schrieb Lazar, Lijo:
>>
>>
>> On 10/6/2021 11:49 AM, Christian König wrote:
>>> Am 06.10.21 um 06:51 schrieb Lazar, Lijo:
>>>>
>>>>
>>>> On 10/5/2021 10:15 PM, Christian König wrote:
>>>>> Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>>>>>>
>>>>>> On 10/5/2021 3:22 PM, Christian König wrote:
>>>>>>>
>>>>>>>
>>>>>>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>>>>>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>>>>>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>>>>>>>> see that as an error. Also validate drm root dentry before creating
>>>>>>>> amdgpu debugfs files.
>>>>>>>>
>>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>>>>>>   1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>> index 6611b3c7c149..d786072e918b 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct 
>>>>>>>> amdgpu_device *adev)
>>>>>>>>       struct dentry *ent;
>>>>>>>>       int r, i;
>>>>>>>>   +    if (IS_ERR(root)) {
>>>>>>>> +        /* When debugfs is disabled we get -EPERM which is not an
>>>>>>>> +         * error as this is user controllable.
>>>>>>>> +         */
>>>>>>>
>>>>>>> Well setting primary->debugfs_root to an error code is probably 
>>>>>>> not a good idea to begin with.
>>>>>>>
>>>>>>> When debugfs is disabled that should most likely be NULL.
>>>>>>
>>>>>>
>>>>>> If we set primary->debugfs_root to  NULL then we need to add bunch 
>>>>>> of NULL checks everywhere before creating any debugfs files
>>>>>>
>>>>>> because debugfs_create_{file|dir}() with NULL root is still valid. 
>>>>>> I am assuming a hypothetical case when debugfs_root dir creation 
>>>>>> fails even with debugfs enabled
>>>>>>
>>>>>> but further calls are successful.  This wont be a problem if we 
>>>>>> propagate the error code.
>>>>>
>>>>> Yeah, but an error code in members is ugly like hell and 
>>>>> potentially causes crashes instead.
>>>>>
>>>>> I strongly suggest to fix this so that root is NULL when debugfs 
>>>>> isn't available and we add proper checks for that instead.
>>>>
>>>> This shouldn't be done. A NULL is a valid parent for debugfs API. An 
>>>> invalid parent is always checked like this
>>>>           if (IS_ERR(parent))
>>>>                 return parent;
>>>>
>>>> Instead of adding redundant work like NULL checks, let the API do 
>>>> its work and don't break the API contract. For ex: usage of sample 
>>>> client, you may look at the drm usage; it does the same.
>>>
>>> Yeah, but that is horrible API design and should be avoided.
>>>
>>> ERR_PTR(), PTR_ERR(), IS_ERR() and similar are supposed to be used as 
>>> alternative to signaling errors as return values from functions and 
>>> should *never* ever be used to signal errors in pointer members.
>>>
>>
>> One escape route may be - add another export from debugfs like 
>> debugfs_is_valid_node() which adheres to the current logic in debugfs 
>> API and use that in client code. Whenever debugfs changes to a 
>> different logic from IS_ERR, let that be changed.
> 
> Well that would then rather be drm_is_debugfs_enabled(), because that we 
> separate debugfs handling into a drm core and individual drivers is drm 
> specific.
> 

Had one more look and looks like this will do the job. In other cases, 
API usage is allowed.

	if (!debugfs_initialized())
		return;

Thanks,
Lijo

> Christian.
> 
>>
>> Thanks,
>> Lijo
>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Thanks,
>>>> Lijo
>>>>
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Nirmoy
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>>> +        if (PTR_ERR(root) == -EPERM)
>>>>>>>> +            return 0;
>>>>>>>> +
>>>>>>>> +        return PTR_ERR(ent);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, 
>>>>>>>> adev,
>>>>>>>>                     &fops_ib_preempt);
>>>>>>>>       if (IS_ERR(ent)) {
>>>>>>>
>>>>>
>>>
> 

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Christian König]

Am 06.10.21 um 08:55 schrieb Lazar, Lijo:
>
>
> On 10/6/2021 12:05 PM, Christian König wrote:
>> Am 06.10.21 um 08:32 schrieb Lazar, Lijo:
>>>
>>>
>>> On 10/6/2021 11:49 AM, Christian König wrote:
>>>> Am 06.10.21 um 06:51 schrieb Lazar, Lijo:
>>>>>
>>>>>
>>>>> On 10/5/2021 10:15 PM, Christian König wrote:
>>>>>> Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>>>>>>>
>>>>>>> On 10/5/2021 3:22 PM, Christian König wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>>>>>>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>>>>>>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We 
>>>>>>>>> shouldn't
>>>>>>>>> see that as an error. Also validate drm root dentry before 
>>>>>>>>> creating
>>>>>>>>> amdgpu debugfs files.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>>>>>>>   1 file changed, 10 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>> index 6611b3c7c149..d786072e918b 100644
>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct 
>>>>>>>>> amdgpu_device *adev)
>>>>>>>>>       struct dentry *ent;
>>>>>>>>>       int r, i;
>>>>>>>>>   +    if (IS_ERR(root)) {
>>>>>>>>> +        /* When debugfs is disabled we get -EPERM which is 
>>>>>>>>> not an
>>>>>>>>> +         * error as this is user controllable.
>>>>>>>>> +         */
>>>>>>>>
>>>>>>>> Well setting primary->debugfs_root to an error code is probably 
>>>>>>>> not a good idea to begin with.
>>>>>>>>
>>>>>>>> When debugfs is disabled that should most likely be NULL.
>>>>>>>
>>>>>>>
>>>>>>> If we set primary->debugfs_root to  NULL then we need to add 
>>>>>>> bunch of NULL checks everywhere before creating any debugfs files
>>>>>>>
>>>>>>> because debugfs_create_{file|dir}() with NULL root is still 
>>>>>>> valid. I am assuming a hypothetical case when debugfs_root dir 
>>>>>>> creation fails even with debugfs enabled
>>>>>>>
>>>>>>> but further calls are successful.  This wont be a problem if we 
>>>>>>> propagate the error code.
>>>>>>
>>>>>> Yeah, but an error code in members is ugly like hell and 
>>>>>> potentially causes crashes instead.
>>>>>>
>>>>>> I strongly suggest to fix this so that root is NULL when debugfs 
>>>>>> isn't available and we add proper checks for that instead.
>>>>>
>>>>> This shouldn't be done. A NULL is a valid parent for debugfs API. 
>>>>> An invalid parent is always checked like this
>>>>>           if (IS_ERR(parent))
>>>>>                 return parent;
>>>>>
>>>>> Instead of adding redundant work like NULL checks, let the API do 
>>>>> its work and don't break the API contract. For ex: usage of sample 
>>>>> client, you may look at the drm usage; it does the same.
>>>>
>>>> Yeah, but that is horrible API design and should be avoided.
>>>>
>>>> ERR_PTR(), PTR_ERR(), IS_ERR() and similar are supposed to be used 
>>>> as alternative to signaling errors as return values from functions 
>>>> and should *never* ever be used to signal errors in pointer members.
>>>>
>>>
>>> One escape route may be - add another export from debugfs like 
>>> debugfs_is_valid_node() which adheres to the current logic in 
>>> debugfs API and use that in client code. Whenever debugfs changes to 
>>> a different logic from IS_ERR, let that be changed.
>>
>> Well that would then rather be drm_is_debugfs_enabled(), because that 
>> we separate debugfs handling into a drm core and individual drivers 
>> is drm specific.
>>
>
> Had one more look and looks like this will do the job. In other cases, 
> API usage is allowed.
>
>     if (!debugfs_initialized())
>         return;

Yeah, that might work as well.

Potentially a good idea to add that to both the core drm function and 
the amdgpu function. and not attempt to create debugfs files in the 
first place.

Christian.

>
> Thanks,
> Lijo
>
>> Christian.
>>
>>>
>>> Thanks,
>>> Lijo
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Thanks,
>>>>> Lijo
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Nirmoy
>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>> +        if (PTR_ERR(root) == -EPERM)
>>>>>>>>> +            return 0;
>>>>>>>>> +
>>>>>>>>> +        return PTR_ERR(ent);
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, 
>>>>>>>>> root, adev,
>>>>>>>>>                     &fops_ib_preempt);
>>>>>>>>>       if (IS_ERR(ent)) {
>>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

[Das, Nirmoy]

On 10/6/2021 8:59 AM, Christian König wrote:
> Am 06.10.21 um 08:55 schrieb Lazar, Lijo:
>>
>>
>> On 10/6/2021 12:05 PM, Christian König wrote:
>>> Am 06.10.21 um 08:32 schrieb Lazar, Lijo:
>>>>
>>>>
>>>> On 10/6/2021 11:49 AM, Christian König wrote:
>>>>> Am 06.10.21 um 06:51 schrieb Lazar, Lijo:
>>>>>>
>>>>>>
>>>>>> On 10/5/2021 10:15 PM, Christian König wrote:
>>>>>>> Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>>>>>>>>
>>>>>>>> On 10/5/2021 3:22 PM, Christian König wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>>>>>>>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>>>>>>>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We 
>>>>>>>>>> shouldn't
>>>>>>>>>> see that as an error. Also validate drm root dentry before 
>>>>>>>>>> creating
>>>>>>>>>> amdgpu debugfs files.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>>>>>> ---
>>>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>>>>>>>>   1 file changed, 10 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>>> index 6611b3c7c149..d786072e918b 100644
>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct 
>>>>>>>>>> amdgpu_device *adev)
>>>>>>>>>>       struct dentry *ent;
>>>>>>>>>>       int r, i;
>>>>>>>>>>   +    if (IS_ERR(root)) {
>>>>>>>>>> +        /* When debugfs is disabled we get -EPERM which is 
>>>>>>>>>> not an
>>>>>>>>>> +         * error as this is user controllable.
>>>>>>>>>> +         */
>>>>>>>>>
>>>>>>>>> Well setting primary->debugfs_root to an error code is 
>>>>>>>>> probably not a good idea to begin with.
>>>>>>>>>
>>>>>>>>> When debugfs is disabled that should most likely be NULL.
>>>>>>>>
>>>>>>>>
>>>>>>>> If we set primary->debugfs_root to  NULL then we need to add 
>>>>>>>> bunch of NULL checks everywhere before creating any debugfs files
>>>>>>>>
>>>>>>>> because debugfs_create_{file|dir}() with NULL root is still 
>>>>>>>> valid. I am assuming a hypothetical case when debugfs_root dir 
>>>>>>>> creation fails even with debugfs enabled
>>>>>>>>
>>>>>>>> but further calls are successful.  This wont be a problem if we 
>>>>>>>> propagate the error code.
>>>>>>>
>>>>>>> Yeah, but an error code in members is ugly like hell and 
>>>>>>> potentially causes crashes instead.
>>>>>>>
>>>>>>> I strongly suggest to fix this so that root is NULL when debugfs 
>>>>>>> isn't available and we add proper checks for that instead.
>>>>>>
>>>>>> This shouldn't be done. A NULL is a valid parent for debugfs API. 
>>>>>> An invalid parent is always checked like this
>>>>>>           if (IS_ERR(parent))
>>>>>>                 return parent;
>>>>>>
>>>>>> Instead of adding redundant work like NULL checks, let the API do 
>>>>>> its work and don't break the API contract. For ex: usage of 
>>>>>> sample client, you may look at the drm usage; it does the same.
>>>>>
>>>>> Yeah, but that is horrible API design and should be avoided.
>>>>>
>>>>> ERR_PTR(), PTR_ERR(), IS_ERR() and similar are supposed to be used 
>>>>> as alternative to signaling errors as return values from functions 
>>>>> and should *never* ever be used to signal errors in pointer members.
>>>>>
>>>>
>>>> One escape route may be - add another export from debugfs like 
>>>> debugfs_is_valid_node() which adheres to the current logic in 
>>>> debugfs API and use that in client code. Whenever debugfs changes 
>>>> to a different logic from IS_ERR, let that be changed.
>>>
>>> Well that would then rather be drm_is_debugfs_enabled(), because 
>>> that we separate debugfs handling into a drm core and individual 
>>> drivers is drm specific.
>>>
>>
>> Had one more look and looks like this will do the job. In other 
>> cases, API usage is allowed.
>>
>>     if (!debugfs_initialized())
>>         return;
>
> Yeah, that might work as well.
>
> Potentially a good idea to add that to both the core drm function and 
> the amdgpu function. and not attempt to create debugfs files in the 
> first place.


Sounds good, I will send patches to add this check.


Thanks,

Nirmoy


>
> Christian.
>
>>
>> Thanks,
>> Lijo
>>
>>> Christian.
>>>
>>>>
>>>> Thanks,
>>>> Lijo
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Lijo
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>>
>>>>>>>> Nirmoy
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>>> +        if (PTR_ERR(root) == -EPERM)
>>>>>>>>>> +            return 0;
>>>>>>>>>> +
>>>>>>>>>> +        return PTR_ERR(ent);
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, 
>>>>>>>>>> root, adev,
>>>>>>>>>>                     &fops_ib_preempt);
>>>>>>>>>>       if (IS_ERR(ent)) {
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>