| Message ID | 20241206152032.1222067-1-bgeffon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mremap: Fix newaddr hint with MREMAP_DONTUNMAP | expand |
+mmap maintainers (maybe mm/mremap.c should be added to the file pattern for "MEMORY MAPPING" in "MAINTAINERS"? I'm not sure) On Fri, Dec 6, 2024 at 4:20 PM Brian Geffon <bgeffon@google.com> wrote: > mmap(2) allows for a destination address to be specified without > MAP_FIXED and in this situation it's a hint to get_unmapped_area(). > This address need not be page aligned because get_unmapped_area() will > align the hint. > > In the case of mremap(2) with MREMAP_DONTUNMAP it shares a code path > with MREMAP_FIXED in mremap_to(), which means this function can be > called in 3 different scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP > only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only > MREMAP_DONTUNMAP is specified we don't need to do alignment or size > checks on newaddr because they will be passed to get_unmapped_area() and > dealt with appropriately. > > This patch corrects that behavior to match what non-MREMAP_DONTUNMAP > mremap(2) and mmap(2) do. This odd behavioral difference was reported by > Marco Vanotti. Additionally, I've included a self test to validate this > behavior. Marco pointed me to this; I had no idea mremap() had this undocumented behavior where it takes a hint address. The mremap() manpage is currently wrong about this, it sort of implies that the new_address argument is only used if MREMAP_FIXED is set. Marco also noticed that upstream glibc now assumes this behavior: https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206 Debian also has a test that explicitly checks for this behavior: https://sources.debian.org/src/glibc/2.40-4/debian/patches/git-updates.diff/?hl=22820#L22818 I guess it's too late to remove that behavior at this point, and the right thing to do is to update the manpage?
On Fri, Dec 06, 2024 at 07:42:51PM +0100, Jann Horn wrote: > +mmap maintainers (maybe mm/mremap.c should be added to the file > pattern for "MEMORY MAPPING" in "MAINTAINERS"? I'm not sure) Yeah I think it's actually right to group together _all_ VMA-related operations under the VMA entry, because we have interaction between them all mprotect, mlock, etc. etc. etc. I will send a patch in a second for this, because we do keep getting bitten by this. > > On Fri, Dec 6, 2024 at 4:20 PM Brian Geffon <bgeffon@google.com> wrote: > > mmap(2) allows for a destination address to be specified without > > MAP_FIXED and in this situation it's a hint to get_unmapped_area(). > > This address need not be page aligned because get_unmapped_area() will > > align the hint. > > > > In the case of mremap(2) with MREMAP_DONTUNMAP it shares a code path > > with MREMAP_FIXED in mremap_to(), which means this function can be > > called in 3 different scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP > > only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only > > MREMAP_DONTUNMAP is specified we don't need to do alignment or size > > checks on newaddr because they will be passed to get_unmapped_area() and > > dealt with appropriately. > > > > This patch corrects that behavior to match what non-MREMAP_DONTUNMAP > > mremap(2) and mmap(2) do. This odd behavioral difference was reported by > > Marco Vanotti. Additionally, I've included a self test to validate this > > behavior. Yeah if this is user-facing - I don't think we can change this. Can we do any v2 as an RFC for now until we can get a chance to look at this? And please cc- the VMA/mmap maintainers on future revisions (sorry this wasn't at all clear, we need to update MAINTAINERS here). Thanks! > > Marco pointed me to this; I had no idea mremap() had this undocumented > behavior where it takes a hint address. The mremap() manpage is > currently wrong about this, it sort of implies that the new_address > argument is only used if MREMAP_FIXED is set. > > Marco also noticed that upstream glibc now assumes this behavior: > https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206 > > Debian also has a test that explicitly checks for this behavior: > https://sources.debian.org/src/glibc/2.40-4/debian/patches/git-updates.diff/?hl=22820#L22818 > > I guess it's too late to remove that behavior at this point, and the > right thing to do is to update the manpage? Yeah, if user-facing we can't fundamentally change behaviour even if it's strange I'd say.
On Fri, Dec 6, 2024 at 10:52 AM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Fri, Dec 06, 2024 at 07:42:51PM +0100, Jann Horn wrote: > > +mmap maintainers (maybe mm/mremap.c should be added to the file > > pattern for "MEMORY MAPPING" in "MAINTAINERS"? I'm not sure) > > Yeah I think it's actually right to group together _all_ VMA-related operations > under the VMA entry, because we have interaction between them all mprotect, > mlock, etc. etc. etc. > > I will send a patch in a second for this, because we do keep getting bitten by > this. > > > > > On Fri, Dec 6, 2024 at 4:20 PM Brian Geffon <bgeffon@google.com> wrote: > > > mmap(2) allows for a destination address to be specified without > > > MAP_FIXED and in this situation it's a hint to get_unmapped_area(). > > > This address need not be page aligned because get_unmapped_area() will > > > align the hint. > > > > > > In the case of mremap(2) with MREMAP_DONTUNMAP it shares a code path > > > with MREMAP_FIXED in mremap_to(), which means this function can be > > > called in 3 different scenarios: MREMAP_FIXED only, MREMAP_DONTUNMAP > > > only, or MREMAP_FIXED | MREMAP_DONTUNMAP. In the second case when only > > > MREMAP_DONTUNMAP is specified we don't need to do alignment or size > > > checks on newaddr because they will be passed to get_unmapped_area() and > > > dealt with appropriately. > > > > > > This patch corrects that behavior to match what non-MREMAP_DONTUNMAP > > > mremap(2) and mmap(2) do. This odd behavioral difference was reported by > > > Marco Vanotti. Additionally, I've included a self test to validate this > > > behavior. > > Yeah if this is user-facing - I don't think we can change this. Can we do any v2 > as an RFC for now until we can get a chance to look at this? And please cc- the > VMA/mmap maintainers on future revisions (sorry this wasn't at all clear, we > need to update MAINTAINERS here). Sure, I'll mail the next series as an RFC in the next few days. This behavior was not introduced intentionally. > > Thanks! > > > > > Marco pointed me to this; I had no idea mremap() had this undocumented > > behavior where it takes a hint address. The mremap() manpage is > > currently wrong about this, it sort of implies that the new_address > > argument is only used if MREMAP_FIXED is set. > > > > Marco also noticed that upstream glibc now assumes this behavior: > > https://sourceware.org/git/?p=glibc.git;a=commit;h=6c40cb0e9f893d49dc7caee580a055de53562206 > > > > Debian also has a test that explicitly checks for this behavior: > > https://sources.debian.org/src/glibc/2.40-4/debian/patches/git-updates.diff/?hl=22820#L22818 > > > > I guess it's too late to remove that behavior at this point, and the > > right thing to do is to update the manpage? > > Yeah, if user-facing we can't fundamentally change behaviour even if it's > strange I'd say. Definitely, no matter what happens we'll need a man page update. I think to make things consistent we'll probably want to consider allowing all variants of mremap(2) (without MREMAP_FIXED) to use newaddr as a hint, like mmap(2). But I'll mail the RFC with much more detail in the cover letter about the history and impact. Brian