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
