All of lore.kernel.org
 help / color / mirror / Atom feed
* [f2fs-dev] [PATCH] f2fs: fix page leak in redirty_blocks
@ 2022-05-28  9:35 Jack Qiu via Linux-f2fs-devel
  2022-05-29  6:58 ` Chao Yu
  0 siblings, 1 reply; 4+ messages in thread
From: Jack Qiu via Linux-f2fs-devel @ 2022-05-28  9:35 UTC (permalink / raw)
  To: linux-f2fs-devel

When find_lock_page return error, page in [i, page_len) will leak.

Signed-off-by: Jack Qiu <jack.qiu@huawei.com>
---
 fs/f2fs/file.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 100637b1adb3..0e8938c5918e 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3954,6 +3954,12 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
 	struct page *page;
 	pgoff_t redirty_idx = page_idx;
 	int i, page_len = 0, ret = 0;
+	struct page **pages;
+
+	pages = f2fs_kvzalloc(F2FS_I_SB(inode),
+				sizeof(struct page *) * len, GFP_NOFS);
+	if (!pages)
+		return -ENOMEM;

 	page_cache_ra_unbounded(&ractl, len, 0);

@@ -3964,6 +3970,7 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
 			break;
 		}
 		page_len++;
+		pages[i] = page;
 	}

 	for (i = 0; i < page_len; i++, redirty_idx++) {
@@ -3975,8 +3982,14 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
 		set_page_dirty(page);
 		f2fs_put_page(page, 1);
 		f2fs_put_page(page, 0);
+		pages[i] = NULL;
 	}

+	/* put pages[i, page_len) when error happens */
+	for (; ret < 0 && i < page_len; i++)
+		f2fs_put_page(pages[i], 0);
+	kvfree(pages);
+
 	return ret;
 }

--
2.31.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix page leak in redirty_blocks
  2022-05-28  9:35 [f2fs-dev] [PATCH] f2fs: fix page leak in redirty_blocks Jack Qiu via Linux-f2fs-devel
@ 2022-05-29  6:58 ` Chao Yu
  2022-05-30  1:02   ` Jack Qiu via Linux-f2fs-devel
  0 siblings, 1 reply; 4+ messages in thread
From: Chao Yu @ 2022-05-29  6:58 UTC (permalink / raw)
  To: Jack Qiu, linux-f2fs-devel

On 2022/5/28 17:35, Jack Qiu via Linux-f2fs-devel wrote:
> When find_lock_page return error, page in [i, page_len) will leak.

I doubt it is impossible to fail in find_lock_page due to one extra
reference count was added in previous read_cache_page().

Thanks,

> 
> Signed-off-by: Jack Qiu <jack.qiu@huawei.com>
> ---
>   fs/f2fs/file.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 100637b1adb3..0e8938c5918e 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3954,6 +3954,12 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>   	struct page *page;
>   	pgoff_t redirty_idx = page_idx;
>   	int i, page_len = 0, ret = 0;
> +	struct page **pages;
> +
> +	pages = f2fs_kvzalloc(F2FS_I_SB(inode),
> +				sizeof(struct page *) * len, GFP_NOFS);
> +	if (!pages)
> +		return -ENOMEM;
> 
>   	page_cache_ra_unbounded(&ractl, len, 0);
> 
> @@ -3964,6 +3970,7 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>   			break;
>   		}
>   		page_len++;
> +		pages[i] = page;
>   	}
> 
>   	for (i = 0; i < page_len; i++, redirty_idx++) {
> @@ -3975,8 +3982,14 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>   		set_page_dirty(page);
>   		f2fs_put_page(page, 1);
>   		f2fs_put_page(page, 0);
> +		pages[i] = NULL;
>   	}
> 
> +	/* put pages[i, page_len) when error happens */
> +	for (; ret < 0 && i < page_len; i++)
> +		f2fs_put_page(pages[i], 0);
> +	kvfree(pages);
> +
>   	return ret;
>   }
> 
> --
> 2.31.1
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix page leak in redirty_blocks
  2022-05-29  6:58 ` Chao Yu
@ 2022-05-30  1:02   ` Jack Qiu via Linux-f2fs-devel
  2022-05-30 12:02     ` Chao Yu
  0 siblings, 1 reply; 4+ messages in thread
From: Jack Qiu via Linux-f2fs-devel @ 2022-05-30  1:02 UTC (permalink / raw)
  To: Chao Yu, linux-f2fs-devel

On 2022/5/29 14:58, Chao Yu wrote:
> On 2022/5/28 17:35, Jack Qiu via Linux-f2fs-devel wrote:
>> When find_lock_page return error, page in [i, page_len) will leak.
> 
> I doubt it is impossible to fail in find_lock_page due to one extra
> reference count was added in previous read_cache_page().

