* [PATCH] reiserfs : fix improper free in reiserfs_get_block
@ 2020-07-14 13:05 trix
2020-07-14 13:10 ` Matthew Wilcox
0 siblings, 1 reply; 5+ messages in thread
From: trix @ 2020-07-14 13:05 UTC (permalink / raw)
To: jack, william.kucharski, jeffm, willy, joseph.qi, liao.pingfang
Cc: reiserfs-devel, linux-kernel, Tom Rix
From: Tom Rix <trix@redhat.com>
clang static analysis flags this error
inode.c:1083:5: warning: Argument to kfree() is the address of the
local variable 'unf_single', which is not memory allocated by
malloc() [unix.Malloc]
kfree(un);
^~~~~~~~~
Assignment of 'un'
/*
* We use this in case we need to allocate
* only one block which is a fastpath
*/
unp_t unf_single = 0;
...
if (blocks_needed == 1) {
un = &unf_single;
} else {
un = kcalloc(min(blocks_needed, max_to_insert),
UNFM_P_SIZE, GFP_NOFS);
if (!un) {
un = &unf_single;
blocks_needed = 1;
max_to_insert = 0;
}
}
The logic to free 'un'
if (blocks_needed != 1)
kfree(un);
Because the kcalloc failure falls back to using unf_single,
the if-check for the free is wrong.
So improve the check.
Signed-off-by: Tom Rix <trix@redhat.com>
---
fs/reiserfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 1509775da040..4d62148e43e6 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -1079,7 +1079,7 @@ int reiserfs_get_block(struct inode *inode, sector_t block,
UNFM_P_SIZE *
blocks_needed);
- if (blocks_needed != 1)
+ if (un != &unf_single)
kfree(un);
if (retval) {
--
2.18.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] reiserfs : fix improper free in reiserfs_get_block
2020-07-14 13:05 [PATCH] reiserfs : fix improper free in reiserfs_get_block trix
@ 2020-07-14 13:10 ` Matthew Wilcox
2020-07-14 13:12 ` Tom Rix
0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2020-07-14 13:10 UTC (permalink / raw)
To: trix
Cc: jack, william.kucharski, jeffm, joseph.qi, liao.pingfang,
reiserfs-devel, linux-kernel
On Tue, Jul 14, 2020 at 06:05:09AM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
>
> clang static analysis flags this error
>
> inode.c:1083:5: warning: Argument to kfree() is the address of the
> local variable 'unf_single', which is not memory allocated by
> malloc() [unix.Malloc]
> kfree(un);
> ^~~~~~~~~
> Assignment of 'un'
>
> /*
> * We use this in case we need to allocate
> * only one block which is a fastpath
> */
> unp_t unf_single = 0;
>
> ...
>
> if (blocks_needed == 1) {
> un = &unf_single;
> } else {
> un = kcalloc(min(blocks_needed, max_to_insert),
> UNFM_P_SIZE, GFP_NOFS);
> if (!un) {
> un = &unf_single;
> blocks_needed = 1;
> max_to_insert = 0;
> }
> }
>
> The logic to free 'un'
>
> if (blocks_needed != 1)
> kfree(un);
>
> Because the kcalloc failure falls back to using unf_single,
> the if-check for the free is wrong.
I think you mean "Because clang's static analysis is limited, it
warns incorrectly about this". There's no path to get to the
kfree with blocks_needed != 1 and un being equal to &unf_single.
> So improve the check.
>
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
> fs/reiserfs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index 1509775da040..4d62148e43e6 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -1079,7 +1079,7 @@ int reiserfs_get_block(struct inode *inode, sector_t block,
> UNFM_P_SIZE *
> blocks_needed);
>
> - if (blocks_needed != 1)
> + if (un != &unf_single)
> kfree(un);
I don't actually object to this patch, but your analysis of clang's
analysis is wrong.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] reiserfs : fix improper free in reiserfs_get_block
2020-07-14 13:10 ` Matthew Wilcox
@ 2020-07-14 13:12 ` Tom Rix
2020-07-15 8:04 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Tom Rix @ 2020-07-14 13:12 UTC (permalink / raw)
To: Matthew Wilcox
Cc: jack, william.kucharski, jeffm, joseph.qi, liao.pingfang,
reiserfs-devel, linux-kernel
On 7/14/20 6:10 AM, Matthew Wilcox wrote:
> On Tue, Jul 14, 2020 at 06:05:09AM -0700, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> clang static analysis flags this error
>>
>> inode.c:1083:5: warning: Argument to kfree() is the address of the
>> local variable 'unf_single', which is not memory allocated by
>> malloc() [unix.Malloc]
>> kfree(un);
>> ^~~~~~~~~
>> Assignment of 'un'
>>
>> /*
>> * We use this in case we need to allocate
>> * only one block which is a fastpath
>> */
>> unp_t unf_single = 0;
>>
>> ...
>>
>> if (blocks_needed == 1) {
>> un = &unf_single;
>> } else {
>> un = kcalloc(min(blocks_needed, max_to_insert),
>> UNFM_P_SIZE, GFP_NOFS);
>> if (!un) {
>> un = &unf_single;
>> blocks_needed = 1;
>> max_to_insert = 0;
>> }
>> }
>>
>> The logic to free 'un'
>>
>> if (blocks_needed != 1)
>> kfree(un);
>>
>> Because the kcalloc failure falls back to using unf_single,
>> the if-check for the free is wrong.
> I think you mean "Because clang's static analysis is limited, it
> warns incorrectly about this". There's no path to get to the
> kfree with blocks_needed != 1 and un being equal to &unf_single.
Ok.
>> So improve the check.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>> fs/reiserfs/inode.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
>> index 1509775da040..4d62148e43e6 100644
>> --- a/fs/reiserfs/inode.c
>> +++ b/fs/reiserfs/inode.c
>> @@ -1079,7 +1079,7 @@ int reiserfs_get_block(struct inode *inode, sector_t block,
>> UNFM_P_SIZE *
>> blocks_needed);
>>
>> - if (blocks_needed != 1)
>> + if (un != &unf_single)
>> kfree(un);
> I don't actually object to this patch, but your analysis of clang's
> analysis is wrong.
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] reiserfs : fix improper free in reiserfs_get_block
2020-07-14 13:12 ` Tom Rix
@ 2020-07-15 8:04 ` Jan Kara
2020-07-16 13:48 ` Tom Rix
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kara @ 2020-07-15 8:04 UTC (permalink / raw)
To: Tom Rix
Cc: Matthew Wilcox, jack, william.kucharski, jeffm, joseph.qi,
liao.pingfang, reiserfs-devel, linux-kernel
On Tue 14-07-20 06:12:47, Tom Rix wrote:
>
> On 7/14/20 6:10 AM, Matthew Wilcox wrote:
> > On Tue, Jul 14, 2020 at 06:05:09AM -0700, trix@redhat.com wrote:
> >> From: Tom Rix <trix@redhat.com>
> >>
> >> clang static analysis flags this error
> >>
> >> inode.c:1083:5: warning: Argument to kfree() is the address of the
> >> local variable 'unf_single', which is not memory allocated by
> >> malloc() [unix.Malloc]
> >> kfree(un);
> >> ^~~~~~~~~
> >> Assignment of 'un'
> >>
> >> /*
> >> * We use this in case we need to allocate
> >> * only one block which is a fastpath
> >> */
> >> unp_t unf_single = 0;
> >>
> >> ...
> >>
> >> if (blocks_needed == 1) {
> >> un = &unf_single;
> >> } else {
> >> un = kcalloc(min(blocks_needed, max_to_insert),
> >> UNFM_P_SIZE, GFP_NOFS);
> >> if (!un) {
> >> un = &unf_single;
> >> blocks_needed = 1;
> >> max_to_insert = 0;
> >> }
> >> }
> >>
> >> The logic to free 'un'
> >>
> >> if (blocks_needed != 1)
> >> kfree(un);
> >>
> >> Because the kcalloc failure falls back to using unf_single,
> >> the if-check for the free is wrong.
> > I think you mean "Because clang's static analysis is limited, it
> > warns incorrectly about this". There's no path to get to the
> > kfree with blocks_needed != 1 and un being equal to &unf_single.
>
> Ok.
I agree with Matthew the patch will make the code more obviously correct so
it's a sensible cleanup. But the changelog needs to redone to reflect this
is just a cleanup before the patch can be merged.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] reiserfs : fix improper free in reiserfs_get_block
2020-07-15 8:04 ` Jan Kara
@ 2020-07-16 13:48 ` Tom Rix
0 siblings, 0 replies; 5+ messages in thread
From: Tom Rix @ 2020-07-16 13:48 UTC (permalink / raw)
To: Jan Kara
Cc: Matthew Wilcox, william.kucharski, jeffm, joseph.qi,
liao.pingfang, reiserfs-devel, linux-kernel
On 7/15/20 1:04 AM, Jan Kara wrote:
> On Tue 14-07-20 06:12:47, Tom Rix wrote:
>> On 7/14/20 6:10 AM, Matthew Wilcox wrote:
>>> On Tue, Jul 14, 2020 at 06:05:09AM -0700, trix@redhat.com wrote:
>>>> From: Tom Rix <trix@redhat.com>
>>>>
>>>> clang static analysis flags this error
>>>>
>>>> inode.c:1083:5: warning: Argument to kfree() is the address of the
>>>> local variable 'unf_single', which is not memory allocated by
>>>> malloc() [unix.Malloc]
>>>> kfree(un);
>>>> ^~~~~~~~~
>>>> Assignment of 'un'
>>>>
>>>> /*
>>>> * We use this in case we need to allocate
>>>> * only one block which is a fastpath
>>>> */
>>>> unp_t unf_single = 0;
>>>>
>>>> ...
>>>>
>>>> if (blocks_needed == 1) {
>>>> un = &unf_single;
>>>> } else {
>>>> un = kcalloc(min(blocks_needed, max_to_insert),
>>>> UNFM_P_SIZE, GFP_NOFS);
>>>> if (!un) {
>>>> un = &unf_single;
>>>> blocks_needed = 1;
>>>> max_to_insert = 0;
>>>> }
>>>> }
>>>>
>>>> The logic to free 'un'
>>>>
>>>> if (blocks_needed != 1)
>>>> kfree(un);
>>>>
>>>> Because the kcalloc failure falls back to using unf_single,
>>>> the if-check for the free is wrong.
>>> I think you mean "Because clang's static analysis is limited, it
>>> warns incorrectly about this". There's no path to get to the
>>> kfree with blocks_needed != 1 and un being equal to &unf_single.
>> Ok.
> I agree with Matthew the patch will make the code more obviously correct so
> it's a sensible cleanup. But the changelog needs to redone to reflect this
> is just a cleanup before the patch can be merged.
>
> Honza
I am going to look into the problem with the analyzer because that is where the fix should go.
If the problem isn't resolvable, i will loop back to this clean up.
Tom
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-16 13:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-14 13:05 [PATCH] reiserfs : fix improper free in reiserfs_get_block trix
2020-07-14 13:10 ` Matthew Wilcox
2020-07-14 13:12 ` Tom Rix
2020-07-15 8:04 ` Jan Kara
2020-07-16 13:48 ` Tom Rix
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox