man/fi_av, prov/util, verbs, rxd, shm, sockets, fabtests/unit: Define out of range lookup behavior & add negative lookup test

[zachdworkin]

Attempting fi_av_lookup with a fi_addr larger than the max entry number of the AV will result in undefined behavior. It is expected by an application to not request lookup of an out of bounds fi_addr.

[zachdworkin]

DISCUSSION:
Behavior of fi_av_lookup is undefined for is an application attempts to lookup an address outside of the AV range.
Example: Create an AV table with 32 entries and lookup fi_addr 33.

Current behavior for this case is provider specific. Most use the util av code which does not check for this error and will segfault (if on non-debug build) in ofi_mem.h:ofi_bufpool_get_ibuf:501 when attempting to access a region in the table that is out of bounds. Providers like ucx specifically check for this case and return -FI_EINVAL if the requested fi_addr is out of bounds.

The next common error case is supported where 4 addresses are inserted into an AV with max 32 entries, fi_addr 3 is removed, and then fi_addr 3 is looked up. This is handled and will return -FI_ADDR_NOTAVAIL.

The out of bounds lookup case must either be defined as unsupported in the man page, or a common error such as -FI_EINVAL or -FI_ADDR_NOTAVAIL must be returned to let an application continue if the lookup "fails". I believe returning -FI_ADDR_NOTAVAIL makes the most sense for both of these error cases so the application can handle them both similarly.

No decision will be made until @j-xiong returns from vacation but please post thoughts here.

[j-xiong]

FI_ADDR_NOTAVAIL is not an error code. It is a special value for fi_addr_t.

[aingerson]

I don't think there needs to be a man page distinction between "out of bounds" and not a valid fiaddr. I think they should both return the same error that indicates the fiaddr is not valid.
The application doesn't know whether something is "out of bounds" because the implementation is provider-specific. For example if an address gets removed, that index can be reused or not. That implementation is determined by the provider so the application doesn't know whether a value is within or outside of the bounds of the array.
Providers should be able to easily determine whether the value is within bounds or not for their own implementation so I don't think it's a huge ask.
I agree that the man page needs to be expanded to include the return the application should expect but I wouldn't say it should be left as "undefined." Providers can and should handle those in the same way.

[shijin-aws]

Looking at EFA code, it looks compliant for the proposal? it has the following check before accessing the av entry

if (OFI_LIKELY(ofi_bufpool_ibuf_is_valid(util_av->av_entry_pool, fi_addr)))
		util_av_entry = ofi_bufpool_get_ibuf(util_av->av_entry_pool, fi_addr);
	else
		return NULL;

and the NULL will finally propagate to a -FI_EINVAL return codee in efa_av_lookup

[zachdworkin]

Looking at EFA code, it looks compliant for the proposal, it has the following check before accessing the av entry

if (OFI_LIKELY(ofi_bufpool_ibuf_is_valid(util_av->av_entry_pool, fi_addr)))
		util_av_entry = ofi_bufpool_get_ibuf(util_av->av_entry_pool, fi_addr);
	else
		return NULL;

and the NULL will finally propagate to a -FI_EINVAL return codee in efa_av_lookup

Great! I'll tag you when I open the PR that adds the negative test so you can run by hand

[shijin-aws]

Maybe I am wrong, I am not sure whether ofi_bufpool_ibuf_is_valid is enough, it seems this call has an assertion

static inline bool ofi_bufpool_ibuf_is_valid(struct ofi_bufpool *pool, size_t index)
{
	void *buf;
	size_t region_index = index / pool->attr.chunk_cnt;

	assert(region_index < pool->region_cnt);

	buf = pool->region_table[region_index]->mem_region +
		(index % pool->attr.chunk_cnt) * pool->entry_size;

	return ofi_buf_is_valid(buf);
}

And I think that assertion will fail if the index is too large

It seems UCX checked it as

	ave = (struct ucx_ave *) ofi_array_at_max(&mav->addr_array, fi_addr,
						  mav->count);
	if (!ave)
		return -FI_EINVAL;

[shijin-aws]

Maybe we should just fix ofi_bufpool_ibuf_is_valid? I do not see a strong reason we have to assert here, if region_index >= pool->region_cnt, we can just return false?

@aingerson @j-xiong WDYT?

[zachdworkin]

Maybe we should just fix ofi_bufpool_ibuf_is_valid? I do not see a strong reason we have to assert here, if region_index >= pool->region_cnt, we can just return false?

@aingerson @j-xiong WDYT?

Changing that function is part of the changeset of the PR that adds the negative test. Still fixing other edge cases before opening it.

[aingerson]

@shijin-aws Yeah I think it might be best to remove the assert from the is valid and let the caller assert it if needed depending on the use case

[j-xiong]

I agree. Assertion is used for conditions that shouldn't happen. Here the value can come from user input. We should handle invalid input instead of making the assertion.

[shijin-aws]

bot:aws:retest

[zachdworkin]

@shijin-aws this PR isn't ready for ci to test yet. I will repush with the full changeset

[j-xiong]

Please add prov/rxd: to the title line of commit 60fea38edebed

[zachdworkin]

@swelch @belynam is this okay to merge?

[belynam]

@swelch @belynam is this okay to merge?

Okay by me, approved!