rpma: mmap() memory for rpma_mr_reg() in rpma_flush_apm_new()

[ldorau]

The memory region passed to rpma_mr_reg() (ibv_mr_reg())
cannot be allocated from the heap (using malloc()
or posix_memalign()), but should be mapped using mmap().

Rationale:

If ibv_fork_init() was called, the memory region
passed to ibv_mr_reg() is marked by this function
with the DC (“do not copy on fork”) flag
and after having called fork(), the child process
does not receive this range of virtual addresses.
If this memory region was allocated from the heap,
the child process receives a corrupted heap
with a “hole” of inaccessible addresses inside.
A memory allocator knows nothing about this “hole”
and if it tries to access (read or write)
that range of virtual addresses, it causes a segfault.

Ref: #751

---

This change is 

[ldorau]

Ref: #751

[pbalcer]

👍 but I'd wrap the mmap in some util function if you plan to allocate memory like that somewhere else.

[ldorau]

@pbalcer We do not plan - as for now - in the library itself, only in a new example that will be added soon (an example dedicated for the fork() function).

Reviewable status: 0 of 15 files reviewed, all discussions resolved

[grom72]

Shall we also fix malloc_aligned() in example/common/cpommon-conn.c?
or in the context of @pbalcer 's suggestion shall we have common utils function in librpma API that is used in examples as well as in the library (flush_apm_new)?

Reviewed 14 of 15 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ldorau)

---

src/flush.h, line 37 at r2 (raw file):

RPMA_E_INVAL

Is there any UT for RPMA_E_INVAL return value?

---

src/flush.c, line 78 at r2 (raw file):

munmap(raw, mmap_size);

(void) munmap(raw, mmap_size);

---

src/flush.c, line 85 at r2 (raw file):

munmap(raw, mmap_size);

.

---

tests/unit/common/mocks-stdlib.h, line 11 at r2 (raw file):

posix_memalign_args

do we need this?

---

tests/unit/common/mocks-stdlib.c, line 36 at r2 (raw file):

__wrap_posix_memalign

Is it still in use in any UT?

---

tests/unit/flush/flush-new.c, line 57 at r2 (raw file):

posix_memalign_ENOMEM

mmap_ENOMEM

[ldorau]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @grom72 and @ldorau)

---

src/flush.c, line 78 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

munmap(raw, mmap_size);

(void) munmap(raw, mmap_size);

Done.

---

src/flush.c, line 85 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

munmap(raw, mmap_size);

.

Done.

---

tests/unit/common/mocks-stdlib.h, line 11 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

posix_memalign_args

do we need this?

Yes, integration tests use it.

---

tests/unit/common/mocks-stdlib.c, line 36 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

__wrap_posix_memalign

Is it still in use in any UT?

Yes, integration tests use it.

---

tests/unit/flush/flush-new.c, line 57 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

posix_memalign_ENOMEM

mmap_ENOMEM

Done.

[ldorau]

Reviewable status: 13 of 16 files reviewed, 6 unresolved discussions (waiting on @grom72)

---

src/flush.h, line 37 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

RPMA_E_INVAL

Is there any UT for RPMA_E_INVAL return value?

Added

[ldorau]

@grom72

Shall we also fix malloc_aligned() in example/common/common-conn.c?
or in the context of @pbalcer 's suggestion shall we have common utils function in librpma API that is used in examples as well as in the library (flush_apm_new)?

1. malloc_aligned() in example/common/common-conn.c does not need any fix - those examples do not use fork()
2. We do not need any until function for fork() as for now.

[ldorau]

Description added and undrafted.

[grom72]

Reviewed 2 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ldorau)

---

tests/unit/common/mocks-stdlib.h, line 11 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Yes, integration tests use it.

but there is a definition in tests/integration/common/mocks.h
This file is used only by UTs

---

tests/unit/common/mocks-stdlib.c, line 36 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Yes, integration tests use it.

.

[codecov-io]

Codecov Report

Merging #866 (1cb0f35) into master (d9fb10a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #866   +/-   ##
=======================================
  Coverage   99.83%   99.83%           
=======================================
  Files          15       15           
  Lines        1215     1220    +5     
=======================================
+ Hits         1213     1218    +5     
  Misses          2        2           

[ldorau]

Reviewable status: 14 of 16 files reviewed, 2 unresolved discussions (waiting on @grom72)

---

tests/unit/common/mocks-stdlib.h, line 11 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

but there is a definition in tests/integration/common/mocks.h
This file is used only by UTs

You are right, removed

---

tests/unit/common/mocks-stdlib.c, line 36 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.

[ldorau]

Reviewable status: 14 of 16 files reviewed, 3 unresolved discussions (waiting on @grom72)

a discussion (no related file):
Unit tests for rpma_flush_delete() will be added

---

[ldorau]

Reviewable status: 14 of 16 files reviewed, 3 unresolved discussions (waiting on @grom72)

---

tests/unit/common/mocks-stdlib.h, line 11 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

You are right, removed

Done.

[janekmi]

