[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZqtD4idp0IyHgSp5@localhost.localdomain>
Date: Thu, 1 Aug 2024 10:14:26 +0200
From: Oscar Salvador <osalvador@...e.de>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, Peter Xu <peterx@...hat.com>,
Muchun Song <muchun.song@...ux.dev>,
David Hildenbrand <david@...hat.com>,
Donet Tom <donettom@...ux.ibm.com>,
Matthew Wilcox <willy@...radead.org>,
Vlastimil Babka <vbabka@...e.cz>, Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH v2 6/9] mm: Make hugetlb mappings go through
mm_get_unmapped_area_vmflags
On Wed, Jul 31, 2024 at 05:15:41PM +0100, Lorenzo Stoakes wrote:
> I've not got the vm debug on in my build, so it's blowing up here for me:
>
> static unsigned long shm_get_unmapped_area(struct file *file,
> unsigned long addr, unsigned long len, unsigned long pgoff,
> unsigned long flags)
> {
> struct shm_file_data *sfd = shm_file_data(file);
>
> return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
> pgoff, flags);
> }
>
> Notice that that doesn't check whether sfd->file->f_op->get_unmapped_area
> is NULL.
I see now, thanks.
> So since you remove this from the f_ops, it causes a NULL pointer deref.
...
> static const struct file_operations shm_file_operations = {
> ..
> .get_unmapped_area = shm_get_unmapped_area,
> ...
> };
>
> Then this get_area() is invoked, which calls shm_get_unmapped_area(), which
> calls f_op->get_unmapped_area() on your hugetlbfs_file_operations object
> which you just deleted and it's NULL.
>
> This is why you have to be super careful here, there's clearly stuff out
> there that assumes that this can't happen, which you need to track down.
>
> A quick grep however _suggests_ this might be the one landmine place. But
> you need to find a smart way to deal with this.
Probably, the most straightforward way to fix this is to instead of
setting .get_unmapped_area to NULL for hugetlbfs_file_operations, would
be to have it re-defined like:
.get_unmapped_area = mm_get_unmapped_area_vmflags
Which is what we call after this patchset.
So no more things have to tweaked.
On a more correct way, __maybe__ have something like:
diff --git a/ipc/shm.c b/ipc/shm.c
index 3e3071252dac..222dca8a3716 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -648,8 +648,11 @@ static unsigned long shm_get_unmapped_area(struct file *file,
{
struct shm_file_data *sfd = shm_file_data(file);
- return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
+ if (sfd->file->f_op->get_unmapped_area)
+ return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
pgoff, flags);
+
+ return mm_get_unmapped_area_vmflags(sfd->file, addr, len, pgoff, flags);
}
static const struct file_operations shm_file_operations = {
Still unsure about which approach looks more correct though.
--
Oscar Salvador
SUSE Labs
Powered by blists - more mailing lists