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: <8f9ad7f8-d4fb-1737-6728-3278af1b3504@amd.com>
Date:   Thu, 2 Sep 2021 18:48:34 +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?RERocDdZYi9oSnhVVC92QXNmUE1CV0gxd0xsZFNOVS9YRVozc1QyUW4zejhs?=
 =?utf-8?B?eGxWZXB6SXBubEx5MDlFYkQ0NVp6M1VsZGJDY3dkeWVMRDJuNUtHT2hMdWxX?=
 =?utf-8?B?M2VUUzdTVVczMlJPdTFaWGYxeThzZlBKOE1jMllCVjNGWW8zTktLeXF2ZDV4?=
 =?utf-8?B?NGdVcjFLNWI5N2w1aWJpdUxWOE1Ib2dRK0NaU292NFRZdzV5cjdjWXJlSGlZ?=
 =?utf-8?B?Wkl6WDduOVF4TWtqOURBV09lcDVjb1FoNlFEVS9CWFh0TWlmcnF5djVSbjZ3?=
 =?utf-8?B?ekkrZ3Q4MUt0SzlrV3ZPeWFzZTUxL2NLQ0dpRHpBWWlHSkp3Y25jdGtSMEdK?=
 =?utf-8?B?OUJsSmpIaVVXTko4MnRZTHdmdHlKUmVOa2hGbFR2M0FEOVNYN1JRZzRxQUZ2?=
 =?utf-8?B?OGwxZjA4Znk5c2NhMVB2aWs5WE1MdkJidm0xZXBjMmw1YkNFcCs0OVZCKzBu?=
 =?utf-8?B?VHpPK245YWloNFFuUGZYWFFBQzJlUy80dkRPOE52L0l0aDV2Y0JZblp6MHdG?=
 =?utf-8?B?UDQyRmYzWTR0enNpeklVWExuRkxGWlVwcHI0STkvakF1Z0o4dWhhTHR0d0p0?=
 =?utf-8?B?Z0lsZ1pQMlpvZEZGMUpyWlowR0hKU1BnQXA1ZUdERHJwMC9Vc0EyNHZBbGo4?=
 =?utf-8?B?Y0ZpbjNtcFNSODZoazhPU083OGFpMmhTUUR0OE4vcE9uNXloVTNYSG54VXZR?=
 =?utf-8?B?VmJ5UzFpR2pPVXNENTBJK3lBRmwzU2Nud1NWNVBvT2NOd05McHpiTzFoRllK?=
 =?utf-8?B?Z1NrTlVwSnAzSGRKc2tpaVRKVGx1YzNBUmRMZElXa1JSVVlzRTIrait4RW5j?=
 =?utf-8?B?Wjk1enZHWG1kL1pUUDY1MHEzMWZDZnl2TVBDQzduSTFpYjJOclhlRzE1STZV?=
 =?utf-8?B?a20wZ1F4ZlR4cXpWZjFxZXhiTWIyZEFJWTFNdUJjbnBIcmFTZU9peU9zcjk3?=
 =?utf-8?B?Zm1mOU5lWHAwWTFHQ0t4aFBMQ0FhcWVyaW1HVUpBWXQwdnFCSUpPZjhXVE12?=
 =?utf-8?B?YVdHQjRXRXVSRHQ3SXQ5UVluZG00M25ndDFuYWpJR1ZybXRKZEI3OStEQjI1?=
 =?utf-8?B?ZXZtYzJ6WXdGbzA4cnMrSUk5YWJHMFZhbmZ6eENPY0JibWFEUGZ6ZFlvSHNv?=
 =?utf-8?B?UWNrekhzSTNGUzNFMU1DTjVFOEk5VVpka2wvZGFNWWthb0ErdUxUUU56ZWR0?=
 =?utf-8?B?ODRuTDNFWUxuWHZEdlkyVnl0U2FMdnJHWWlCZksrb1N5RWozVndmQkh1TFM4?=
 =?utf-8?B?T0xDUzBaVjdrdU9mL0o5SmxXZlloTHZUQ3VKM0plKzEzVHRsdTE3OTdXRks4?=
 =?utf-8?B?QitLeHhaMThPZ20zenVPM0VLZVl6c1ZHUmlVdnRmRkRZc29FMUF1TFlXQ2FU?=
 =?utf-8?B?VXNGcGdCT1NHTWRhazQ4bFNtY2gyaTBqd1l4anF4QVlNWE4vT29CMzdFMU4z?=
 =?utf-8?B?ZUsvRDhlZHJ5cll5M1RzYmdJYndvV2t4M29sbHpNY2hpUDIzTWIxdFJCOU41?=
 =?utf-8?B?djNkOGhQd3BLOUhNOG0xK0hwQTgxOU5YMG5EaHc4SFM0b1c1ZkdCbS9KT3Zm?=
 =?utf-8?B?VEt0Z0xoamxSSFoxdHNrQTFLbmxWSjFiOVljeFNDaVYyNWw2OFpFZ3Q2c2sx?=
 =?utf-8?B?cHNuODBnWFdDcThWNm10MHBKM05uL3kvWXpHbjQ3eFkzRTJkOHdvaXdRY01X?=
 =?utf-8?B?TkdKdTNhUEVhZGZrc00xNnhwVkxodzkzcHlKSWY0LzZwejNvUU5NdHlEdHV5?=
 =?utf-8?B?NmlFQVhJUGdDSlJCbGhIbkpTZWxPbFdLeVlJYUxEWWxGcWg1QkNwZGYxTDdu?=
 =?utf-8?B?ZHBrUTRkZ0xOcTJpc3hDNHhSSGljRzZVTThJaXJkTzh4UjI1b0MwcDRucmlZ?=
 =?utf-8?Q?q/qjbXPvsZm1o?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 349b930b-e846-42ff-e99b-08d96e318396
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:48:39.0809
 (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: soEIzVJ8Ei1rMjHosmIotjzX+GXAPq9eevcFzF8C9i4bbovrbjz2+lceQf3+BvPDUDUjUic9tXwloix8orgvgA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5311
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.

[I pressed send too early in my previous email :/ ]

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. So I 
think it still makes sense to keep the check with IS_ERR_OR_NULL()


Regards,

Nirmoy


>
>> 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