Re: [PATCH] swapfile: avoid NULL pointer dereference in swapon when s_bdev is NULL

[Suresh Jayaraman <sjayaraman@suse.de>]

Hugh Dickins wrote:
> On Tue, 29 Sep 2009, Suresh Jayaraman wrote:
> 
>> While testing Swap over NFS patchset, I noticed an oops that was triggered
>> during swapon. Investigating further, the NULL pointer deference is due to the
>> SSD device check/optimization in the swapon code that assumes s_bdev could never
>> be NULL.
>>
>> inode->i_sb->s_bdev could be NULL in a few cases. For e.g. one such case is
>> loopback NFS mount, there could be others as well. Fix this by ensuring s_bdev
>> is not NULL before we try to deference s_bdev.
>>
>> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> 
> Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> 
> Thanks a lot for that: sorry to say I was ignorant of the possibility.
> 
> This is only an issue with an out-of-tree patch, is that correct?

Yes, it was reproducible only with an out-of-tree patch.

> I'd like it to be fixed anyway, but if there's a way in which it can
> happen in unpatched 2.6.31, then we ought to send the fix to -stable.
> 
> I've added Rafael to the Cc, because CONFIG_HIBERNATION's swap_type_of()
> looks also dangerous in this respect - and especially where it does that
> "if (bdev == sis->bdev) {" match, I think it's assuming NULL bdev cannot
> match against anything.

Yeah, perhaps. I stumbled upon one more of such error - a NULL pointer
dereference in blkdev_issue_discard() called from get_swap_page() when I ran
memhog, a simple program to generate a memory hog with Swap over NFS patches.

The call sequence is add_to_swap() -> get_swap_page() ->  scan_swap_map()
-> discard_swap_cluster() -> blkdev_issue_discard().

Wrapping the code around a NULL check fixes the Oops for me.


Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 mm/swapfile.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 4de7f02..51f39cf 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -160,11 +160,13 @@ static int discard_swap(struct swap_info_struct *si)
 				continue;
 		}
 
-		err = blkdev_issue_discard(si->bdev, start_block,
-						nr_blocks, GFP_KERNEL,
-						DISCARD_FL_BARRIER);
-		if (err)
-			break;
+		if (si->bdev) {
+			err = blkdev_issue_discard(si->bdev, start_block,
+					nr_blocks, GFP_KERNEL,
+					DISCARD_FL_BARRIER);
+			if (err)
+				break;
+		}
 
 		cond_resched();
 	}
@@ -200,10 +202,12 @@ static void discard_swap_cluster(struct swap_info_struct *si,
 
 			start_block <<= PAGE_SHIFT - 9;
 			nr_blocks <<= PAGE_SHIFT - 9;
-			if (blkdev_issue_discard(si->bdev, start_block,
+			if (si->bdev) {
+				if (blkdev_issue_discard(si->bdev, start_block,
 							nr_blocks, GFP_NOIO,
 							DISCARD_FL_BARRIER))
-				break;
+					break;
+			}
 		}
 
 		lh = se->list.next;