[PATCH] mm/madvise: Move up the behavior parameter validation

[PATCH] mm/madvise: Move up the behavior parameter validation

[Anshuman Khandual]

The madvise_behavior_valid() function should be called before
acting upon the behavior parameter. Hence move up the function.
This also includes MADV_SOFT_OFFLINE and MADV_HWPOISON options
as valid behavior parameter for the system call madvise().

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
This applies on top of the other madvise clean up patch I sent
earlier this week https://patchwork.kernel.org/patch/9672095/

 mm/madvise.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index efd4721..3cb427a 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -694,6 +694,8 @@ static int madvise_inject_error(int behavior,
 #endif
 	case MADV_DONTDUMP:
 	case MADV_DODUMP:
+	case MADV_SOFT_OFFLINE:
+	case MADV_HWPOISON:
 		return true;
 
 	default:
@@ -767,12 +769,13 @@ static int madvise_inject_error(int behavior,
 	size_t len;
 	struct blk_plug plug;
 
+	if (!madvise_behavior_valid(behavior))
+		return error;
+
 #ifdef CONFIG_MEMORY_FAILURE
 	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
 		return madvise_inject_error(behavior, start, start + len_in);
 #endif
-	if (!madvise_behavior_valid(behavior))
-		return error;
 
 	if (start & ~PAGE_MASK)
 		return error;
-- 
1.8.5.2

Re: [PATCH] mm/madvise: Move up the behavior parameter validation

[kbuild test robot]

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

Hi Anshuman,

[auto build test ERROR on next-20170412]
[also build test ERROR on v4.11-rc6]
[cannot apply to mmotm/master v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Anshuman-Khandual/mm-madvise-Move-up-the-behavior-parameter-validation/20170413-203541
config: parisc-c3000_defconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=parisc 

All errors (new ones prefixed by >>):

   mm/madvise.c: In function 'madvise_behavior_valid':
>> mm/madvise.c:690:7: error: 'MADV_SOFT_OFFLINE' undeclared (first use in this function)
     case MADV_SOFT_OFFLINE:
          ^~~~~~~~~~~~~~~~~
   mm/madvise.c:690:7: note: each undeclared identifier is reported only once for each function it appears in
>> mm/madvise.c:691:7: error: 'MADV_HWPOISON' undeclared (first use in this function)
     case MADV_HWPOISON:
          ^~~~~~~~~~~~~

vim +/MADV_SOFT_OFFLINE +690 mm/madvise.c

   684	#ifdef CONFIG_TRANSPARENT_HUGEPAGE
   685		case MADV_HUGEPAGE:
   686		case MADV_NOHUGEPAGE:
   687	#endif
   688		case MADV_DONTDUMP:
   689		case MADV_DODUMP:
 > 690		case MADV_SOFT_OFFLINE:
 > 691		case MADV_HWPOISON:
   692			return true;
   693	
   694		default:

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14651 bytes --]

[PATCH V2] mm/madvise: Move up the behavior parameter validation

[Anshuman Khandual]

The madvise_behavior_valid() function should be called before
acting upon the behavior parameter. Hence move up the function.
This also includes MADV_SOFT_OFFLINE and MADV_HWPOISON options
as valid behavior parameter for the system call madvise().

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
Changes in V2:

Added CONFIG_MEMORY_FAILURE check before using MADV_SOFT_OFFLINE
and MADV_HWPOISONE constants.

 mm/madvise.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index efd4721..ccff186 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -694,6 +694,10 @@ static int madvise_inject_error(int behavior,
 #endif
 	case MADV_DONTDUMP:
 	case MADV_DODUMP:
+#ifdef CONFIG_MEMORY_FAILURE
+	case MADV_SOFT_OFFLINE:
+	case MADV_HWPOISON:
+#endif
 		return true;
 
 	default:
@@ -767,12 +771,13 @@ static int madvise_inject_error(int behavior,
 	size_t len;
 	struct blk_plug plug;
 
+	if (!madvise_behavior_valid(behavior))
+		return error;
+
 #ifdef CONFIG_MEMORY_FAILURE
 	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
 		return madvise_inject_error(behavior, start, start + len_in);
 #endif
-	if (!madvise_behavior_valid(behavior))
-		return error;
 
 	if (start & ~PAGE_MASK)
 		return error;
-- 
1.8.5.2

Re: [PATCH V2] mm/madvise: Move up the behavior parameter validation

[Naoya Horiguchi]

On Fri, Apr 14, 2017 at 07:21:41PM +0530, Anshuman Khandual wrote:
> The madvise_behavior_valid() function should be called before
> acting upon the behavior parameter. Hence move up the function.
> This also includes MADV_SOFT_OFFLINE and MADV_HWPOISON options
> as valid behavior parameter for the system call madvise().
> 
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> ---
> Changes in V2:
> 
> Added CONFIG_MEMORY_FAILURE check before using MADV_SOFT_OFFLINE
> and MADV_HWPOISONE constants.
> 
>  mm/madvise.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index efd4721..ccff186 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -694,6 +694,10 @@ static int madvise_inject_error(int behavior,
>  #endif
>  	case MADV_DONTDUMP:
>  	case MADV_DODUMP:
> +#ifdef CONFIG_MEMORY_FAILURE
> +	case MADV_SOFT_OFFLINE:
> +	case MADV_HWPOISON:
> +#endif
>  		return true;
>  
>  	default:
> @@ -767,12 +771,13 @@ static int madvise_inject_error(int behavior,
>  	size_t len;
>  	struct blk_plug plug;
>  
> +	if (!madvise_behavior_valid(behavior))
> +		return error;
> +
>  #ifdef CONFIG_MEMORY_FAILURE
>  	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
>  		return madvise_inject_error(behavior, start, start + len_in);
>  #endif
> -	if (!madvise_behavior_valid(behavior))
> -		return error;

Hi Anshuman,

I'm wondering why current code calls madvise_inject_error() at the beginning
of SYSCALL_DEFINE3(madvise), without any boundary checks of address or length.
I agree to checking madvise_behavior_valid for MADV_{HWPOISON,SOFT_OFFLINE},
but checking boundary of other arguments is also helpful, so how about moving
down the existing #ifdef block like below?

diff --git a/mm/madvise.c b/mm/madvise.c
index a09d2d3dfae9..7b36912e1f4a 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -754,10 +754,6 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 	size_t len;
 	struct blk_plug plug;
 
-#ifdef CONFIG_MEMORY_FAILURE
-	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
-		return madvise_inject_error(behavior, start, start+len_in);
-#endif
 	if (!madvise_behavior_valid(behavior))
 		return error;
 
@@ -777,6 +773,11 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 	if (end == start)
 		return error;
 
+#ifdef CONFIG_MEMORY_FAILURE
+	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
+		return madvise_inject_error(behavior, start, start+len_in);
+#endif
+
 	write = madvise_need_mmap_write(behavior);
 	if (write) {
 		if (down_write_killable(&current->mm->mmap_sem))


Thanks,
Naoya Horiguchi

Re: [PATCH V2] mm/madvise: Move up the behavior parameter validation

[Anshuman Khandual]

On 04/17/2017 10:57 AM, Naoya Horiguchi wrote:
> On Fri, Apr 14, 2017 at 07:21:41PM +0530, Anshuman Khandual wrote:
>> The madvise_behavior_valid() function should be called before
>> acting upon the behavior parameter. Hence move up the function.
>> This also includes MADV_SOFT_OFFLINE and MADV_HWPOISON options
>> as valid behavior parameter for the system call madvise().
>>
>> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>> ---
>> Changes in V2:
>>
>> Added CONFIG_MEMORY_FAILURE check before using MADV_SOFT_OFFLINE
>> and MADV_HWPOISONE constants.
>>
>>  mm/madvise.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index efd4721..ccff186 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -694,6 +694,10 @@ static int madvise_inject_error(int behavior,
>>  #endif
>>  	case MADV_DONTDUMP:
>>  	case MADV_DODUMP:
>> +#ifdef CONFIG_MEMORY_FAILURE
>> +	case MADV_SOFT_OFFLINE:
>> +	case MADV_HWPOISON:
>> +#endif
>>  		return true;
>>  
>>  	default:
>> @@ -767,12 +771,13 @@ static int madvise_inject_error(int behavior,
>>  	size_t len;
>>  	struct blk_plug plug;
>>  
>> +	if (!madvise_behavior_valid(behavior))
>> +		return error;
>> +
>>  #ifdef CONFIG_MEMORY_FAILURE
>>  	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
>>  		return madvise_inject_error(behavior, start, start + len_in);
>>  #endif
>> -	if (!madvise_behavior_valid(behavior))
>> -		return error;
> 
> Hi Anshuman,
> 
> I'm wondering why current code calls madvise_inject_error() at the beginning
> of SYSCALL_DEFINE3(madvise), without any boundary checks of address or length.
> I agree to checking madvise_behavior_valid for MADV_{HWPOISON,SOFT_OFFLINE},
> but checking boundary of other arguments is also helpful, so how about moving
> down the existing #ifdef block like below?

Sure, will fold both the patches together and send it out.

[PATCH V3] mm/madvise: Move up the behavior parameter validation

[Anshuman Khandual]

The madvise_behavior_valid() function should be called before
acting upon the behavior parameter. Hence move up the function.
This also includes MADV_SOFT_OFFLINE and MADV_HWPOISON options
as valid behavior parameter for the system call madvise().

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
Changes in V3:

Moved the madvise_inject_error() function down which will make
sure that the boundary conditions are checked for address and
length arguments as per Naoya.

Changes in V2:

Added CONFIG_MEMORY_FAILURE check before using MADV_SOFT_OFFLINE
and MADV_HWPOISONE constants.

 mm/madvise.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index efd4721..721dd6f 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -694,6 +694,10 @@ static int madvise_inject_error(int behavior,
 #endif
 	case MADV_DONTDUMP:
 	case MADV_DODUMP:
+#ifdef CONFIG_MEMORY_FAILURE
+	case MADV_SOFT_OFFLINE:
+	case MADV_HWPOISON:
+#endif
 		return true;
 
 	default:
@@ -767,10 +771,6 @@ static int madvise_inject_error(int behavior,
 	size_t len;
 	struct blk_plug plug;
 
-#ifdef CONFIG_MEMORY_FAILURE
-	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
-		return madvise_inject_error(behavior, start, start + len_in);
-#endif
 	if (!madvise_behavior_valid(behavior))
 		return error;
 
@@ -790,6 +790,11 @@ static int madvise_inject_error(int behavior,
 	if (end == start)
 		return error;
 
+#ifdef CONFIG_MEMORY_FAILURE
+	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
+		return madvise_inject_error(behavior, start, start + len_in);
+#endif
+
 	write = madvise_need_mmap_write(behavior);
 	if (write) {
 		if (down_write_killable(&current->mm->mmap_sem))
-- 
1.8.5.2

Re: [PATCH V3] mm/madvise: Move up the behavior parameter validation

[David Rientjes]

On Tue, 18 Apr 2017, Anshuman Khandual wrote:

> The madvise_behavior_valid() function should be called before
> acting upon the behavior parameter. Hence move up the function.
> This also includes MADV_SOFT_OFFLINE and MADV_HWPOISON options
> as valid behavior parameter for the system call madvise().
> 
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>

Acked-by: David Rientjes <rientjes@google.com>

Looks like this depends on existing patches in -mm.

Re: [PATCH V3] mm/madvise: Move up the behavior parameter validation

[Naoya Horiguchi]

On Tue, Apr 18, 2017 at 10:58:44AM +0530, Anshuman Khandual wrote:
> The madvise_behavior_valid() function should be called before
> acting upon the behavior parameter. Hence move up the function.
> This also includes MADV_SOFT_OFFLINE and MADV_HWPOISON options
> as valid behavior parameter for the system call madvise().
> 
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>

Acked-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>