[PATCH 0/2] media: siano: Fix two bugs in smsdvb_debugfs_create()

[PATCH 0/2] media: siano: Fix two bugs in smsdvb_debugfs_create()

[Jinjie Ruan]

As debugfs_create_file() returns ERR_PTR and never NULL, check NULL is not
correct to catch the error.

And if kzalloc() fails in smsdvb_debugfs_create(), the dir and file which
is created by debugfs_create_dir() and debugfs_create_file() is
not freed.

This patch set fix the above 2 bugs.

Jinjie Ruan (2):
  media: siano: Fix the NULL vs IS_ERR() bug for debugfs_create_file()
  media: siano: Fix the missing err path in smsdvb_debugfs_create()

 drivers/media/common/siano/smsdvb-debugfs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] media: siano: Fix the NULL vs IS_ERR() bug for debugfs_create_file()

[Jinjie Ruan]

Since debugfs_create_file() returns ERR_PTR and never NULL, use IS_ERR() to
check the return value.

Fixes: 503efe5cfc9f ("[media] siano: split debugfs code into a separate file")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/media/common/siano/smsdvb-debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/common/siano/smsdvb-debugfs.c b/drivers/media/common/siano/smsdvb-debugfs.c
index e0beefd80d7b..16d3b9ab31c5 100644
--- a/drivers/media/common/siano/smsdvb-debugfs.c
+++ b/drivers/media/common/siano/smsdvb-debugfs.c
@@ -369,7 +369,7 @@ int smsdvb_debugfs_create(struct smsdvb_client_t *client)
 
 	d = debugfs_create_file("stats", S_IRUGO | S_IWUSR, client->debugfs,
 				client, &debugfs_stats_ops);
-	if (!d) {
+	if (IS_ERR(d)) {
 		debugfs_remove(client->debugfs);
 		return -ENOMEM;
 	}
-- 
2.34.1

[PATCH 2/2] media: siano: Fix the missing err path in smsdvb_debugfs_create()

[Jinjie Ruan]

If kzalloc() fails in smsdvb_debugfs_create(), the dir and file which
is created by debugfs_create_dir() and debugfs_create_file() is
not freed. So use debugfs_remove_recursive() to free them.

Fixes: 503efe5cfc9f ("[media] siano: split debugfs code into a separate file")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/media/common/siano/smsdvb-debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/siano/smsdvb-debugfs.c b/drivers/media/common/siano/smsdvb-debugfs.c
index 16d3b9ab31c5..38b25e88ce57 100644
--- a/drivers/media/common/siano/smsdvb-debugfs.c
+++ b/drivers/media/common/siano/smsdvb-debugfs.c
@@ -375,8 +375,10 @@ int smsdvb_debugfs_create(struct smsdvb_client_t *client)
 	}
 
 	debug_data = kzalloc(sizeof(*client->debug_data), GFP_KERNEL);
-	if (!debug_data)
+	if (!debug_data) {
+		debugfs_remove_recursive(client->debugfs);
 		return -ENOMEM;
+	}
 
 	client->debug_data        = debug_data;
 	client->prt_dvb_stats     = smsdvb_print_dvb_stats;
-- 
2.34.1

Re: [PATCH 0/2] media: siano: Fix two bugs in smsdvb_debugfs_create()

[Ruan Jinjie]

Ping.

On 2023/9/14 11:50, Jinjie Ruan wrote:
> As debugfs_create_file() returns ERR_PTR and never NULL, check NULL is not
> correct to catch the error.
> 
> And if kzalloc() fails in smsdvb_debugfs_create(), the dir and file which
> is created by debugfs_create_dir() and debugfs_create_file() is
> not freed.
> 
> This patch set fix the above 2 bugs.
> 
> Jinjie Ruan (2):
>   media: siano: Fix the NULL vs IS_ERR() bug for debugfs_create_file()
>   media: siano: Fix the missing err path in smsdvb_debugfs_create()
> 
>  drivers/media/common/siano/smsdvb-debugfs.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Re: [DKIM] [PATCH 1/2] media: siano: Fix the NULL vs IS_ERR() bug for debugfs_create_file()

[Hans Verkuil]

