Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: kernel BUG at mm/hugetlb.c:LINE!
To:     Miklos Szeredi <miklos@szeredi.hu>
Cc:     Colin Walters <walters@verbum.org>,
        syzbot <syzbot+d6ec23007e951dadf3de@syzkaller.appspotmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm <linux-mm@kvack.org>,
        Miklos Szeredi <mszeredi@redhat.com>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        Al Viro <viro@zeniv.linux.org.uk>
References: <000000000000b4684e05a2968ca6@google.com>
 <aa7812b8-60ae-8578-40db-e71ad766b4d3@oracle.com>
 <CAJfpegtVca6H1JPW00OF-7sCwpomMCo=A2qr5K=9uGKEGjEp3w@mail.gmail.com>
 <bb232cfa-5965-42d0-88cf-46d13f7ebda3@www.fastmail.com>
 <9a56a79a-88ed-9ff4-115e-ec169cba5c0b@oracle.com>
 <CAJfpegsNVB12MQ-Jgbb-f=+i3g0Xy52miT3TmUAYL951HVQS_w@mail.gmail.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
Message-ID: <78313ae9-8596-9cbe-f648-3152660be9b3@oracle.com>
Date:   Wed, 20 May 2020 10:27:15 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <CAJfpegsNVB12MQ-Jgbb-f=+i3g0Xy52miT3TmUAYL951HVQS_w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 5/20/20 4:20 AM, Miklos Szeredi wrote:
> On Tue, May 19, 2020 at 2:35 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>
>> On 5/18/20 4:41 PM, Colin Walters wrote:
>>>
>>> On Tue, May 12, 2020, at 11:04 AM, Miklos Szeredi wrote:
>>>
>>>>> However, in this syzbot test case the 'file' is in an overlayfs filesystem
>>>>> created as follows:
>>>>>
>>>>> mkdir("./file0", 000)                   = 0
>>>>> mount(NULL, "./file0", "hugetlbfs", MS_MANDLOCK|MS_POSIXACL, NULL) = 0
>>>>> chdir("./file0")                        = 0
>>>>> mkdir("./file1", 000)                   = 0
>>>>> mkdir("./bus", 000)                     = 0
>>>>> mkdir("./file0", 000)                   = 0
>>>>> mount("\177ELF\2\1\1", "./bus", "overlay", 0, "lowerdir=./bus,workdir=./file1,u"...) = 0
>>>
>>> Is there any actual valid use case for mounting an overlayfs on top of hugetlbfs?  I can't think of one.  Why isn't the response to this to instead only allow mounting overlayfs on top of basically a set of whitelisted filesystems?
>>>
>>
>> I can not think of a use case.  I'll let Miklos comment on adding whitelist
>> capability to overlayfs.
> 
> I've not heard of overlayfs being used over hugetlbfs.
> 
> Overlayfs on tmpfs is definitely used, I guess without hugepages.
> But if we'd want to allow tmpfs _without_ hugepages but not tmpfs
> _with_ hugepages, then we can't just whitelist based on filesystem
> type, but need to look at mount options as well.  Which isn't really a
> clean solution either.
> 
>> IMO - This BUG/report revealed two issues.  First is the BUG by mmap'ing
>> a hugetlbfs file on overlayfs.  The other is that core mmap code will skip
>> any filesystem specific get_unmapped area routine if on a union/overlay.
>> My patch fixes both, but if we go with a whitelist approach and don't allow
>> hugetlbfs I think we still need to address the filesystem specific
>> get_unmapped area issue.  That is easy enough to do by adding a routine to
>> overlayfs which calls the routine for the underlying fs.
> 
> I think the two are strongly related:  get_unmapped_area() adjusts the
> address alignment, and the is_file_hugepages() call in
> ksys_mmap_pgoff() adjusts the length alignment.
> 
> Is there any other purpose for which  f_op->get_unmapped_area() is
> used by a filesystem?
> 

I am fairly confident it is all about checking limits and alignment.  The
filesystem knows if it can/should align to base or huge page size. DAX has
some interesting additional restrictions, and several 'traditional' filesystems
check if they are 'on DAX'.

In a previous e-mail, you suggested hugetlb_get_unmapped_area could do the
length adjustment in hugetlb_get_unmapped_area (generic and arch specific).
I agree, although there may be the need to add length overflow checks in
these routines (after round up) as this is done in core code now.  However,
this can be done as a separate cleanup patch.

In any case, we need to get the core mmap code to call filesystem specific
get_unmapped_area if on a union/overlay.  The patch I suggested does this
by simply calling real_file to determine if there is a filesystem specific
get_unmapped_area.  The other approach would be to provide an overlayfs
get_unmapped_area that calls the underlying filesystem get_unmapped_area.

-- 
Mike Kravetz