As of commit ca57f02295f, brk() can expand ordinary file mappings (but
not file mappings with weird flags), and I think it does it with
insufficient locks. I think brk() probably needs some extra checks to
make sure it's operating on a brk-like VMA (which means it should at
least be anonymous, and perhaps pass the full can_vma_merge_after()
check so that we're not creating unnecessary special cases?).
user@vm:~/brk_stretch$ cat brk_file.c
#define _GNU_SOURCE
#include <unistd.h>
#include <stdio.h>
#include <fcntl.h>
#include <err.h>
#include <stdlib.h>
#include <malloc.h>
#include <sys/auxv.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#define SYSCHK(x) ({ \
typeof(x) __res = (x); \
if (__res == (typeof(x))-1) \
err(1, "SYSCHK(" #x ")"); \
__res; \
})
int main(void) {
mallopt(M_MMAP_THRESHOLD, 0);
void *brk_space = sbrk(0x2000);
if (brk_space == NULL)
errx(1, "sbrk() fail");
printf("brk_space = %p\n", brk_space);
int fd = SYSCHK(open("/etc/services", O_RDONLY));
void *map = SYSCHK(mmap(brk_space, 0x2000, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED, fd, 0));
/* stretch */
if (sbrk(0x111000) == NULL)
err(1, "sbrk");
printf("sbrk() success\n");
system("cat /proc/$PPID/maps");
return 0;
}
user@vm:~/brk_stretch$ gcc -o brk_file brk_file.c
user@vm:~/brk_stretch$ ./brk_file
brk_space = 0x557f71b5d000
sbrk() success
557f70616000-557f70617000 r--p 00000000 fd:00 2752938
/home/user/brk_stretch/brk_file
557f70617000-557f70618000 r-xp 00001000 fd:00 2752938
/home/user/brk_stretch/brk_file
557f70618000-557f70619000 r--p 00002000 fd:00 2752938
/home/user/brk_stretch/brk_file
557f70619000-557f7061a000 r--p 00002000 fd:00 2752938
/home/user/brk_stretch/brk_file
557f7061a000-557f7061b000 rw-p 00003000 fd:00 2752938
/home/user/brk_stretch/brk_file
557f71b5d000-557f71c70000 rw-p 00000000 fd:00 2621496
/etc/services
7fd67993d000-7fd67995f000 r--p 00000000 fd:00 527268
/usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd67995f000-7fd679aa6000 r-xp 00022000 fd:00 527268
/usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679aa6000-7fd679af2000 r--p 00169000 fd:00 527268
/usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679af2000-7fd679af3000 ---p 001b5000 fd:00 527268
/usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679af3000-7fd679af7000 r--p 001b5000 fd:00 527268
/usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679af7000-7fd679af9000 rw-p 001b9000 fd:00 527268
/usr/lib/x86_64-linux-gnu/libc-2.28.so
7fd679af9000-7fd679aff000 rw-p 00000000 00:00 0
7fd679b16000-7fd679b18000 rw-p 00000000 00:00 0
7fd679b18000-7fd679b19000 r--p 00000000 fd:00 527258
/usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b19000-7fd679b37000 r-xp 00001000 fd:00 527258
/usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b37000-7fd679b3f000 r--p 0001f000 fd:00 527258
/usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b3f000-7fd679b40000 r--p 00026000 fd:00 527258
/usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b40000-7fd679b41000 rw-p 00027000 fd:00 527258
/usr/lib/x86_64-linux-gnu/ld-2.28.so
7fd679b41000-7fd679b42000 rw-p 00000000 00:00 0
7ffd58087000-7ffd580a8000 rw-p 00000000 00:00 0 [stack]
7ffd581fa000-7ffd581fe000 r--p 00000000 00:00 0 [vvar]
7ffd581fe000-7ffd58200000 r-xp 00000000 00:00 0 [vdso]
user@vm:~/brk_stretch$
The codepaths that are intended to expand file VMAs do stuff like
i_mmap_lock_write() and vma_interval_tree_remove(), which
do_brk_flags() doesn't seem to do (because it was never intended to
operate on file VMAs?).
On Fri, Dec 2, 2022 at 7:53 PM Jann Horn <[email protected]> wrote:
> As of commit ca57f02295f, brk() can expand ordinary file mappings (but
Sorry, that was worded confusingly - I meant "ca57f02295f is the
commit from Linus' tree on top of which I was testing".
The broken code seems to have been introduced in
commit 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand
existing VMA and add do_brk_munmap()").
* Jann Horn <[email protected]> [221202 13:54]:
> As of commit ca57f02295f, brk() can expand ordinary file mappings (but
> not file mappings with weird flags), and I think it does it with
> insufficient locks. I think brk() probably needs some extra checks to
> make sure it's operating on a brk-like VMA (which means it should at
> least be anonymous, and perhaps pass the full can_vma_merge_after()
> check so that we're not creating unnecessary special cases?).
Thanks. This is probably caused by commit 2e7ce7d354f2: "mm/mmap:
change do_brk_flags() to expand existing VMA and add do_brk_munmap()"
Specifically the checks around expanding the VMA.
> user@vm:~/brk_stretch$ cat brk_file.c
Thanks for the testcase. I have a fix that I'm testing, but it's worth
noting that the brk call will succeed - except a new VMA will be
created. Is this what you expect?
...
>
> The codepaths that are intended to expand file VMAs do stuff like
> i_mmap_lock_write() and vma_interval_tree_remove(), which
> do_brk_flags() doesn't seem to do (because it was never intended to
> operate on file VMAs?).
I don't think the locks were there before and I think you are correct
that it wasn't intended to operate on file VMAs. I made sure all the
ltp tests around brk pass with my changes but this seems insufficient.
On Mon, Dec 5, 2022 at 3:50 PM Liam Howlett <[email protected]> wrote:
> * Jann Horn <[email protected]> [221202 13:54]:
> > As of commit ca57f02295f, brk() can expand ordinary file mappings (but
> > not file mappings with weird flags), and I think it does it with
> > insufficient locks. I think brk() probably needs some extra checks to
> > make sure it's operating on a brk-like VMA (which means it should at
> > least be anonymous, and perhaps pass the full can_vma_merge_after()
> > check so that we're not creating unnecessary special cases?).
>
>
> Thanks. This is probably caused by commit 2e7ce7d354f2: "mm/mmap:
> change do_brk_flags() to expand existing VMA and add do_brk_munmap()"
Yeah.
> Specifically the checks around expanding the VMA.
>
> > user@vm:~/brk_stretch$ cat brk_file.c
>
> Thanks for the testcase. I have a fix that I'm testing, but it's worth
> noting that the brk call will succeed - except a new VMA will be
> created. Is this what you expect?
Yes, that's what I would expect.