Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1622854 > unrolled thread

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

Started byAnshuman Khandual <khandual@linux.vnet.ibm.com>
First post2017-04-13 11:30 +0200
Last post2017-04-19 02:50 +0200
Articles 8 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/madvise: Move up the behavior parameter validation Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-04-13 11:30 +0200
    Re: [PATCH] mm/madvise: Move up the behavior parameter validation kbuild test robot <lkp@intel.com> - 2017-04-13 16:00 +0200
    [PATCH V2] mm/madvise: Move up the behavior parameter validation Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-04-14 16:00 +0200
      Re: [PATCH V2] mm/madvise: Move up the behavior parameter validation Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-04-17 07:30 +0200
        Re: [PATCH V2] mm/madvise: Move up the behavior parameter validation Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-04-18 04:50 +0200
    [PATCH V3] mm/madvise: Move up the behavior parameter validation Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-04-18 07:30 +0200
      Re: [PATCH V3] mm/madvise: Move up the behavior parameter  validation David Rientjes <rientjes@google.com> - 2017-04-18 23:10 +0200
      Re: [PATCH V3] mm/madvise: Move up the behavior parameter validation Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-04-19 02:50 +0200

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

FromAnshuman Khandual <khandual@linux.vnet.ibm.com>
Date2017-04-13 11:30 +0200
Subject[PATCH] mm/madvise: Move up the behavior parameter validation
Message-ID<tvJeG-Wr-23@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1623051

Fromkbuild test robot <lkp@intel.com>
Date2017-04-13 16:00 +0200
Message-ID<tvNrY-3KR-15@gated-at.bofh.it>
In reply to#1622854

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

FromAnshuman Khandual <khandual@linux.vnet.ibm.com>
Date2017-04-14 16:00 +0200
Subject[PATCH V2] mm/madvise: Move up the behavior parameter validation
Message-ID<tw9Vw-1JY-7@gated-at.bofh.it>
In reply to#1622854
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

[toc] | [prev] | [next] | [standalone]

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

FromNaoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date2017-04-17 07:30 +0200
SubjectRe: [PATCH V2] mm/madvise: Move up the behavior parameter validation
Message-ID<tx7oC-5as-5@gated-at.bofh.it>
In reply to#1623671
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

[toc] | [prev] | [next] | [standalone]

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

FromAnshuman Khandual <khandual@linux.vnet.ibm.com>
Date2017-04-18 04:50 +0200
SubjectRe: [PATCH V2] mm/madvise: Move up the behavior parameter validation
Message-ID<txrnj-w5-3@gated-at.bofh.it>
In reply to#1624534
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.

[toc] | [prev] | [next] | [standalone]

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

FromAnshuman Khandual <khandual@linux.vnet.ibm.com>
Date2017-04-18 07:30 +0200
Subject[PATCH V3] mm/madvise: Move up the behavior parameter validation
Message-ID<txtSa-2d0-5@gated-at.bofh.it>
In reply to#1622854
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

[toc] | [prev] | [next] | [standalone]

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

FromDavid Rientjes <rientjes@google.com>
Date2017-04-18 23:10 +0200
SubjectRe: [PATCH V3] mm/madvise: Move up the behavior parameter validation
Message-ID<txIxQ-2AU-21@gated-at.bofh.it>
In reply to#1625027
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.

[toc] | [prev] | [next] | [standalone]

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

FromNaoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date2017-04-19 02:50 +0200
SubjectRe: [PATCH V3] mm/madvise: Move up the behavior parameter validation
Message-ID<txLYK-4Jt-5@gated-at.bofh.it>
In reply to#1625027
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>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web