[PATCH] To add NULL pointer check

[PATCH] To add NULL pointer check

[P J P]

    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

Re: [PATCH] To add NULL pointer check

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

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 <ppandit@redhat.com>
Date: Wed, 3 Apr 2013 11:38:00 +0900
Subject: [PATCH] f2fs: add NULL pointer check
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net

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 <ppandit@redhat.com>
Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] To add NULL pointer check

[Namjae Jeon]

2013/4/3, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> 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 <ppandit@redhat.com>
> Date: Wed, 3 Apr 2013 11:38:00 +0900
> Subject: [PATCH] f2fs: add NULL pointer check
> Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-f2fs-devel@lists.sourceforge.net
>
> 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 <namjae.jeon@samsung.com>

Thanks.
>
> Signed-off-by: P J P <ppandit@redhat.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> ---

Re: [PATCH] To add NULL pointer check

[P J P]

  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

Re: [PATCH] To add NULL pointer check

[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'.

Thank you.
--
Prasad J Pandit / Red Hat Security Response Team
DB7A 84C5 D3F9 7CD1 B5EB  C939 D048 7860 3655 602B

Re: [PATCH] To add NULL pointer check

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] To add NULL pointer check

[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);
        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

Re: [PATCH] To add NULL pointer check

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] To add NULL pointer check

[P J P]

+-- 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