linux-kernel - Re: [PATCH] aoe: adjust ref of head for compound page tails

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 7 Aug 2013 20:50:09 -0400
From:	Ed Cashin <ecashin@...aid.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] aoe: adjust ref of head for compound page tails

On Aug 7, 2013, at 8:05 PM, Andrew Morton wrote:

> On Wed, 7 Aug 2013 19:54:45 -0400 Ed Cashin <ecashin@...aid.com> wrote:
> 
>>> +bio_pageinc(struct bio *bio)
>>> +{
>>> +       struct bio_vec *bv;
>>> +       struct page *page;
>>> +       int i;
>>> +
>>> +       bio_for_each_segment(bv, bio, i) {
>>> +               page = bv->bv_page;
>>> +               /* Non-zero page count for non-head members of
>>> +                * compound pages is no longer allowed by the kernel,
>>> +                * but this has never been seen here.
>>> +                */
>>> +               if (unlikely(PageCompound(page)))
>>> +                       if (compound_trans_head(page) != page) {
>>> +                               pr_crit("page tail used for block I/O\n");
>>> +                               BUG();
>>> +                       }
>>> 
>>> But get_page/put_page against a tail page of a compound page should
>>> Just Work.  The core MM will hunt down the head page and manipulate its
>>> refcount.  
>>> 
>>> Perhaps the problem here is that AOE is diving under the covers and is
>>> using low-level page refcount alterations:
>>> 
>>> +               atomic_inc(&page->_count);
>>> 
>>> Why does AOE do this?  It would be better if it were to use the
>>> official published MM interfaces.  If those interfaces need
>>> alteration/augmentation then we can do that.
>> 
>> The get_page cannot be used when the _count is 0 (which is exactly when we need to increment it), because it will VM_BUG_ON.
>> 
>> 	/*
>> 	 * Getting a normal page or the head of a compound page
>> 	 * requires to already have an elevated page->_count.
>> 	 */
>> 	VM_BUG_ON(atomic_read(&page->_count) <= 0);
>> 
> 
> But we shouldn't get that far:
> 
> static inline void get_page(struct page *page)
> {
> 	if (unlikely(PageTail(page)))
> 		if (likely(__get_page_tail(page)))
> 			return;
> 	/*
> 	 * Getting a normal page or the head of a compound page
> 	 * requires to already have an elevated page->_count.
> 	 */
> 	VM_BUG_ON(atomic_read(&page->_count) <= 0);
> 	atomic_inc(&page->_count);
> }
> 
> This is a tail page, so we should be using __get_page_tail().


When the workaround was created, it was with the assumption that the zero-count pages are not always tail pages, and that seemed to be the case in 2007, but as I said, I don't have a mechanism for detecting that now, so I cannot say whether it really happens with today's kernel.

If it is never correct for normal pages or compound page heads to have a zero count while they are associated with a bio, then yes, I think get_page is a great solution.  The VM_BUG_ON in get_page would identify any parts of the kernel that are supplying bios that have pages without references.

Just a note in response to "we shouldn't get that far":  I believe the VM_BUG_ON line in get_page does get executed for tail pages when the __get_page_tail returns false because the compound page head has a zero count:

  get_page -> __get_page_tail -> get_page_unless_zero returns false, so
    __get_page_tail returns "got", which is still false, so
      get_page executes the VM_BUG_ON, where the _count will be zero for the tail page.

-- 
  Ed Cashin
  ecashin@...aid.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/