Reviewed 2 of 15 files at r1, 1 of 1 files at r2, 1 of 2 files at r3.
Reviewable status: 14 of 16 files reviewed, 5 unresolved discussions (waiting on @grom72 and @ldorau)

---

src/flush.h, line 37 at r5 (raw file):

 *
 * - RPMA_E_PROVIDER - ibv_dereg_mr() failed
 * - RPMA_E_INVAL - munmap() failed

This new error may be passed to the user level when it is called from rpma_conn_delete(). Please update the librpma.h accordingly.

---

src/flush.c, line 69 at r5 (raw file):

	/* allocate memory for the read-after-write buffer (RAW) */
	void *raw = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,

PROT_READ is not enough?

[janekmi]

Reviewed 1 of 15 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @grom72 and @ldorau)

---

tests/unit/common/mocks-stdlib.c, line 61 at r5 (raw file):

	(void) __len; /* unsused */

	test_free(__addr);

It seems we can validate the addr and len here.

---

tests/unit/common/mocks-stdlib.c, line 63 at r5 (raw file):

	test_free(__addr);

	return 0;

Do we have a negative scenario for munmap()?

---

tests/unit/flush/flush-new.c, line 57 at r5 (raw file):

/*
 * new__apm_mmap_ENOMEM -- malloc() fail with ENOMEM

malloc?

[janekmi]

Reviewed 2 of 15 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @grom72 and @ldorau)

---

tests/integration/common/mocks.c, line 793 at r5 (raw file):

	(void) __len; /* unsused */

	test_free(__addr);

Can we validate addr and len here?

[janekmi]

Reviewed 6 of 15 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @grom72 and @ldorau)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

	struct mmap_args allocated_raw = {0};
	will_return(__wrap_mmap, &allocated_raw);
	will_return(__wrap_mmap, &allocated_raw);

Why it is called twice?

---

tests/integration/example-01-connection/example-01-connection.c, line 250 at r5 (raw file):

	struct mmap_args allocated_raw = {0};
	will_return(__wrap_mmap, &allocated_raw);
	will_return(__wrap_mmap, &allocated_raw);

.

---

tests/integration/example-02-read-to-volatile/example-02-read-to-volatile.c, line 112 at r5 (raw file):

	struct mmap_args allocated_raw = {0};
	will_return(__wrap_mmap, &allocated_raw);
	will_return(__wrap_mmap, &allocated_raw);

Why twice?

---

tests/integration/example-02-read-to-volatile/example-02-read-to-volatile.c, line 310 at r5 (raw file):

	struct mmap_args allocated_raw = {0};
	will_return(__wrap_mmap, &allocated_raw);
	will_return(__wrap_mmap, &allocated_raw);

.

---

tests/integration/example-04-write-to-persistent/example-04-write-to-persistent.c, line 116 at r5 (raw file):

	struct mmap_args flush = {0};
	will_return(__wrap_mmap, &flush);
	will_return(__wrap_mmap, &flush);

Twice?

---

tests/integration/example-04-write-to-persistent/example-04-write-to-persistent.c, line 347 at r5 (raw file):

	struct mmap_args allocated_raw = {0};
	will_return(__wrap_mmap, &allocated_raw);
	will_return(__wrap_mmap, &allocated_raw);

.

[ldorau]

Reviewable status: 13 of 17 files reviewed, 14 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

Unit tests for rpma_flush_delete() will be added

Added.

---

[ldorau]

Reviewable status: 13 of 17 files reviewed, 14 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

---

tests/unit/flush/flush-new.c, line 57 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

malloc?

Done.

[ldorau]

Reviewable status: 13 of 17 files reviewed, 14 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

---

src/flush.c, line 69 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

PROT_READ is not enough?

No, ibv_reg_mr() requires PROT_WRITE.

[ldorau]

Reviewable status: 13 of 17 files reviewed, 14 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

---

src/flush.c, line 69 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

No, ibv_reg_mr() requires PROT_WRITE.

Done.

---

tests/unit/common/mocks-stdlib.c, line 63 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Do we have a negative scenario for munmap()?

Yes - delete__apm_munmap_E_INVAL

[janekmi]

Reviewed 4 of 4 files at r6.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @grom72 and @ldorau)

---

src/flush.c, line 69 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Done.

It is interesting. Did you get this from documentation or by experience? What is the error in this case?

[ldorau]

Reviewable status: 8 of 19 files reviewed, 12 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

---

src/flush.h, line 37 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This new error may be passed to the user level when it is called from rpma_conn_delete(). Please update the librpma.h accordingly.

Done.

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why it is called twice?

cmocka needs it for unknown reason ... :-/

---

tests/unit/common/mocks-stdlib.c, line 61 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems we can validate the addr and len here.

Done.

[janekmi]

Reviewed 11 of 11 files at r7.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @grom72 and @ldorau)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

cmocka needs it for unknown reason ... :-/

Can you set a breakpoint in the __wrap_mmap function and you see it being called twice?

---

tests/unit/flush/flush-common.h, line 27 at r7 (raw file):

struct flush_test_state {
	struct rpma_flush *flush;
	struct mmap_args allocated_raw;

Good idea!

[grom72]

Reviewed 1 of 4 files at r6, 1 of 11 files at r7.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ldorau)

[grom72]

Reviewed 1 of 11 files at r7.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ldorau)

[ldorau]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @janekmi and @ldorau)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Can you set a breakpoint in the __wrap_mmap function and you see it being called twice?

I did it long ago :-) It is hit only once of course, but if there is only one line, cmocka doesn't seem to see it. Apparently I will have to debug cmocka later ....

---

tests/integration/example-01-connection/example-01-connection.c, line 250 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.

---

tests/integration/example-02-read-to-volatile/example-02-read-to-volatile.c, line 112 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why twice?

.

---

tests/integration/example-02-read-to-volatile/example-02-read-to-volatile.c, line 310 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.

---

tests/integration/example-04-write-to-persistent/example-04-write-to-persistent.c, line 116 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Twice?

.

---

tests/integration/example-04-write-to-persistent/example-04-write-to-persistent.c, line 347 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

.

[ldorau]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @janekmi)

---

src/flush.c, line 69 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is interesting. Did you get this from documentation or by experience? What is the error in this case?

From test - EINVAL

---

tests/integration/common/mocks.c, line 793 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Can we validate addr and len here?

Yes. Done.

[ldorau]

Reviewable status: 13 of 19 files reviewed, 8 unresolved discussions (waiting on @grom72 and @janekmi)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

I did it long ago :-) It is hit only once of course, but if there is only one line, cmocka doesn't seem to see it. Apparently I will have to debug cmocka later ....

When there is only one such line, I get the error:

[ RUN      ] test_client__success
[  ERROR   ] --- No entries for symbol __wrap_mmap.
tests/integration/common/mocks.c:775: error: Could not get value to mock function __wrap_mmap
tests/integration/example-01-connection/example-01-connection.c:96: note: Previously returned mock value was declared here
[  FAILED  ] test_client__success

[ldorau]

Reviewable status: 13 of 19 files reviewed, 8 unresolved discussions (waiting on @grom72 and @janekmi)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

When there is only one such line, I get the error:

[ RUN      ] test_client__success
[  ERROR   ] --- No entries for symbol __wrap_mmap.
tests/integration/common/mocks.c:775: error: Could not get value to mock function __wrap_mmap
tests/integration/example-01-connection/example-01-connection.c:96: note: Previously returned mock value was declared here
[  FAILED  ] test_client__success

The same on CI: https://github.com/ldorau/rpma/runs/1877956647?check_suite_focus=true

[grom72]

Reviewed 2 of 11 files at r7, 3 of 6 files at r8.
Reviewable status: 16 of 19 files reviewed, 8 unresolved discussions (waiting on @grom72 and @janekmi)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

When there is only one such line, I get the error:

[ RUN      ] test_client__success
[  ERROR   ] --- No entries for symbol __wrap_mmap.
tests/integration/common/mocks.c:775: error: Could not get value to mock function __wrap_mmap
tests/integration/example-01-connection/example-01-connection.c:96: note: Previously returned mock value was declared here
[  FAILED  ] test_client__success

Does it mean that somebody else calls mmap()?

[grom72]

Reviewed 1 of 11 files at r7, 3 of 6 files at r8.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @janekmi)

[ldorau]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @janekmi)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Does it mean that somebody else calls mmap()?

Maybe cmocka? :-)

[ldorau]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @janekmi)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Maybe cmocka? :-)

I will debug it.

[ldorau]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @janekmi)

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

I will debug it.

What a blindness ... I already know ...

[ldorau]

Reviewable status: 13 of 19 files reviewed, 8 unresolved discussions (waiting on @grom72 and @janekmi)

---

src/flush.c, line 69 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

From test - EINVAL

Done.

---

tests/integration/example-01-connection/example-01-connection.c, line 97 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

What a blindness ... I already know ...

Fixed.

Done.

---

tests/integration/example-01-connection/example-01-connection.c, line 250 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.

---

tests/integration/example-02-read-to-volatile/example-02-read-to-volatile.c, line 112 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.

---

tests/integration/example-02-read-to-volatile/example-02-read-to-volatile.c, line 310 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.

---

tests/integration/example-04-write-to-persistent/example-04-write-to-persistent.c, line 116 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.

---

tests/integration/example-04-write-to-persistent/example-04-write-to-persistent.c, line 347 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.

[janekmi]

Reviewed 3 of 6 files at r8, 6 of 6 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ldorau)

---

src/flush.c, line 69 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Done.

It looks like an undocumented feature. 🤔

[ldorau]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ldorau)

---

src/flush.c, line 69 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It looks like an undocumented feature. 🤔

In rdma-core/libibverbs/cmd_ioctl.c :

147│         if (ioctl(context->cmd_fd, RDMA_VERBS_IOCTL, &cmd->hdr))
148├───────────────> return errno;

returns errno = 22 (EINVAL)

[osalyk]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ldorau)