Thanks for review.
I'm not sure about it with limited knowledge. If it is true, maybe use f2fs_bug_on(sbi, !page) is better?

> 
> Thanks,
> 
>>
>> Signed-off-by: Jack Qiu <jack.qiu@huawei.com>
>> ---
>>   fs/f2fs/file.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 100637b1adb3..0e8938c5918e 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -3954,6 +3954,12 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>>       struct page *page;
>>       pgoff_t redirty_idx = page_idx;
>>       int i, page_len = 0, ret = 0;
>> +    struct page **pages;
>> +
>> +    pages = f2fs_kvzalloc(F2FS_I_SB(inode),
>> +                sizeof(struct page *) * len, GFP_NOFS);
>> +    if (!pages)
>> +        return -ENOMEM;
>>
>>       page_cache_ra_unbounded(&ractl, len, 0);
>>
>> @@ -3964,6 +3970,7 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>>               break;
>>           }
>>           page_len++;
>> +        pages[i] = page;
>>       }
>>
>>       for (i = 0; i < page_len; i++, redirty_idx++) {
>> @@ -3975,8 +3982,14 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>>           set_page_dirty(page);
>>           f2fs_put_page(page, 1);
>>           f2fs_put_page(page, 0);
>> +        pages[i] = NULL;
>>       }
>>
>> +    /* put pages[i, page_len) when error happens */
>> +    for (; ret < 0 && i < page_len; i++)
>> +        f2fs_put_page(pages[i], 0);
>> +    kvfree(pages);
>> +
>>       return ret;
>>   }
>>
>> -- 
>> 2.31.1
>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix page leak in redirty_blocks
  2022-05-30  1:02   ` Jack Qiu via Linux-f2fs-devel
@ 2022-05-30 12:02     ` Chao Yu
  0 siblings, 0 replies; 4+ messages in thread
From: Chao Yu @ 2022-05-30 12:02 UTC (permalink / raw)
  To: Jack Qiu, linux-f2fs-devel

On 2022/5/30 9:02, Jack Qiu wrote:
> On 2022/5/29 14:58, Chao Yu wrote:
>> On 2022/5/28 17:35, Jack Qiu via Linux-f2fs-devel wrote:
>>> When find_lock_page return error, page in [i, page_len) will leak.
>>
>> I doubt it is impossible to fail in find_lock_page due to one extra
>> reference count was added in previous read_cache_page().
> 
> Thanks for review.
> I'm not sure about it with limited knowledge.

I guess something like those pages were pinned by adding extra
reference, and find_lock_page() should never miss them...

 > If it is true, maybe use f2fs_bug_on(sbi, !page) is better?

I think so.

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jack Qiu <jack.qiu@huawei.com>
>>> ---
>>>    fs/f2fs/file.c | 13 +++++++++++++
>>>    1 file changed, 13 insertions(+)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 100637b1adb3..0e8938c5918e 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -3954,6 +3954,12 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>>>        struct page *page;
>>>        pgoff_t redirty_idx = page_idx;
>>>        int i, page_len = 0, ret = 0;
>>> +    struct page **pages;
>>> +
>>> +    pages = f2fs_kvzalloc(F2FS_I_SB(inode),
>>> +                sizeof(struct page *) * len, GFP_NOFS);
>>> +    if (!pages)
>>> +        return -ENOMEM;
>>>
>>>        page_cache_ra_unbounded(&ractl, len, 0);
>>>
>>> @@ -3964,6 +3970,7 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>>>                break;
>>>            }
>>>            page_len++;
>>> +        pages[i] = page;
>>>        }
>>>
>>>        for (i = 0; i < page_len; i++, redirty_idx++) {
>>> @@ -3975,8 +3982,14 @@ static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len)
>>>            set_page_dirty(page);
>>>            f2fs_put_page(page, 1);
>>>            f2fs_put_page(page, 0);
>>> +        pages[i] = NULL;
>>>        }
>>>
>>> +    /* put pages[i, page_len) when error happens */
>>> +    for (; ret < 0 && i < page_len; i++)
>>> +        f2fs_put_page(pages[i], 0);
>>> +    kvfree(pages);
>>> +
>>>        return ret;
>>>    }
>>>
>>> -- 
>>> 2.31.1
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-30 12:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-28  9:35 [f2fs-dev] [PATCH] f2fs: fix page leak in redirty_blocks Jack Qiu via Linux-f2fs-devel
2022-05-29  6:58 ` Chao Yu
2022-05-30  1:02   ` Jack Qiu via Linux-f2fs-devel
2022-05-30 12:02     ` Chao Yu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.