linux-kernel - Re: [PATCH v2 6/6] staging: exfat: replace kmalloc with kmalloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20191101093447.GG18421@kadam>
Date:   Fri, 1 Nov 2019 12:34:47 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Roi Martin <jroi.martin@...il.com>
Cc:     valdis.kletnieks@...edu, devel@...verdev.osuosl.org,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/6] staging: exfat: replace kmalloc with kmalloc_array

On Thu, Oct 31, 2019 at 05:03:56PM +0100, Roi Martin wrote:
> > > diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c
> > > index f71235c6a338..f4f82aecc05d 100644
> > > --- a/drivers/staging/exfat/exfat_core.c
> > > +++ b/drivers/staging/exfat/exfat_core.c
> > > @@ -713,8 +713,8 @@ static s32 __load_upcase_table(struct super_block *sb, sector_t sector,
> > >  
> > >  	u32 checksum = 0;
> > >  
> > > -	upcase_table = p_fs->vol_utbl = kmalloc(UTBL_COL_COUNT * sizeof(u16 *),
> > > -						GFP_KERNEL);
> > > +	upcase_table = kmalloc_array(UTBL_COL_COUNT, sizeof(u16 *), GFP_KERNEL);
> > > +	p_fs->vol_utbl = upcase_table;
> > 
> > This patch is fine, but one idea for future patches is that you could
> > remove the "upcase_table" variable and use "p_fs->vol_utbl" everywhere
> > instead.
> 
> Thanks for the suggestion.
> 
> This is my first contribution and I tried to introduce the minimum
> number of changes necessary to fix the issues reported by checkpatch.pl.
> Also, I'm still immersed in getting familiar with the contribution
> process and the code.
> 
> Do you think it makes sense to include this change in a future patch
> series along with other refactoring? Or, should I modify this patch?

No don't modify the patch.  The patch is fine.

> 
> By the way, upcase_table is sometimes accessed in quite complex ways.
> For instance:
> 
> 	upcase_table[col_index][get_row_index(index)] = uni;
> 
> Where having an intermediate variable instead of using the struct field
> directly seems to improve readability a bit. Otherwise:
> 
> 	p_fs->vol_utbl[col_index][get_row_index(index)] = uni;

This line isn't very complex.  It's fine.


> 
> I assume, in cases like this, from a coding style perspective, the
> following approach is preferred:
> 
> 	row_index = get_row_index(index);
> 	p_fs->vol_utbl[col_index][row_index] = uni;

But this is better, yes.

regards,
dan carpenter