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 <khand...@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