Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932671AbdDQF2L convert rfc822-to-8bit (ORCPT ); Mon, 17 Apr 2017 01:28:11 -0400 Received: from tyo161.gate.nec.co.jp ([114.179.232.161]:57236 "EHLO tyo161.gate.nec.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932370AbdDQF2I (ORCPT ); Mon, 17 Apr 2017 01:28:08 -0400 From: Naoya Horiguchi To: Anshuman Khandual CC: "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "akpm@linux-foundation.org" Subject: Re: [PATCH V2] mm/madvise: Move up the behavior parameter validation Thread-Topic: [PATCH V2] mm/madvise: Move up the behavior parameter validation Thread-Index: AQHStSZptrTsavh71kOVjG3wDgArWqHIdXAA Date: Mon, 17 Apr 2017 05:27:30 +0000 Message-ID: <20170417052729.GA23423@hori1.linux.bs1.fc.nec.co.jp> References: <20170413092008.5437-1-khandual@linux.vnet.ibm.com> <20170414135141.15340-1-khandual@linux.vnet.ibm.com> In-Reply-To: <20170414135141.15340-1-khandual@linux.vnet.ibm.com> Accept-Language: en-US, ja-JP Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.128.101.19] Content-Type: text/plain; charset="iso-2022-jp" Content-ID: Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2695 Lines: 84 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 > --- > 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