diff mbox series

reiserfs: journal: Increase jl pointer check

Message ID 20221025084704.3922-1-zeming@nfschina.com
State New
Headers
Series reiserfs: journal: Increase jl pointer check |

Commit Message

Li zeming Oct. 25, 2022, 8:47 a.m. UTC 
  If kzalloc fails to allocate the jl pointer, NULL is returned directly.

Signed-off-by: Li zeming <zeming@nfschina.com>
---
 fs/reiserfs/journal.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

NeilBrown Oct. 25, 2022, 9:30 p.m. UTC | #1 
On Tue, 25 Oct 2022, Li zeming wrote:
> If kzalloc fails to allocate the jl pointer, NULL is returned directly.
> 
> Signed-off-by: Li zeming <zeming@nfschina.com>
> ---
>  fs/reiserfs/journal.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
> index 94addfcefede..d64e9de126c1 100644
> --- a/fs/reiserfs/journal.c
> +++ b/fs/reiserfs/journal.c
> @@ -2569,6 +2569,9 @@ static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
>  	struct reiserfs_journal_list *jl;
>  	jl = kzalloc(sizeof(struct reiserfs_journal_list),
>  		     GFP_NOFS | __GFP_NOFAIL);
> +	if (!jl)
> +		return NULL;
> +

What do you think the __GFP_NOFAIL flag might mean?

NeilBrown



>  	INIT_LIST_HEAD(&jl->j_list);
>  	INIT_LIST_HEAD(&jl->j_working_list);
>  	INIT_LIST_HEAD(&jl->j_tail_bh_list);
> -- 
> 2.18.2
> 
>
Li zeming Oct. 26, 2022, 1:47 a.m. UTC | #2 
I'm sorry, I think __GFP_NOFAIL should be like this:

The __GFP_NOFAIL flag will cause memory to be allocated an infinite
 number of times until the allocation is successful, but it is best to
 use it only for very necessary code, and try not to use it.

I'm not sure alloc_journal_list function is a very important function
 here. If it is, this patch ignores it and does not consider it anymore
 __GFP_NOFAIL allocatiIon problem.
NeilBrown Oct. 26, 2022, 3:05 a.m. UTC | #3 
On Wed, 26 Oct 2022, Li zeming wrote:
> I'm sorry, I think __GFP_NOFAIL should be like this:
> 
> The __GFP_NOFAIL flag will cause memory to be allocated an infinite
>  number of times until the allocation is successful, but it is best to
>  use it only for very necessary code, and try not to use it.

I agree with that.  But you didn't try not to use it - you left it there
in the code.
Had you removed the flag as well, then your patch would have made a bit
more sense.  However you would then need to explain why it is safe to
return NULL from alloc_journal_list().  Are you sure callers don't try
to dereference the pointer?

> 
> I'm not sure alloc_journal_list function is a very important function
>  here. If it is, this patch ignores it and does not consider it anymore
>  __GFP_NOFAIL allocatiIon problem.
> 
> 

You are correct that the function isn't very important, but that is
because reiserfs itself isn't very important.
You might not have noticed, but in the reiserfs Kconfig file it says:

	  Reiserfs is deprecated and scheduled to be removed from the kernel
	  in 2025. If you are still using it, please migrate to another
	  filesystem or tell us your usecase for reiserfs.

So it will still be around for a few years, but there isn't much point
with small clean-ups like this as hardly anyone uses the code, and in a
few years it won't be in mainline Linux at all.

Thanks,
NeilBrown
Li zeming Oct. 26, 2022, 3:55 a.m. UTC | #4 
many thanks for your reply. I think I should do something more meaningful
.
diff mbox series

Patch

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index 94addfcefede..d64e9de126c1 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -2569,6 +2569,9 @@  static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
 	struct reiserfs_journal_list *jl;
 	jl = kzalloc(sizeof(struct reiserfs_journal_list),
 		     GFP_NOFS | __GFP_NOFAIL);
+	if (!jl)
+		return NULL;
+
 	INIT_LIST_HEAD(&jl->j_list);
 	INIT_LIST_HEAD(&jl->j_working_list);
 	INIT_LIST_HEAD(&jl->j_tail_bh_list);