Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
To:     Greg KH <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Cc:     rafael@kernel.org, linux-kernel@vger.kernel.org
References: <20210902102917.2233-1-nirmoy.das@amd.com>
 <YTCpuo00wM7jGSIc@kroah.com> <5ab7739d-15a4-2e84-0320-fee8d5df1e83@amd.com>
 <YTDBfO/byKzGWV8G@kroah.com> <c918f1a5-7c88-bf47-14b6-b6e892695951@amd.com>
 <YTD8+tLP7KeGRXEc@kroah.com>
From:   "Das, Nirmoy" <nirmoy.das@amd.com>
Message-ID: <70d43cb6-c36a-90d4-b249-3d59706e2027@amd.com>
Date:   Thu, 2 Sep 2021 18:44:39 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
In-Reply-To: <YTD8+tLP7KeGRXEc@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?K2wzeWN1UERXeEVoZ3JJK3NVR05rK2tvZ3lJdERDUHorcEVZSHpHczVkWWRP?=
 =?utf-8?B?VGNMMEx0azA1L2hJMTA4YlhhUkNNejgxWU03emJTN0puYU5MdVVuN3hhT0U4?=
 =?utf-8?B?RGtaWXVwYk5SLzBYMG0rZTJ4VFVlY3Z5U3JXbGZrMUc1UjZpMjFLWlNqbUJG?=
 =?utf-8?B?VEs2a0NsOHNTUlJIWlBjeSt3MmoyWTlwOElaR3pITitIN2dTdmxqWXlxalc1?=
 =?utf-8?B?bVl0WVp0UnZnV2FsRWdXVmE5T3JRNVhTOG5CcDJtZWZBbEpFVFdYYmU4TGsr?=
 =?utf-8?B?ZUlzUUVTRG9jUXUrVjZvdVFzcFpzVEtlZklLQ0VsL2tnWHR1aVRCYWNWZVFU?=
 =?utf-8?B?REIxZ1N5MTRybjY4ZWxEWTBlb1A0RUp5Qlc0MlhIQW9ReU8vTEs5Wlcvc25Z?=
 =?utf-8?B?R2JrdHgwa3VENWlvbDBvYzJKZ0hqVUttT2kvRExkSzdZMFdsbkRoV3BBZnd4?=
 =?utf-8?B?VERLK1NvT2tCK2dHUU82NFV0UDFtZEZ2MFRUSmt2dCtaMjhXRmFsYjV6ZDZj?=
 =?utf-8?B?SVEyRmYwSkU5UTdtUE5rUnVRaEZNc3NqWWpnSzlCbkFFTXJ6K1JxMmhmdTJn?=
 =?utf-8?B?d1piemRFazJuclA0czgvMDBLZ1dSNEZyMmZrRWlxWU1qbW9NekxiUlFrcFFq?=
 =?utf-8?B?b0ozNGkzNExDM2I1L2NaZDVPVmx3ckxoTzBLUEF5Z3g5NnFNSkI3ejZEckNV?=
 =?utf-8?B?eVpScDRrQ1VJcjlVb2RLdVprWWJiamttMGdsVFI1RVBXZ1dLaGkyK0pTUUZn?=
 =?utf-8?B?cjdCQjcrUEZ5bTZYYUF0WndmRWtoc09jVFlOYjFBQnJNU1FsWTlaY1M2eXJm?=
 =?utf-8?B?RS85MlJPMFRYckpYbW1hWUpLbzRzd0VOZ29yV2FpV2lraEw2cGdmRXJBV29o?=
 =?utf-8?B?M3RQT0JDQ0xjdE1Ua0ZkRm5maFJ0Z2dyY3hSeDh2SUJnRDVseFRmMHYxaGlm?=
 =?utf-8?B?dkMwck4vdDNaa1poS2NmTmoxNU1VTE5RRDNzeXcyWGxUeTN0MTZNUkRjempx?=
 =?utf-8?B?enM1OHEwbDF5dTliV0M4eFFleWdHRWVHcWJqaDhONVNTYTN4YW5uMC9CTHFO?=
 =?utf-8?B?VFlIMUUrYzlPbEhTTDdKN1IvOVRsOHNLNVVUZ1g1eTJ6NUdEQzRvM29rZHR1?=
 =?utf-8?B?MHh6cVZHVTNhWU5ST3dNKzRBSm5xcWNaaVk4V2YyWEtVcVV3NXFJb0NPdUpt?=
 =?utf-8?B?VnJ1ODVUTm1hZ2ZBRjU0Wnk3UFJwamVRWVdwMDM5bldmcVZqYlYzVHliYzZL?=
 =?utf-8?B?WkdSWDlUU1pGY01xQzNSR0FnbzdvTTBKMWlyRCtmSnJheW9TckEvRnNodzZC?=
 =?utf-8?B?TDVoNFdRZmJZOE9jR3MxbUMxcjdldEx5dnhMRjNxNWdNZWNTY3hWaUI3MDA3?=
 =?utf-8?B?b1l5amNzRWFxOUtXOE1vVTFmTEVLZG5sckxDd2lYdFFoNUZNRU1sMjVqNFd2?=
 =?utf-8?B?RkJBRlJkQWV3b3FKakpRYW5RbUtMR0R6ajRnanJ4YzI2b3hMWGhReDFJWlBt?=
 =?utf-8?B?dGlwVkFZTHgzejNkY0xjdEM3ZlJTK2srVWJqbmtHZVY3ajhRaElGMFg0ajly?=
 =?utf-8?B?MU43Wi82ZE44ZG1YOFp6ckQwQ09qTk1TZ0hmK1ZhTDEydWwrZ1JOTUpEdElH?=
 =?utf-8?B?cUVPNS9KajVJZUc5QVpGNG1aWjA5dGRMdjY4elRaMnZnRGUwanBmZzFWVjQr?=
 =?utf-8?B?U1NOY3lMbmVGTWxDQUZ5ZDRkeUtSeXJXeTlDcFlrdER6T29samtTVVpGaUFR?=
 =?utf-8?B?NHg3Qmt3NDhMVnlMN3BNNDg2Z2dmVjBibERXZlgvZ1VqcnhDdTdxdC9hd0Ur?=
 =?utf-8?B?N1JoSytRaFp5S215K040SC9vUlBaeFhsL3kxcG1UUTdxdGhKNkhkYm90aDJk?=
 =?utf-8?Q?z9jFvQjH2N/34?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f8909be3-e98d-470f-80f4-08d96e30f6fd
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5136.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Sep 2021 16:44:43.3379
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: aY3YP7FlwkTu4lGhATrgAyuAsZYirR+3jsvwFTSR9N+AuTwKGSqT0HeRsfHnGQJcf4PYnTiIjb6ygVppjYh1AQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR12MB5448
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/2/2021 6:34 PM, Greg KH wrote:
> On Thu, Sep 02, 2021 at 05:10:24PM +0200, Christian König wrote:
>> Am 02.09.21 um 14:20 schrieb Greg KH:
>>> On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
>>>> Am 02.09.21 um 12:38 schrieb Greg KH:
>>>>> On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
>>>>>> debugfs_create_file() returns encoded error so
>>>>>> use IS_ERR for checking return value.
>>>>>>
>>>>>> References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&amp;data=04%7C01%7Cnirmoy.das%40amd.com%7C7a1f1095c0d64416576c08d96e2f7b38%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661973378236086%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=g9BQRJG8gvjGFq6oj5vk9PCemQ39U19CLmkMNHVUafg%3D&amp;reserved=0
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>> ---
>>>>>>     fs/debugfs/inode.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>>>>>> index 8129a430d789..2f117c57160d 100644
>>>>>> --- a/fs/debugfs/inode.c
>>>>>> +++ b/fs/debugfs/inode.c
>>>>>> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
>>>>>>     {
>>>>>>     	struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
>>>>>> -	if (de)
>>>>>> +	if (!IS_ERR(de))
>>>>>>     		d_inode(de)->i_size = file_size;
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>>>>>> -- 
>>>>>> 2.32.0
>>>>>>
>>>>> Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
>>>> Thinking more about this if I'm not completely mistaken
>>>> debugfs_create_file() returns -ENODEV when debugfs is disabled and NULL on
>>>> any other error.
>>> How can this function be called if debugfs is not enabled in the system
>>> configuration?  This _is_ the debugfs core code.
>> Well, that's what I meant. The original code is correct and Nirmoy's patch
>> here is breaking it.
> Ah, yes, sorry, you are right.  This function can not return an error
> value, if something went wrong, the result will always be NULL.


The issue occurs when CONFIG_DEBUG_FS=y and CONFIG_DEBUG_FS_ALLOW_NONE=y 
config options are set, so a call to

debugfs_create_file() will return ERR_PTR(-EPERM) instead of NULL.


>
>> Nirmoys other patch is for a driver and there the function can indeed return
>> both error code and NULL.
> You should never be checking this stuff in a caller anyway, so no, don't
> do it there either.
>
> thanks,
>
> greg k-h