From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754681AbaHNSyu (ORCPT ); Thu, 14 Aug 2014 14:54:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18109 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753829AbaHNSyt (ORCPT ); Thu, 14 Aug 2014 14:54:49 -0400 Date: Thu, 14 Aug 2014 15:54:21 -0300 From: Rafael Aquini To: Andrey Ryabinin Cc: Konstantin Khlebnikov , Sasha Levin , Andrew Morton , Vlastimil Babka , David Rientjes , Mel Gorman , Joonsoo Kim , Dave Jones , "linux-mm@kvack.org" , LKML , Andrey Ryabinin Subject: Re: mm: compaction: buffer overflow in isolate_migratepages_range Message-ID: <20140814185420.GA26367@optiplex.redhat.com> References: <53E6CEAA.9020105@oracle.com> <20140813153501.GE21041@optiplex.redhat.com> <20140814151329.GA22187@optiplex.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote: > 2014-08-14 19:13 GMT+04:00 Rafael Aquini : > > It still a harmless condition as before, but considering what goes above > > I'm now convinced & confident the patch proposed by Andrey is the real fix > > for such occurrences. > > > > I don't think that it's harmless, because we could cross page boundary here and > try to read from a memory hole. > I think isolate_migratepages_range() skips over holes, doesn't it? > And this code has more potential problems like use after free. Since > we don't hold locks properly here, > page->mapping could point to freed struct address_space. > Thinking on how things go for isolate_migratepages_range() and balloon pages, I struggle to find a way where that could happen. OTOH, I failed to see things more blatant before, so I won't argue here. Defensive programming is always better than negating possibilities ;) > We discussed this with Konstantin and he suggested a better solution for this. > If I understood him correctly the main idea was to store bit > identifying ballon page > in struct page (special value in _mapcount), so we won't need to check > mapping->flags. > I liked it. Something in the line of PageBuddy()/PAGE_BUDDY_MAPCOUNT_VALUE scheme. This is clearly cleaner than what we have in place today, and I'm ashamed I didn't think of it before. Thanks for pointing that out. Cheers, -- Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qa0-f46.google.com (mail-qa0-f46.google.com [209.85.216.46]) by kanga.kvack.org (Postfix) with ESMTP id F0E436B0036 for ; Thu, 14 Aug 2014 16:01:57 -0400 (EDT) Received: by mail-qa0-f46.google.com with SMTP id v10so1364449qac.33 for ; Thu, 14 Aug 2014 13:01:57 -0700 (PDT) Received: from mx1.redhat.com (mx1.redhat.com. [209.132.183.28]) by mx.google.com with ESMTPS id x8si8783690qcx.16.2014.08.14.13.01.55 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 14 Aug 2014 13:01:57 -0700 (PDT) Date: Thu, 14 Aug 2014 15:54:21 -0300 From: Rafael Aquini Subject: Re: mm: compaction: buffer overflow in isolate_migratepages_range Message-ID: <20140814185420.GA26367@optiplex.redhat.com> References: <53E6CEAA.9020105@oracle.com> <20140813153501.GE21041@optiplex.redhat.com> <20140814151329.GA22187@optiplex.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Andrey Ryabinin Cc: Konstantin Khlebnikov , Sasha Levin , Andrew Morton , Vlastimil Babka , David Rientjes , Mel Gorman , Joonsoo Kim , Dave Jones , "linux-mm@kvack.org" , LKML , Andrey Ryabinin On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote: > 2014-08-14 19:13 GMT+04:00 Rafael Aquini : > > It still a harmless condition as before, but considering what goes above > > I'm now convinced & confident the patch proposed by Andrey is the real fix > > for such occurrences. > > > > I don't think that it's harmless, because we could cross page boundary here and > try to read from a memory hole. > I think isolate_migratepages_range() skips over holes, doesn't it? > And this code has more potential problems like use after free. Since > we don't hold locks properly here, > page->mapping could point to freed struct address_space. > Thinking on how things go for isolate_migratepages_range() and balloon pages, I struggle to find a way where that could happen. OTOH, I failed to see things more blatant before, so I won't argue here. Defensive programming is always better than negating possibilities ;) > We discussed this with Konstantin and he suggested a better solution for this. > If I understood him correctly the main idea was to store bit > identifying ballon page > in struct page (special value in _mapcount), so we won't need to check > mapping->flags. > I liked it. Something in the line of PageBuddy()/PAGE_BUDDY_MAPCOUNT_VALUE scheme. This is clearly cleaner than what we have in place today, and I'm ashamed I didn't think of it before. Thanks for pointing that out. Cheers, -- Rafael -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org