Hello,
Commit - fa9150a84c - replaces a call to generic_writepages() in
f2fs_write_data_pages() with write_cache_pages(), with a function pointer
argument pointing to routine: __f2fs_writepage.
-> https://git.kernel.org/linus/fa9150a84ca333f68127097c4fa1eda4b3913a22
The patch below adds a NULL pointer check, from generic_writepages(), to avoid
a possible NULL pointer dereference, in case if - mapping->a_ops->writepage -
is NULL.
===
$ git diff
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7bd22a2..9841372 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -550,9 +550,16 @@ redirty_out:
static int __f2fs_writepage(struct page *page, struct writeback_control *wbc,
void *data)
{
+ int ret = 0;
struct address_space *mapping = data;
- int ret = mapping->a_ops->writepage(page, wbc);
+
+ /* deal with chardevs and other special file */
+ if (!mapping->a_ops->writepage)
+ return ret;
+
+ ret = mapping->a_ops->writepage(page, wbc);
mapping_set_error(mapping, ret);
+
return ret;
}
===
Thank you.
--
Prasad J Pandit / Red Hat Security Response Team
DB7A 84C5 D3F9 7CD1 B5EB C939 D048 7860 3655 602B
Hi,
Thank you for your contribution.
As I consider the null pointer check, generic_writepages() originally
does so.
Therefore, I think f2fs_write_data_pages() is better to handle this.
Please review the modified patch.
Thanks,
---
From d3c811a51c7062fb1b66bec910ed346447c02032 Mon Sep 17 00:00:00 2001
From: P J P <[email protected]>
Date: Wed, 3 Apr 2013 11:38:00 +0900
Subject: [PATCH] f2fs: add NULL pointer check
Cc: [email protected], [email protected],
[email protected]
Commit - fa9150a84c - replaces a call to generic_writepages() in
f2fs_write_data_pages() with write_cache_pages(), with a function
pointer
argument pointing to routine: __f2fs_writepage.
->
https://git.kernel.org/linus/fa9150a84ca333f68127097c4fa1eda4b3913a22
This patch adds a NULL pointer check in f2fs_write_data_pages() to
avoid
a possible NULL pointer dereference, in case if -
mapping->a_ops->writepage -
is NULL.
Signed-off-by: P J P <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/data.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 47a2d7c..cf9ff5f 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -559,6 +559,10 @@ static int f2fs_write_data_pages(struct
address_space *mapping,
int ret;
long excess_nrtw = 0, desired_nrtw;
+ /* deal with chardevs and other special file */
+ if (!mapping->a_ops->writepage)
+ return 0;
+
if (wbc->nr_to_write < MAX_DESIRED_PAGES_WP) {
desired_nrtw = MAX_DESIRED_PAGES_WP;
excess_nrtw = desired_nrtw - wbc->nr_to_write;
--
1.8.1.3.566.gaa39828
--
Jaegeuk Kim
Samsung
2013/4/3, Jaegeuk Kim <[email protected]>:
> Hi,
> Thank you for your contribution.
>
> As I consider the null pointer check, generic_writepages() originally
> does so.
> Therefore, I think f2fs_write_data_pages() is better to handle this.
> Please review the modified patch.
> Thanks,
>
> ---
> From d3c811a51c7062fb1b66bec910ed346447c02032 Mon Sep 17 00:00:00 2001
> From: P J P <[email protected]>
> Date: Wed, 3 Apr 2013 11:38:00 +0900
> Subject: [PATCH] f2fs: add NULL pointer check
> Cc: [email protected], [email protected],
> [email protected]
>
> Commit - fa9150a84c - replaces a call to generic_writepages() in
> f2fs_write_data_pages() with write_cache_pages(), with a function
> pointer
> argument pointing to routine: __f2fs_writepage.
>
> ->
> https://git.kernel.org/linus/fa9150a84ca333f68127097c4fa1eda4b3913a22
>
> This patch adds a NULL pointer check in f2fs_write_data_pages() to
> avoid
> a possible NULL pointer dereference, in case if -
> mapping->a_ops->writepage -
> is NULL.
Yes, I agree. Looks better!
Reviewed-by: Namjae Jeon <[email protected]>
Thanks.
>
> Signed-off-by: P J P <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
Hello Jaegeuk,
+-- On Wed, 3 Apr 2013, Jaegeuk Kim wrote --+
| Therefore, I think f2fs_write_data_pages() is better to handle this. Please
| review the modified patch. Thanks,
|
| diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
| index 47a2d7c..cf9ff5f 100644
| --- a/fs/f2fs/data.c
| +++ b/fs/f2fs/data.c
| @@ -559,6 +559,10 @@ static int f2fs_write_data_pages(struct
| address_space *mapping,
| int ret;
| long excess_nrtw = 0, desired_nrtw;
|
| + /* deal with chardevs and other special file */
| + if (!mapping->a_ops->writepage)
| + return 0;
| +
| if (wbc->nr_to_write < MAX_DESIRED_PAGES_WP) {
| desired_nrtw = MAX_DESIRED_PAGES_WP;
| excess_nrtw = desired_nrtw - wbc->nr_to_write;
|
Yeah, I considered this, it saves us a call to `write_cache_pages'; But then
thought it'll help if `__f2fs_writepage' is called from other place later.
I agree, above patch serves too.
Thank you.
--
Prasad J Pandit / Red Hat Security Response Team
DB7A 84C5 D3F9 7CD1 B5EB C939 D048 7860 3655 602B
+-- On Wed, 3 Apr 2013, Jaegeuk Kim wrote --+
| diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
| index 47a2d7c..cf9ff5f 100644
| --- a/fs/f2fs/data.c
| +++ b/fs/f2fs/data.c
| @@ -559,6 +559,10 @@ static int f2fs_write_data_pages(struct
| address_space *mapping,
| int ret;
| long excess_nrtw = 0, desired_nrtw;
|
| + /* deal with chardevs and other special file */
| + if (!mapping->a_ops->writepage)
| + return 0;
| +
Small question, is it okay to `return 0' here?
Earlier even if `generic_writepages' returned 0, that did not abort routine
`f2fs_write_data_pages'.
Thank you.
--
Prasad J Pandit / Red Hat Security Response Team
DB7A 84C5 D3F9 7CD1 B5EB C939 D048 7860 3655 602B
Hi,
2013-04-03 (수), 12:30 +0530, P J P:
> +-- On Wed, 3 Apr 2013, Jaegeuk Kim wrote --+
> | diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> | index 47a2d7c..cf9ff5f 100644
> | --- a/fs/f2fs/data.c
> | +++ b/fs/f2fs/data.c
> | @@ -559,6 +559,10 @@ static int f2fs_write_data_pages(struct
> | address_space *mapping,
> | int ret;
> | long excess_nrtw = 0, desired_nrtw;
> |
> | + /* deal with chardevs and other special file */
> | + if (!mapping->a_ops->writepage)
> | + return 0;
> | +
>
> Small question, is it okay to `return 0' here?
>
> Earlier even if `generic_writepages' returned 0, that did not abort routine
> `f2fs_write_data_pages'.
I'm confusing the question because f2fs doesn't use
generic_writepages(), since f2fs_write_data_pages() is linked to
a_ops->writepages.
In do_writepages(), always f2fs_write_data_pages() is triggered instead
of generic_writepages().
Isn't it?
Thanks,
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Security Response Team
> DB7A 84C5 D3F9 7CD1 B5EB C939 D048 7860 3655 602B
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Jaegeuk Kim
Samsung
+-- On Wed, 3 Apr 2013, Jaegeuk Kim wrote --+
| I'm confusing the question because f2fs doesn't use generic_writepages(),
| since f2fs_write_data_pages() is linked to a_ops->writepages. In
| do_writepages(), always f2fs_write_data_pages() is triggered instead of
| generic_writepages(). Isn't it?
Before commit fa9150a84c, when `generic_writepages' returned 0, it did not
abort `f2fs_write_data_pages', as the proposed patch does. I was wondering if
that's intentional OR if the patch below does it right?
===
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7bd22a2..7be750e 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -561,7 +561,7 @@ static int f2fs_write_data_pages(struct address_space
*mapping,
{
struct inode *inode = mapping->host;
struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
- int ret;
+ int ret = 0;
long excess_nrtw = 0, desired_nrtw;
if (wbc->nr_to_write < MAX_DESIRED_PAGES_WP) {
@@ -572,7 +572,9 @@ static int f2fs_write_data_pages(struct address_space *mapping,
if (!S_ISDIR(inode->i_mode))
mutex_lock(&sbi->writepages);
- ret = write_cache_pages(mapping, wbc, __f2fs_writepage, mapping);
+ /* deal with chardevs and other special file */
+ if (mapping->a_ops->writepage)
+ ret = write_cache_pages(mapping, wbc, __f2fs_writepage, mapping);
if (!S_ISDIR(inode->i_mode))
mutex_unlock(&sbi->writepages);
f2fs_submit_bio(sbi, DATA, (wbc->sync_mode == WB_SYNC_ALL));
===
Thank you.
--
Prasad J Pandit / Red Hat Security Response Team
DB7A 84C5 D3F9 7CD1 B5EB C939 D048 7860 3655 602B
Hi,
2013-04-03 (수), 14:43 +0530, P J P:
> +-- On Wed, 3 Apr 2013, Jaegeuk Kim wrote --+
> | I'm confusing the question because f2fs doesn't use generic_writepages(),
> | since f2fs_write_data_pages() is linked to a_ops->writepages. In
> | do_writepages(), always f2fs_write_data_pages() is triggered instead of
> | generic_writepages(). Isn't it?
>
> Before commit fa9150a84c, when `generic_writepages' returned 0, it did not
> abort `f2fs_write_data_pages', as the proposed patch does. I was wondering if
> that's intentional OR if the patch below does it right?
>
> ===
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 7bd22a2..7be750e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -561,7 +561,7 @@ static int f2fs_write_data_pages(struct address_space
> *mapping,
> {
> struct inode *inode = mapping->host;
> struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> - int ret;
> + int ret = 0;
> long excess_nrtw = 0, desired_nrtw;
>
> if (wbc->nr_to_write < MAX_DESIRED_PAGES_WP) {
> @@ -572,7 +572,9 @@ static int f2fs_write_data_pages(struct address_space *mapping,
>
> if (!S_ISDIR(inode->i_mode))
> mutex_lock(&sbi->writepages);
> - ret = write_cache_pages(mapping, wbc, __f2fs_writepage, mapping);
> + /* deal with chardevs and other special file */
> + if (mapping->a_ops->writepage)
> + ret = write_cache_pages(mapping, wbc, __f2fs_writepage, mapping);
Why should we take unnecessary locks and an f2fs_submit_bio call?
Thanks,
> if (!S_ISDIR(inode->i_mode))
> mutex_unlock(&sbi->writepages);
> f2fs_submit_bio(sbi, DATA, (wbc->sync_mode == WB_SYNC_ALL));
> ===
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Security Response Team
> DB7A 84C5 D3F9 7CD1 B5EB C939 D048 7860 3655 602B
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Jaegeuk Kim
Samsung
+-- On Thu, 4 Apr 2013, Jaegeuk Kim wrote --+
| Why should we take unnecessary locks and an f2fs_submit_bio call?
Yep, we should not. I wasn't sure if these are unnecessary when
a_ops->writepage = NULL.
Thank you.
--
Prasad J Pandit / Red Hat Security Response Team
DB7A 84C5 D3F9 7CD1 B5EB C939 D048 7860 3655 602B