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 <[email protected]>
---
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
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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(¤t->mm->mmap_sem))
Thanks,
Naoya Horiguchi
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 <[email protected]>
>> ---
>> 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.
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 <[email protected]>
---
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(¤t->mm->mmap_sem))
--
1.8.5.2
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 <[email protected]>
Acked-by: David Rientjes <[email protected]>
Looks like this depends on existing patches in -mm.
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 <[email protected]>
Acked-by: Naoya Horiguchi <[email protected]>