linux-kernel - Re: ubifs: fix page_count in ->ubifs_migrate

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:   Fri, 14 Dec 2018 14:15:18 +0800
From:   zhangjun <openzhangj@...il.com>
To:     Dave Chinner <david@...morbit.com>,
        Richard Weinberger <richard@....at>
Cc:     Artem Bityutskiy <dedekind1@...il.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        kirill.shutemov@...ux.intel.com, hch@....de,
        linux-fsdevel@...r.kernel.org
Subject: Re: ubifs: fix page_count in ->ubifs_migrate_page()

On 2018/12/14 上午6:57, Dave Chinner wrote:
> On Thu, Dec 13, 2018 at 03:23:37PM +0100, Richard Weinberger wrote:
>> Hello zhangjun,
>>
>> thanks a lot for bringing this up!
>>
>> Am Mittwoch, 12. Dezember 2018, 15:13:57 CET schrieb zhangjun:
>>> Because the PagePrivate() in UBIFS is different meanings,
>>> alloc_cma() will fail when one dirty page cache located in
>>> the type of MIGRATE_CMA
>>>
>>> If not adjust the 'extra_count' for dirty page,
>>> ubifs_migrate_page() -> migrate_page_move_mapping() will
>>> always return -EAGAIN for:
>>> 	expected_count += page_has_private(page)
>>> This causes the migration to fail until the page cache is cleaned
>>>
>>> In general, PagePrivate() indicates that buff_head is already bound
>>> to this page, and at the same time page_count() will also increase.
>
> That's an invalid assumption.
>
> We should not be trying to infer what PagePrivate() means in code
> that has no business using looking at it i.e. page->private is private
> information for the owner of the page, and it's life cycle and
> intent are unknown to anyone other than the page owner.
>
> e.g. on XFS, a page cache page's page->private /might/ contain a
> struct iomap_page, or it might be NULL. Assigning a struct
> iomap_page to the page does not change the reference count on the
> page.  IOWs, the page needs to be handled exactly the same
> way by external code regardless of whether there is somethign
> attached to page->private or not.
>
> Hence it looks to me like the migration code is making invalid
> assumptions about PagePrivate inferring reference counts and so the
> migration code needs to be fixed. Requiring filesystems to work
> around invalid assumptions in the migration code is a sure recipe
> for problems with random filesystems using page->private for their
> own internal purposes....
>
> Cheers,
>
> Dave.
I agree with your main point of view, but for the buffer_head based file 
system this assumption is no problem,
and the parameters and comments from the migrate_page_move_mapping() 
function:
   * 3 for pages with a mapping and PagePrivate/PagePrivate2 set.
This assumption has been explained.
Or to accurately say that the migrate system does not currently have a 
generic function for this case.
Since you call the function implemented for buffer_head, you should 
follow its rules.