Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1662116347-17649-1-git-send-email-zhaoyang.huang@unisoc.com>
 <20220902115839.1e3fafd159e42d4e7dae90af@linux-foundation.org> <YxJUxDn5v2MqmjKM@casper.infradead.org>
In-Reply-To: <YxJUxDn5v2MqmjKM@casper.infradead.org>
From:   Zhaoyang Huang <huangzhaoyang@gmail.com>
Date:   Tue, 6 Sep 2022 15:29:52 +0800
Message-ID: <CAGWkznHzCnjUUjceAvj_E_WCSwGP5_SoGYDeKaHL9dTeUinEBQ@mail.gmail.com>
Subject: Re: [Resend RFC PATCH] mm: introduce __GFP_TRACKLEAK to track
 in-kernel allocation
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        "zhaoyang.huang" <zhaoyang.huang@unisoc.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        "open list:MEMORY MANAGEMENT" <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>, Ke Wang <ke.wang@unisoc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Sep 3, 2022 at 3:08 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Sep 02, 2022 at 11:58:39AM -0700, Andrew Morton wrote:
> > Cc willy for page-flags changes.
>
> Thanks.  This is probably OK.  The biggest problem is that it won't
> work for drivers which allocate memory and then map it to userspace.
> If they try, they'll get a nice splat, but it may limit the usefulness
> of this option.  We should probably document that limitation in this
> patch.
>
> > On Fri, 2 Sep 2022 18:59:07 +0800 "zhaoyang.huang" <zhaoyang.huang@unisoc.com> wrote:
> > > +++ b/mm/page_alloc.c
> > > @@ -1361,6 +1361,8 @@ static __always_inline bool free_pages_prepare(struct page *page,
> > >             page->mapping = NULL;
> > >     if (memcg_kmem_enabled() && PageMemcgKmem(page))
> > >             __memcg_kmem_uncharge_page(page, order);
> > > +   if (PageTrackleak(page))
> > > +           kmemleak_free(page);
>
> Don't we also need to __ClearPageTrackleak()?
ok
>
> > > +   if (gfp & __GFP_TRACKLEAK) {
> >
> > And we'd want __GFP_TRACKLEAK to evaluate to zero at compile time if
> > CONFIG_HAVE_DEBUG_KMEMLEAK=n.
> >
> > > +           kmemleak_alloc(page_address(page), PAGE_SIZE << order, 1, gfp & ~__GFP_TRACKLEAK);
> > > +           __SetPageTrackleak(page);
> > > +   }
>
> We only set this on the first page we allocate.  I think there's a
> problem for multi-page, non-compound allocations, no?  Particularly
> when you consider the problem fixed in e320d3012d25.
Please correct me if I am wrong. AFAICT, the leak_object is created
for tracking from page_address(page) to page_address(page) + PAGE_SIZE
<< order via checksum of the whole area while not referring to
page->cnt. Non-compound high order tail pages will not be checked
anymore since the leak_object has been freed together with head page
by put_page, whereas, it would not be a problem as the tail pages
should NOT be accessed also as they should be considered as buddy
pages, no?
>
> I'm not opposed to this tracking, it just needs a bit more thought and
> awareness of some of the corner cases of the VM.  A few test cases would
> be nice; they could demonstrate that this works for both compound and
> non-compound high-order allocations.