On 14/09/2023 05:50, Jinjie Ruan wrote:
> Since debugfs_create_file() returns ERR_PTR and never NULL, use IS_ERR() to
> check the return value.
> 
> Fixes: 503efe5cfc9f ("[media] siano: split debugfs code into a separate file")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  drivers/media/common/siano/smsdvb-debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/siano/smsdvb-debugfs.c b/drivers/media/common/siano/smsdvb-debugfs.c
> index e0beefd80d7b..16d3b9ab31c5 100644
> --- a/drivers/media/common/siano/smsdvb-debugfs.c
> +++ b/drivers/media/common/siano/smsdvb-debugfs.c
> @@ -369,7 +369,7 @@ int smsdvb_debugfs_create(struct smsdvb_client_t *client)
>  
>  	d = debugfs_create_file("stats", S_IRUGO | S_IWUSR, client->debugfs,
>  				client, &debugfs_stats_ops);
> -	if (!d) {
> +	if (IS_ERR(d)) {
>  		debugfs_remove(client->debugfs);
>  		return -ENOMEM;
>  	}

Actually, standard behavior is not to check the error at all. It is not
considered a bug if creating a debugfs directory fails, you can just continue.

So rather than 'fixing' this, just drop the error check completely.

See e.g. commit 8c23f411296e ("media: sti: no need to check return value of
debugfs_create functions").

Regards,

	Hans

Re: [DKIM] [PATCH 2/2] media: siano: Fix the missing err path in smsdvb_debugfs_create()

[Hans Verkuil]

On 14/09/2023 05:50, Jinjie Ruan wrote:
> If kzalloc() fails in smsdvb_debugfs_create(), the dir and file which
> is created by debugfs_create_dir() and debugfs_create_file() is
> not freed. So use debugfs_remove_recursive() to free them.
> 
> Fixes: 503efe5cfc9f ("[media] siano: split debugfs code into a separate file")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  drivers/media/common/siano/smsdvb-debugfs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/siano/smsdvb-debugfs.c b/drivers/media/common/siano/smsdvb-debugfs.c
> index 16d3b9ab31c5..38b25e88ce57 100644
> --- a/drivers/media/common/siano/smsdvb-debugfs.c
> +++ b/drivers/media/common/siano/smsdvb-debugfs.c
> @@ -375,8 +375,10 @@ int smsdvb_debugfs_create(struct smsdvb_client_t *client)
>  	}
>  
>  	debug_data = kzalloc(sizeof(*client->debug_data), GFP_KERNEL);
> -	if (!debug_data)
> +	if (!debug_data) {
> +		debugfs_remove_recursive(client->debugfs);
>  		return -ENOMEM;
> +	}
>  
>  	client->debug_data        = debug_data;
>  	client->prt_dvb_stats     = smsdvb_print_dvb_stats;

It's much better to first allocate debug_data before calling debugfs_create_dir.

No need to clean anything up in that case.

You can also ignore any errors from debugfs_create_file.

Regards,

	Hans

Re: [DKIM] [PATCH 2/2] media: siano: Fix the missing err path in smsdvb_debugfs_create()

[Ruan Jinjie]

On 2023/10/5 16:30, Hans Verkuil wrote:
> On 14/09/2023 05:50, Jinjie Ruan wrote:
>> If kzalloc() fails in smsdvb_debugfs_create(), the dir and file which
>> is created by debugfs_create_dir() and debugfs_create_file() is
>> not freed. So use debugfs_remove_recursive() to free them.
>>
>> Fixes: 503efe5cfc9f ("[media] siano: split debugfs code into a separate file")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  drivers/media/common/siano/smsdvb-debugfs.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/common/siano/smsdvb-debugfs.c b/drivers/media/common/siano/smsdvb-debugfs.c
>> index 16d3b9ab31c5..38b25e88ce57 100644
>> --- a/drivers/media/common/siano/smsdvb-debugfs.c
>> +++ b/drivers/media/common/siano/smsdvb-debugfs.c
>> @@ -375,8 +375,10 @@ int smsdvb_debugfs_create(struct smsdvb_client_t *client)
>>  	}
>>  
>>  	debug_data = kzalloc(sizeof(*client->debug_data), GFP_KERNEL);
>> -	if (!debug_data)
>> +	if (!debug_data) {
>> +		debugfs_remove_recursive(client->debugfs);
>>  		return -ENOMEM;
>> +	}
>>  
>>  	client->debug_data        = debug_data;
>>  	client->prt_dvb_stats     = smsdvb_print_dvb_stats;
> 
> It's much better to first allocate debug_data before calling debugfs_create_dir.

Right! After that the code will be more cleaner.

> 
> No need to clean anything up in that case.
> 
> You can also ignore any errors from debugfs_create_file.
> 
> Regards,
> 
> 	Hans
>