Many new checkpatch warnings have been introduce to ntfs3. These could
have been prevent if checkpatch is used. One thing that worrys me is
that Konstantin puts new code without code reviewing process to ntfs3.
Patch commit message says one thing, but one huge patch address that and
lot of just refactoring code. Also with review process we can prevent
these kind of silly checkpatch mistakes.
Kmalloc_array was my fault for some reason checkpatch did not show
those. I have no idea how, but I just fix it now and be very ashamed.
You should also Konstantin use checkpatch always before push so you can
spot these things before hand. I will try to get CI going for patches.
Kari Argillander (5):
fs/ntfs3: Use kmalloc_array over kmalloc with multiply
fs/ntfs3: Use consistent spacing around '+'
fs/ntfs3: Place Comparisons constant right side of the test
fs/ntfs3: Remove braces from single statment block
fs/ntfs3: Remove tabs before spaces from comment
fs/ntfs3/bitmap.c | 2 +-
fs/ntfs3/frecord.c | 8 ++++----
fs/ntfs3/index.c | 4 ++--
fs/ntfs3/lznt.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
base-commit: d3624466b56dd5b1886c1dff500525b544c19c83
--
2.25.1
For better code readability place constant always right side of the
test. This will also address checkpatch warning.
Signed-off-by: Kari Argillander <[email protected]>
---
fs/ntfs3/frecord.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index b207d260ac06..bc3887635012 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -1606,7 +1606,7 @@ struct ATTR_FILE_NAME *ni_fname_type(struct ntfs_inode *ni, u8 name_type,
*le = NULL;
- if (FILE_NAME_POSIX == name_type)
+ if (name_type == FILE_NAME_POSIX)
return NULL;
/* Enumerate all names. */
--
2.25.1
If we do not use kmalloc_array we get checkpatch warning. It is also
little safer if something goes wrong with coding.
Signed-off-by: Kari Argillander <[email protected]>
---
fs/ntfs3/bitmap.c | 2 +-
fs/ntfs3/index.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/ntfs3/bitmap.c b/fs/ntfs3/bitmap.c
index 831501555009..aa0b1ea66cd0 100644
--- a/fs/ntfs3/bitmap.c
+++ b/fs/ntfs3/bitmap.c
@@ -1331,7 +1331,7 @@ int wnd_extend(struct wnd_bitmap *wnd, size_t new_bits)
new_last = wbits;
if (new_wnd != wnd->nwnd) {
- new_free = kmalloc(new_wnd * sizeof(u16), GFP_NOFS);
+ new_free = kmalloc_array(new_wnd, sizeof(u16), GFP_NOFS);
if (!new_free)
return -ENOMEM;
diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
index 0daca9adc54c..7676a4a657d5 100644
--- a/fs/ntfs3/index.c
+++ b/fs/ntfs3/index.c
@@ -685,7 +685,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
if (end > 0x10000)
goto next;
- offs = kmalloc(sizeof(u16) * nslots, GFP_NOFS);
+ offs = kmalloc_array(nslots, sizeof(u16), GFP_NOFS);
if (!offs)
goto next;
@@ -707,7 +707,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
u16 *ptr;
int new_slots = ALIGN(2 * nslots, 8);
- ptr = kmalloc(sizeof(u16) * new_slots, GFP_NOFS);
+ ptr = kmalloc_array(new_slots, sizeof(u16), GFP_NOFS);
if (ptr)
memcpy(ptr, offs, sizeof(u16) * max_idx);
kfree(offs);
--
2.25.1
On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> If we do not use kmalloc_array we get checkpatch warning. It is also
> little safer if something goes wrong with coding.
[]
> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
[]
> @@ -707,7 +707,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
> ? u16 *ptr;
> ? int new_slots = ALIGN(2 * nslots, 8);
> ?
>
> - ptr = kmalloc(sizeof(u16) * new_slots, GFP_NOFS);
> + ptr = kmalloc_array(new_slots, sizeof(u16), GFP_NOFS);
> ? if (ptr)
> ? memcpy(ptr, offs, sizeof(u16) * max_idx);
This multiplication could also overflow.
Maybe use krealloc?
On Tue, Aug 31, 2021 at 07:40:58PM -0700, Joe Perches wrote:
> On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> > If we do not use kmalloc_array we get checkpatch warning. It is also
> > little safer if something goes wrong with coding.
> []
> > diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> []
> > @@ -707,7 +707,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
> > ? u16 *ptr;
> > ? int new_slots = ALIGN(2 * nslots, 8);
> > ?
> >
> > - ptr = kmalloc(sizeof(u16) * new_slots, GFP_NOFS);
> > + ptr = kmalloc_array(new_slots, sizeof(u16), GFP_NOFS);
> > ? if (ptr)
> > ? memcpy(ptr, offs, sizeof(u16) * max_idx);
>
> This multiplication could also overflow.
>
> Maybe use krealloc?
Seems to fit lot better here. But as I was watching this it seems that
we do not even need to resize this array. It is quite costly operation
compared to what entry compare cost.
We just need to compare it and if entry diff > 0 then we start entry
table again from zero without need resize array. It may be that we do
not even need to allocate memory. We can probably survive with stack,
but let's think that later.
This can also speed up search a lot. It is quite odd that we always fill
whole table and will not never check if we are over. Worst case is very
very bad. With this change this will be more like jump search but I
think it will be good in this case because we won't need memory allocation.
Thanks Joe.
From: Joe Perches
> Sent: 01 September 2021 03:41
>
> On Tue, 2021-08-31 at 21:15 +0300, Kari Argillander wrote:
> > If we do not use kmalloc_array we get checkpatch warning. It is also
> > little safer if something goes wrong with coding.
> []
> > diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> []
> > @@ -707,7 +707,7 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
> > u16 *ptr;
> > int new_slots = ALIGN(2 * nslots, 8);
> >
> >
> > - ptr = kmalloc(sizeof(u16) * new_slots, GFP_NOFS);
> > + ptr = kmalloc_array(new_slots, sizeof(u16), GFP_NOFS);
> > if (ptr)
> > memcpy(ptr, offs, sizeof(u16) * max_idx);
>
> This multiplication could also overflow.
Not if kmalloc_array() has suceeded.
OTOH the ALIGN(2 * nslots, 8) can also go wrong.
(But probably not if the previous kmalloc() for 1/2 the size worked.)
But there really ought to be some kind of bound check earlier.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)