tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++ 1 file changed, 76 insertions(+)
From: Jeff Xu <jeffxu@google.com>
It appears there is a regression on the latest mm,
when munmap sealed memory, it can cause unexpected VMA split.
E.g. repro use this test.
---
tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
index fa74dbe4a684..0af33e13b606 100644
--- a/tools/testing/selftests/mm/mseal_test.c
+++ b/tools/testing/selftests/mm/mseal_test.c
@@ -1969,6 +1969,79 @@ static void test_madvise_filebacked_was_writable(bool seal)
REPORT_TEST_PASS();
}
+static void test_munmap_free_multiple_ranges_with_split(bool seal)
+{
+ void *ptr;
+ unsigned long page_size = getpagesize();
+ unsigned long size = 12 * page_size;
+ int ret;
+ int prot;
+
+ setup_single_address(size, &ptr);
+ FAIL_TEST_IF_FALSE(ptr != (void *)-1);
+
+ /* seal the middle 4 page */
+ if (seal) {
+ ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
+ FAIL_TEST_IF_FALSE(!ret);
+
+ size = get_vma_size(ptr, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+
+ size = get_vma_size(ptr + 4 * page_size, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+
+ size = get_vma_size(ptr + 8 * page_size, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+ }
+
+ /* munmap 4 pages from the third page */
+ ret = sys_munmap(ptr + 2 * page_size, 4 * page_size);
+ if (seal) {
+ FAIL_TEST_IF_FALSE(ret);
+ FAIL_TEST_IF_FALSE(errno == EPERM);
+
+ size = get_vma_size(ptr, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+
+ size = get_vma_size(ptr + 4 * page_size, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+
+ size = get_vma_size(ptr + 8 * page_size, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+ } else
+ FAIL_TEST_IF_FALSE(!ret);
+
+ /* munmap 4 pages from the sealed page */
+ ret = sys_munmap(ptr + 6 * page_size, 4 * page_size);
+ if (seal) {
+ FAIL_TEST_IF_FALSE(ret);
+ FAIL_TEST_IF_FALSE(errno == EPERM);
+
+ size = get_vma_size(ptr + 4 * page_size, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+
+ size = get_vma_size(ptr + 4 * page_size, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+
+ size = get_vma_size(ptr + 8 * page_size, &prot);
+ FAIL_TEST_IF_FALSE(size == 4 * page_size);
+ FAIL_TEST_IF_FALSE(prot == 4);
+ } else
+ FAIL_TEST_IF_FALSE(!ret);
+
+ REPORT_TEST_PASS();
+}
+
+
int main(int argc, char **argv)
{
bool test_seal = seal_support();
@@ -2099,5 +2172,8 @@ int main(int argc, char **argv)
test_madvise_filebacked_was_writable(false);
test_madvise_filebacked_was_writable(true);
+ test_munmap_free_multiple_ranges_with_split(false);
+ test_munmap_free_multiple_ranges_with_split(true);
+
ksft_finished();
}
--
2.47.0.rc1.288.g06298d1525-googAnother thing about etiquette - sending a barely coherent _failing_ test
with basically zero explanation as a... patch is NOT how to interact with
the upstream community.
The sensible, respectful and workable way of doing this is to send
something like a [DISCUSSION] or something and say 'hey guys I think I
found a bug' with an explanation and a test patch attached.
A lot of your problems with the community could be resolved by being more
polite, respectful and taking a step back and breathing and _communicating_
with us who are here to try to help fix problems.
We are all _extremely_ busy, I am ill today also, so taking the time to try
to explain problems patiently instead of firing off barely documented
patches is far more likely to get you good results.
Also you fail to actually say what the problem is, what fails, where yoru
test fails etc.
Anyway, let's try to decode (please take this as input as to how you should
try to communicate these things):
So we start with a VMA like this:
012345678901
xxxxxxxxxxxx
We then seal the middle, starting at offset 4:
012345678901
xxxx****xxxx
This sets the VM_SEALED flag in the middle and splits VMAs resulting in 3
VMAs.
We then attempt to unmap 4 pages from offset 2, but this fails, as
expected.
012345678901
xxxx****xxxx
|--| fail
We then attempt to unmap 4 pages from offset 6, but this fails, as
expected.
012345678901
xxxx****xxxx
|--| fail
At each stage we should observe 4 VMAs.
Are you suggesting there is a larger unexpected split? Where? Under what
circumstances?
Let's figure out if there's a problem here _together_.
On Thu, Oct 17, 2024 at 02:26:27AM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
>
> It appears there is a regression on the latest mm,
> when munmap sealed memory, it can cause unexpected VMA split.
> E.g. repro use this test.
> ---
> tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> index fa74dbe4a684..0af33e13b606 100644
> --- a/tools/testing/selftests/mm/mseal_test.c
> +++ b/tools/testing/selftests/mm/mseal_test.c
> @@ -1969,6 +1969,79 @@ static void test_madvise_filebacked_was_writable(bool seal)
> REPORT_TEST_PASS();
> }
>
> +static void test_munmap_free_multiple_ranges_with_split(bool seal)
> +{
> + void *ptr;
> + unsigned long page_size = getpagesize();
> + unsigned long size = 12 * page_size;
> + int ret;
> + int prot;
> +
> + setup_single_address(size, &ptr);
> + FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> + /* seal the middle 4 page */
> + if (seal) {
> + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + size = get_vma_size(ptr, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + }
> +
> + /* munmap 4 pages from the third page */
> + ret = sys_munmap(ptr + 2 * page_size, 4 * page_size);
> + if (seal) {
> + FAIL_TEST_IF_FALSE(ret);
> + FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> + size = get_vma_size(ptr, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + } else
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + /* munmap 4 pages from the sealed page */
> + ret = sys_munmap(ptr + 6 * page_size, 4 * page_size);
> + if (seal) {
> + FAIL_TEST_IF_FALSE(ret);
> + FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
This is repeated below, presumably you mean to do size = get_vma_size(ptr,
&prot) here?
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + } else
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + REPORT_TEST_PASS();
> +}
> +
> +
> int main(int argc, char **argv)
> {
> bool test_seal = seal_support();
> @@ -2099,5 +2172,8 @@ int main(int argc, char **argv)
> test_madvise_filebacked_was_writable(false);
> test_madvise_filebacked_was_writable(true);
>
> + test_munmap_free_multiple_ranges_with_split(false);
> + test_munmap_free_multiple_ranges_with_split(true);
> +
> ksft_finished();
> }
> --
> 2.47.0.rc1.288.g06298d1525-goog
>
OK having said all of the below I think I know exactly what this is...
When an munmap() operation aborts due to error it does not attempt to
re-merge previously split VMAs so you might observe more splits than you
expect.
This is not a bug, it's expected behaviour. We do intend to address this
going forward re: the splits, with an intent to see if we can re-merge or
otherwise change the ordering so if an unmap fails you observe the same VMA
layout.
Before Liam's series I believe you'd see the unsealed portion cleared and
there'd be no recovery so that part of the VMA would just be gone. now we
recover it.
In any case it's absolutely standard in Linux for a task that performs
compound work all of which might fail that, on failure of the overall
operation, that it may be partially fulfilled, partially not.
So yeah, not a bug.
On Thu, Oct 17, 2024 at 10:46:10AM +0100, Lorenzo Stoakes wrote:
> Another thing about etiquette - sending a barely coherent _failing_ test
> with basically zero explanation as a... patch is NOT how to interact with
> the upstream community.
>
> The sensible, respectful and workable way of doing this is to send
> something like a [DISCUSSION] or something and say 'hey guys I think I
> found a bug' with an explanation and a test patch attached.
>
> A lot of your problems with the community could be resolved by being more
> polite, respectful and taking a step back and breathing and _communicating_
> with us who are here to try to help fix problems.
>
> We are all _extremely_ busy, I am ill today also, so taking the time to try
> to explain problems patiently instead of firing off barely documented
> patches is far more likely to get you good results.
>
> Also you fail to actually say what the problem is, what fails, where yoru
> test fails etc.
>
> Anyway, let's try to decode (please take this as input as to how you should
> try to communicate these things):
>
>
> So we start with a VMA like this:
>
> 012345678901
> xxxxxxxxxxxx
>
> We then seal the middle, starting at offset 4:
>
> 012345678901
> xxxx****xxxx
>
> This sets the VM_SEALED flag in the middle and splits VMAs resulting in 3
> VMAs.
>
> We then attempt to unmap 4 pages from offset 2, but this fails, as
> expected.
>
> 012345678901
> xxxx****xxxx
> |--| fail
>
> We then attempt to unmap 4 pages from offset 6, but this fails, as
> expected.
>
> 012345678901
> xxxx****xxxx
> |--| fail
>
> At each stage we should observe 4 VMAs.
>
> Are you suggesting there is a larger unexpected split? Where? Under what
> circumstances?
>
> Let's figure out if there's a problem here _together_.
>
> On Thu, Oct 17, 2024 at 02:26:27AM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@google.com>
> >
> > It appears there is a regression on the latest mm,
> > when munmap sealed memory, it can cause unexpected VMA split.
> > E.g. repro use this test.
> > ---
> > tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++
> > 1 file changed, 76 insertions(+)
> >
> > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > index fa74dbe4a684..0af33e13b606 100644
> > --- a/tools/testing/selftests/mm/mseal_test.c
> > +++ b/tools/testing/selftests/mm/mseal_test.c
> > @@ -1969,6 +1969,79 @@ static void test_madvise_filebacked_was_writable(bool seal)
> > REPORT_TEST_PASS();
> > }
> >
> > +static void test_munmap_free_multiple_ranges_with_split(bool seal)
> > +{
> > + void *ptr;
> > + unsigned long page_size = getpagesize();
> > + unsigned long size = 12 * page_size;
> > + int ret;
> > + int prot;
> > +
> > + setup_single_address(size, &ptr);
> > + FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> > +
> > + /* seal the middle 4 page */
> > + if (seal) {
> > + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > + FAIL_TEST_IF_FALSE(!ret);
> > +
> > + size = get_vma_size(ptr, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 4 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 8 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > + }
> > +
> > + /* munmap 4 pages from the third page */
> > + ret = sys_munmap(ptr + 2 * page_size, 4 * page_size);
> > + if (seal) {
> > + FAIL_TEST_IF_FALSE(ret);
> > + FAIL_TEST_IF_FALSE(errno == EPERM);
> > +
> > + size = get_vma_size(ptr, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 4 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 8 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > + } else
> > + FAIL_TEST_IF_FALSE(!ret);
> > +
> > + /* munmap 4 pages from the sealed page */
> > + ret = sys_munmap(ptr + 6 * page_size, 4 * page_size);
> > + if (seal) {
> > + FAIL_TEST_IF_FALSE(ret);
> > + FAIL_TEST_IF_FALSE(errno == EPERM);
> > +
> > + size = get_vma_size(ptr + 4 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
>
> This is repeated below, presumably you mean to do size = get_vma_size(ptr,
> &prot) here?
>
> > +
> > + size = get_vma_size(ptr + 4 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 8 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > + } else
> > + FAIL_TEST_IF_FALSE(!ret);
> > +
> > + REPORT_TEST_PASS();
> > +}
> > +
> > +
> > int main(int argc, char **argv)
> > {
> > bool test_seal = seal_support();
> > @@ -2099,5 +2172,8 @@ int main(int argc, char **argv)
> > test_madvise_filebacked_was_writable(false);
> > test_madvise_filebacked_was_writable(true);
> >
> > + test_munmap_free_multiple_ranges_with_split(false);
> > + test_munmap_free_multiple_ranges_with_split(true);
> > +
> > ksft_finished();
> > }
> > --
> > 2.47.0.rc1.288.g06298d1525-goog
> >
On Thu, Oct 17, 2024 at 10:46:10AM +0100, Lorenzo Stoakes wrote: [snip] > Anyway, let's try to decode (please take this as input as to how you should > try to communicate these things): > > > So we start with a VMA like this: > > 012345678901 > xxxxxxxxxxxx > > We then seal the middle, starting at offset 4: > > 012345678901 > xxxx****xxxx > > This sets the VM_SEALED flag in the middle and splits VMAs resulting in 3 > VMAs. > > We then attempt to unmap 4 pages from offset 2, but this fails, as > expected. > > 012345678901 > xxxx****xxxx > |--| fail > > We then attempt to unmap 4 pages from offset 6, but this fails, as > expected. > > 012345678901 > xxxx****xxxx > |--| fail > > At each stage we should observe 4 VMAs. CORRECTION: 3 VMAs. [snip]
NACK. Greg's bot got to it but...
As per Greg's bot, no signed-off-by line.
The subject should be something about adding a test.
You later say you are somehow dependning on things (what?) to make this work but
it's broken.
Jeff - you're doing things that were raised on previous reviews as if we
never said them. It's starting to get annoying now. Please try to listen to
upstream.
On Thu, Oct 17, 2024 at 02:26:27AM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
>
> It appears there is a regression on the latest mm,
> when munmap sealed memory, it can cause unexpected VMA split.
> E.g. repro use this test.
This is an unacceptably short commit message. You've been told about this
before.
> ---
> tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> index fa74dbe4a684..0af33e13b606 100644
> --- a/tools/testing/selftests/mm/mseal_test.c
> +++ b/tools/testing/selftests/mm/mseal_test.c
> @@ -1969,6 +1969,79 @@ static void test_madvise_filebacked_was_writable(bool seal)
> REPORT_TEST_PASS();
> }
>
> +static void test_munmap_free_multiple_ranges_with_split(bool seal)
> +{
> + void *ptr;
> + unsigned long page_size = getpagesize();
> + unsigned long size = 12 * page_size;
> + int ret;
> + int prot;
> +
> + setup_single_address(size, &ptr);
> + FAIL_TEST_IF_FALSE(ptr != (void *)-1);
I'm not going to accept any test where you do:
FAIL_TEST_IF_FALSE(<negation>)
As that's totally unreadable. I asked you before for justification and you
didn't provide it, no other tests appear to do this, I wrote thousands of
lines of tests recently without doing this - stop it.
Also referene MAP_FAILED here please. You've been told before.
> +
> + /* seal the middle 4 page */
> + if (seal) {
> + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + size = get_vma_size(ptr, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
Again, you've been told before, stop referencing numbers instead of
PROT_... flags.
OK I'm stopping at this point, you _must listen to review_ Jeff.
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + }
> +
> + /* munmap 4 pages from the third page */
> + ret = sys_munmap(ptr + 2 * page_size, 4 * page_size);
> + if (seal) {
> + FAIL_TEST_IF_FALSE(ret);
> + FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> + size = get_vma_size(ptr, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + } else
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + /* munmap 4 pages from the sealed page */
> + ret = sys_munmap(ptr + 6 * page_size, 4 * page_size);
> + if (seal) {
> + FAIL_TEST_IF_FALSE(ret);
> + FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + } else
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + REPORT_TEST_PASS();
> +}
> +
> +
> int main(int argc, char **argv)
> {
> bool test_seal = seal_support();
> @@ -2099,5 +2172,8 @@ int main(int argc, char **argv)
> test_madvise_filebacked_was_writable(false);
> test_madvise_filebacked_was_writable(true);
>
> + test_munmap_free_multiple_ranges_with_split(false);
> + test_munmap_free_multiple_ranges_with_split(true);
> +
> ksft_finished();
> }
> --
> 2.47.0.rc1.288.g06298d1525-goog
>
On Thu, Oct 17, 2024 at 1:18 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> NACK. Greg's bot got to it but...
>
> As per Greg's bot, no signed-off-by line.
>
Sorry for confusion, I wasn't meant to send this as a PATCH, but
reporting the issue.
The diff was just sent as reference to repro the bug, and I forgot to
remove PATCH from the title. I apologize for the confusion.
-Jeff
> The subject should be something about adding a test.
>
> You later say you are somehow dependning on things (what?) to make this work but
> it's broken.
>
> Jeff - you're doing things that were raised on previous reviews as if we
> never said them. It's starting to get annoying now. Please try to listen to
> upstream.
>
> On Thu, Oct 17, 2024 at 02:26:27AM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@google.com>
> >
> > It appears there is a regression on the latest mm,
> > when munmap sealed memory, it can cause unexpected VMA split.
> > E.g. repro use this test.
>
> This is an unacceptably short commit message. You've been told about this
> before.
>
> > ---
> > tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++
> > 1 file changed, 76 insertions(+)
> >
> > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> > index fa74dbe4a684..0af33e13b606 100644
> > --- a/tools/testing/selftests/mm/mseal_test.c
> > +++ b/tools/testing/selftests/mm/mseal_test.c
> > @@ -1969,6 +1969,79 @@ static void test_madvise_filebacked_was_writable(bool seal)
> > REPORT_TEST_PASS();
> > }
> >
> > +static void test_munmap_free_multiple_ranges_with_split(bool seal)
> > +{
> > + void *ptr;
> > + unsigned long page_size = getpagesize();
> > + unsigned long size = 12 * page_size;
> > + int ret;
> > + int prot;
> > +
> > + setup_single_address(size, &ptr);
> > + FAIL_TEST_IF_FALSE(ptr != (void *)-1);
>
> I'm not going to accept any test where you do:
>
> FAIL_TEST_IF_FALSE(<negation>)
>
> As that's totally unreadable. I asked you before for justification and you
> didn't provide it, no other tests appear to do this, I wrote thousands of
> lines of tests recently without doing this - stop it.
>
> Also referene MAP_FAILED here please. You've been told before.
>
> > +
> > + /* seal the middle 4 page */
> > + if (seal) {
> > + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> > + FAIL_TEST_IF_FALSE(!ret);
> > +
> > + size = get_vma_size(ptr, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
>
> Again, you've been told before, stop referencing numbers instead of
> PROT_... flags.
>
> OK I'm stopping at this point, you _must listen to review_ Jeff.
>
> > +
> > + size = get_vma_size(ptr + 4 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 8 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > + }
> > +
> > + /* munmap 4 pages from the third page */
> > + ret = sys_munmap(ptr + 2 * page_size, 4 * page_size);
> > + if (seal) {
> > + FAIL_TEST_IF_FALSE(ret);
> > + FAIL_TEST_IF_FALSE(errno == EPERM);
> > +
> > + size = get_vma_size(ptr, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 4 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 8 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > + } else
> > + FAIL_TEST_IF_FALSE(!ret);
> > +
> > + /* munmap 4 pages from the sealed page */
> > + ret = sys_munmap(ptr + 6 * page_size, 4 * page_size);
> > + if (seal) {
> > + FAIL_TEST_IF_FALSE(ret);
> > + FAIL_TEST_IF_FALSE(errno == EPERM);
> > +
> > + size = get_vma_size(ptr + 4 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 4 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > +
> > + size = get_vma_size(ptr + 8 * page_size, &prot);
> > + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> > + FAIL_TEST_IF_FALSE(prot == 4);
> > + } else
> > + FAIL_TEST_IF_FALSE(!ret);
> > +
> > + REPORT_TEST_PASS();
> > +}
> > +
> > +
> > int main(int argc, char **argv)
> > {
> > bool test_seal = seal_support();
> > @@ -2099,5 +2172,8 @@ int main(int argc, char **argv)
> > test_madvise_filebacked_was_writable(false);
> > test_madvise_filebacked_was_writable(true);
> >
> > + test_munmap_free_multiple_ranges_with_split(false);
> > + test_munmap_free_multiple_ranges_with_split(true);
> > +
> > ksft_finished();
> > }
> > --
> > 2.47.0.rc1.288.g06298d1525-goog
> >
On Thu, Oct 17, 2024 at 09:20:20AM -0700, Jeff Xu wrote: > On Thu, Oct 17, 2024 at 1:18 AM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > NACK. Greg's bot got to it but... > > > > As per Greg's bot, no signed-off-by line. > > > Sorry for confusion, I wasn't meant to send this as a PATCH, but > reporting the issue. > The diff was just sent as reference to repro the bug, and I forgot to > remove PATCH from the title. I apologize for the confusion. > Can you explain what the issue is? I don't get it. -- Pedro
On Thu, Oct 17, 2024 at 12:14 PM Pedro Falcato <pedro.falcato@gmail.com> wrote: > > On Thu, Oct 17, 2024 at 09:20:20AM -0700, Jeff Xu wrote: > > On Thu, Oct 17, 2024 at 1:18 AM Lorenzo Stoakes > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > NACK. Greg's bot got to it but... > > > > > > As per Greg's bot, no signed-off-by line. > > > > > Sorry for confusion, I wasn't meant to send this as a PATCH, but > > reporting the issue. > > The diff was just sent as reference to repro the bug, and I forgot to > > remove PATCH from the title. I apologize for the confusion. > > > > Can you explain what the issue is? I don't get it. > The issue is there is one VMA that gets splitted after an unmap call fails. Two splitted VMA share the same attributes. e.g. - Allocate 12 pages (0-11). - Seal middle 4 pages (4567) - munmap (2345) - this will fail due to 4567 being sealed. The VMA for page (0123) is split as 2 VMAs (01)-(23), those 2 VMA have the same attribute, and should be merged as one. > -- > Pedro
On Thu, Oct 17, 2024 at 08:14:11PM +0100, Pedro Falcato wrote: > On Thu, Oct 17, 2024 at 09:20:20AM -0700, Jeff Xu wrote: > > On Thu, Oct 17, 2024 at 1:18 AM Lorenzo Stoakes > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > NACK. Greg's bot got to it but... > > > > > > As per Greg's bot, no signed-off-by line. > > > > > Sorry for confusion, I wasn't meant to send this as a PATCH, but > > reporting the issue. > > The diff was just sent as reference to repro the bug, and I forgot to > > remove PATCH from the title. I apologize for the confusion. > > > > Can you explain what the issue is? I don't get it. > > -- > Pedro I'll let Jeff explain from his perspective, but 99% the issue is as described in [0], i.e. not a bug. [0]:https://lore.kernel.org/linux-mm/e359abef-316f-4fca-8d1d-84b69c8bc060@lucifer.local/
On Thu, Oct 17, 2024 at 02:26:27AM +0000, jeffxu@chromium.org wrote: > From: Jeff Xu <jeffxu@google.com> > > It appears there is a regression on the latest mm, > when munmap sealed memory, it can cause unexpected VMA split. > E.g. repro use this test. > --- > tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++ > 1 file changed, 76 insertions(+) > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - Your patch does not have a Signed-off-by: line. Please read the kernel file, Documentation/process/submitting-patches.rst and resend it after adding that line. Note, the line needs to be in the body of the email, before the patch, not at the bottom of the patch or in the email signature. - You did not write a descriptive Subject: for the patch, allowing Greg, and everyone else, to know what this patch is all about. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what a proper Subject: line should look like. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
On Wed, Oct 16, 2024 at 11:04 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Oct 17, 2024 at 02:26:27AM +0000, jeffxu@chromium.org wrote: > > From: Jeff Xu <jeffxu@google.com> > > > > It appears there is a regression on the latest mm, > > when munmap sealed memory, it can cause unexpected VMA split. > > E.g. repro use this test. > > --- > > tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++ > > 1 file changed, 76 insertions(+) > > > > Hi, > > This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him > a patch that has triggered this response. He used to manually respond > to these common problems, but in order to save his sanity (he kept > writing the same thing over and over, yet to different people), I was > created. Hopefully you will not take offence and will fix the problem > in your patch and resubmit it so that it can be accepted into the Linux > kernel tree. > > You are receiving this message because of the following common error(s) > as indicated below: > > - Your patch does not have a Signed-off-by: line. Please read the > kernel file, Documentation/process/submitting-patches.rst and resend > it after adding that line. Note, the line needs to be in the body of > the email, before the patch, not at the bottom of the patch or in the > email signature. > > - You did not write a descriptive Subject: for the patch, allowing Greg, > and everyone else, to know what this patch is all about. Please read > the section entitled "The canonical patch format" in the kernel file, > Documentation/process/submitting-patches.rst for what a proper > Subject: line should look like. > > If you wish to discuss this problem further, or you have questions about > how to resolve this issue, please feel free to respond to this email and > Greg will reply once he has dug out from the pending patches received > from other developers. > Sorry, the title is wrong, it shouldn't start with PATCH, I was trying to send a test case to help debug this issue. > thanks, > > greg k-h's patch email bot
On Wed, Oct 16, 2024 at 7:26 PM <jeffxu@chromium.org> wrote:
>
> From: Jeff Xu <jeffxu@google.com>
>
> It appears there is a regression on the latest mm,
> when munmap seals memory, it can cause an unexpected VMA split.
> E.g. repro use this test.
It appears that this test has some dependency tests that haven't been
merged, so can't be run as is.
This is the repro step:
- Allocate 12 pages (0-11).
- Seal middle 4 pages (4567)
- munmap (2345) - this will fail
Seeing VMA for page (0123) is split as 2 VMAs (01)-(23), those 2 VMA
have the same attribute, and should be merged as one.
> ---
> tools/testing/selftests/mm/mseal_test.c | 76 +++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c
> index fa74dbe4a684..0af33e13b606 100644
> --- a/tools/testing/selftests/mm/mseal_test.c
> +++ b/tools/testing/selftests/mm/mseal_test.c
> @@ -1969,6 +1969,79 @@ static void test_madvise_filebacked_was_writable(bool seal)
> REPORT_TEST_PASS();
> }
>
> +static void test_munmap_free_multiple_ranges_with_split(bool seal)
> +{
> + void *ptr;
> + unsigned long page_size = getpagesize();
> + unsigned long size = 12 * page_size;
> + int ret;
> + int prot;
> +
> + setup_single_address(size, &ptr);
> + FAIL_TEST_IF_FALSE(ptr != (void *)-1);
> +
> + /* seal the middle 4 page */
> + if (seal) {
> + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + size = get_vma_size(ptr, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + }
> +
> + /* munmap 4 pages from the third page */
> + ret = sys_munmap(ptr + 2 * page_size, 4 * page_size);
> + if (seal) {
> + FAIL_TEST_IF_FALSE(ret);
> + FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> + size = get_vma_size(ptr, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + } else
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + /* munmap 4 pages from the sealed page */
> + ret = sys_munmap(ptr + 6 * page_size, 4 * page_size);
> + if (seal) {
> + FAIL_TEST_IF_FALSE(ret);
> + FAIL_TEST_IF_FALSE(errno == EPERM);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 4 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> +
> + size = get_vma_size(ptr + 8 * page_size, &prot);
> + FAIL_TEST_IF_FALSE(size == 4 * page_size);
> + FAIL_TEST_IF_FALSE(prot == 4);
> + } else
> + FAIL_TEST_IF_FALSE(!ret);
> +
> + REPORT_TEST_PASS();
> +}
> +
> +
> int main(int argc, char **argv)
> {
> bool test_seal = seal_support();
> @@ -2099,5 +2172,8 @@ int main(int argc, char **argv)
> test_madvise_filebacked_was_writable(false);
> test_madvise_filebacked_was_writable(true);
>
> + test_munmap_free_multiple_ranges_with_split(false);
> + test_munmap_free_multiple_ranges_with_split(true);
> +
> ksft_finished();
> }
> --
> 2.47.0.rc1.288.g06298d1525-goog
>
© 2016 - 2024 Red Hat, Inc.