UBIFS: use kmalloc_array() in recomp_data_node()

fs/ubifs/journal.c static int recomp_data_node(struct ubifs_data_node *dn, int *new_len) · line 1109

Quoted 5 lines · Dan Carpenter

fs/ubifs/journal.c +1 -1

 	out_len = le32_to_cpu(dn->size);
-	buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS);
+	buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
 	if (!buf)
 		return -ENOMEM;

I think this makes the code unreadable, because we really allocate a buffer, not an array.

Quoted 9 lines · Artem Bityutskiy

fs/ubifs/journal.c static int recomp_data_node(struct ubifs_data_node *dn, int *new_len) · line 1109

Quoted 5 lines · Dan Carpenter

fs/ubifs/journal.c +1 -1

 	out_len = le32_to_cpu(dn->size);
-	buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS);
+	buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
 	if (!buf)
 		return -ENOMEM;

I think this makes the code unreadable, because we really allocate a buffer, not an array.

The problem with the original code is that the multiply looks very suspect. Everyone who reads it has to backtrack to find where dn->size is capped.

I guess in one sense we never allocate an array, we always declare it on the stack. We debated the naming and there really isn't a good name. kmalloc_safe() isn't right either. But anyway, the intent is that eventually someone will right a coccinelle script which replaces all these allocations with kmalloc_array().

When I look at this code more, I still don't see a place where dn->size is capped. So I think we *need* the integer overflow check as an integer overflow fix and not just as a cleanup.

Quoted 21 lines · Dan Carpenter

Quoted 9 lines · Artem Bityutskiy

fs/ubifs/journal.c static int recomp_data_node(struct ubifs_data_node *dn, int *new_len) · line 1109

Quoted 5 lines · Dan Carpenter

fs/ubifs/journal.c +1 -1

 	out_len = le32_to_cpu(dn->size);
-	buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS);
+	buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
 	if (!buf)
 		return -ENOMEM;

I think this makes the code unreadable, because we really allocate a buffer, not an array.

The problem with the original code is that the multiply looks very suspect. Everyone who reads it has to backtrack to find where dn->size is capped.

I guess in one sense we never allocate an array, we always declare it on the stack. We debated the naming and there really isn't a good name. kmalloc_safe() isn't right either. But anyway, the intent is that eventually someone will right a coccinelle script which replaces all these allocations with kmalloc_array().

Quoted 3 lines

When I look at this code more, I still don't see a place where dn->size is capped. So I think we *need* the integer overflow check as an integer overflow fix and not just as a cleanup.

It is validated in fs/ubifs/io.c in 'ubifs_check_node()'.

'dn' stands for 'direntry node'. We read it from the media and validate it immediately after we've read it, including 'dn->len'.

The entire code is written with the following assumption that whatever is read from the flash media is validated.

It's actually dn->size that we care about here. That's not checked in ubifs_check_node(). :( It may be checked somewhere else, I'm still looking.

Quoted 3 lines · Dan Carpenter

It's actually dn->size that we care about here. That's not checked in ubifs_check_node(). :( It may be checked somewhere else, I'm still looking.

Wow, despite us trying to be very careful about validating what we read from flash, it seems we indeed never validate 'size'... Let me invent a fix for this, which should also be sent to -stable.

Quoted 9 lines · Artem Bityutskiy

fs/ubifs/journal.c static int recomp_data_node(struct ubifs_data_node *dn, int *new_len) · line 1109

Quoted 5 lines · Dan Carpenter

fs/ubifs/journal.c +1 -1

 	out_len = le32_to_cpu(dn->size);
-	buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS);
+	buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
 	if (!buf)
 		return -ENOMEM;

I think this makes the code unreadable, because we really allocate a buffer, not an array.

The problem with the original code is that the multiply looks very suspect. Everyone who reads it has to backtrack to find where dn->size is capped.

I guess in one sense we never allocate an array, we always declare it on the stack. We debated the naming and there really isn't a good name. kmalloc_safe() isn't right either. But anyway, the intent is that eventually someone will right a coccinelle script which replaces all these allocations with kmalloc_array().

Did you consider kcalloc() ? Just like malloc() / calloc() libc functions?

Quoted 2 lines · Artem Bityutskiy

Did you consider kcalloc() ? Just like malloc() / calloc() libc functions?

Ah, there is kcaoloc already, so I guess then we could just put the overflow check there instead?

Quoted 15 lines · Dan Carpenter

Quoted 9 lines · Artem Bityutskiy

fs/ubifs/journal.c static int recomp_data_node(struct ubifs_data_node *dn, int *new_len) · line 1109

Quoted 5 lines · Dan Carpenter

fs/ubifs/journal.c +1 -1

 	out_len = le32_to_cpu(dn->size);
-	buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS);
+	buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
 	if (!buf)
 		return -ENOMEM;

I think this makes the code unreadable, because we really allocate a buffer, not an array.

The problem with the original code is that the multiply looks very suspect. Everyone who reads it has to backtrack to find where dn->size is capped.

Quoted 5 lines

I guess in one sense we never allocate an array, we always declare it on the stack. We debated the naming and there really isn't a good name. kmalloc_safe() isn't right either. But anyway, the intent is that eventually someone will right a coccinelle script which replaces all these allocations with kmalloc_array().

Did you consider kcalloc() ? Just like malloc() / calloc() libc functions?