ceph: Check PagePrivate(page) before dereference, page->private

ceph: Check PagePrivate(page) before dereference, page->private

[Yan, Zheng]

I got lots of NULL pointer dereference Oops when compiling kernel on ceph.
The bug is because the kernel page migration routine replaces some pages
in the page cache with new pages, these new pages' private can be non-zero.

Signed-off-by: Zheng Yan <zheng.z.yan@intel.com>
---
 fs/ceph/addr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 173b1d2..1f180b1 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -984,7 +984,10 @@ retry_locked:
 	BUG_ON(!ci->i_snap_realm);
 	down_read(&mdsc->snap_rwsem);
 	BUG_ON(!ci->i_snap_realm->cached_context);
-	snapc = (void *)page->private;
+	if (PagePrivate(page))
+		snapc = (void *)page->private;
+	else
+		snapc = NULL;
 	if (snapc && snapc != ci->i_head_snapc) {
 		/*
 		 * this page is already dirty in another (older) snap
-- 
1.7.6.5

Re: ceph: Check PagePrivate(page) before dereference, page->private

[Sage Weil]

Hi Yan,

On Fri, 25 May 2012, Yan, Zheng wrote:
> I got lots of NULL pointer dereference Oops when compiling kernel on ceph.
> The bug is because the kernel page migration routine replaces some pages
> in the page cache with new pages, these new pages' private can be non-zero.
> 
> Signed-off-by: Zheng Yan <zheng.z.yan@intel.com>
> ---
>  fs/ceph/addr.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 173b1d2..1f180b1 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -984,7 +984,10 @@ retry_locked:
>  	BUG_ON(!ci->i_snap_realm);
>  	down_read(&mdsc->snap_rwsem);
>  	BUG_ON(!ci->i_snap_realm->cached_context);
> -	snapc = (void *)page->private;
> +	if (PagePrivate(page))
> +		snapc = (void *)page->private;
> +	else
> +		snapc = NULL;
>  	if (snapc && snapc != ci->i_head_snapc) {
>  		/*
>  		 * this page is already dirty in another (older) snap
> -- 

This looks right for this case.  What I'm less sure about is whether it's 
sufficient.  I take it the migration only happens on clean pages?  (Unless 
->migrate is implemented?)

Before we were assuming private == NULL on any clean page, independent of 
PG_private.  For example, ceph_invalidatepage() and ceph_releasepage() 
have BUG_ONs that are probably wrong.  writepage_nounlock() has a sanity 
check that should probably be strengthened/guarded with a PagePrivate() 
check, too.

sage

Re: ceph: Check PagePrivate(page) before dereference, page->private

[Yan, Zheng]

On 05/26/2012 12:24 AM, Sage Weil wrote:
> Hi Yan,
> 
> On Fri, 25 May 2012, Yan, Zheng wrote:
>> I got lots of NULL pointer dereference Oops when compiling kernel on ceph.
>> The bug is because the kernel page migration routine replaces some pages
>> in the page cache with new pages, these new pages' private can be non-zero.
>>
>> Signed-off-by: Zheng Yan <zheng.z.yan@intel.com>
>> ---
>>  fs/ceph/addr.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
>> index 173b1d2..1f180b1 100644
>> --- a/fs/ceph/addr.c
>> +++ b/fs/ceph/addr.c
>> @@ -984,7 +984,10 @@ retry_locked:
>>  	BUG_ON(!ci->i_snap_realm);
>>  	down_read(&mdsc->snap_rwsem);
>>  	BUG_ON(!ci->i_snap_realm->cached_context);
>> -	snapc = (void *)page->private;
>> +	if (PagePrivate(page))
>> +		snapc = (void *)page->private;
>> +	else
>> +		snapc = NULL;
>>  	if (snapc && snapc != ci->i_head_snapc) {
>>  		/*
>>  		 * this page is already dirty in another (older) snap
>> -- 
> 
> This looks right for this case.  What I'm less sure about is whether it's 
> sufficient.  I take it the migration only happens on clean pages?  (Unless 
> ->migrate is implemented?)
Yes, migration only happens on pages that are clean and have no private
stuff.

> 
> Before we were assuming private == NULL on any clean page, independent of 
> PG_private.  For example, ceph_invalidatepage() and ceph_releasepage() 
> have BUG_ONs that are probably wrong.  writepage_nounlock() has a sanity 
> check that should probably be strengthened/guarded with a PagePrivate() 
> check, too.
>
Will send the v2 patch soon.

Regards
Yan, Zheng