Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH v4] linux-gen: shm: make internal ishm block names unique #2174

Merged
merged 3 commits into from
Jan 28, 2025
Merged

[PATCH v4] linux-gen: shm: make internal ishm block names unique #2174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a88184a
abi: shm: increase ODP_SHM_NAME_LEN value
MatiasElo Jan 16, 2025
4577650
linux-gen: shm: check shm block name length
MatiasElo Jan 15, 2025
d8a2f1a
linux-gen: shm: make internal ishm block names unique
MatiasElo Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/odp/api/abi-default/shared_memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ typedef struct { char dummy; /**< @internal Dummy */ } _odp_abi_shm_t;
typedef _odp_abi_shm_t *odp_shm_t;

#define ODP_SHM_INVALID ((odp_shm_t)0)
#define ODP_SHM_NAME_LEN 32
#define ODP_SHM_NAME_LEN 64

#define ODP_SHM_IOVA_INVALID ((uint64_t)-1)
#define ODP_SHM_PA_INVALID ODP_SHM_IOVA_INVALID
Expand Down
2 changes: 1 addition & 1 deletion platform/linux-generic/include-abi/odp/api/abi/shared_memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ typedef ODP_HANDLE_T(odp_shm_t);

#define ODP_SHM_INVALID _odp_cast_scalar(odp_shm_t, 0)

#define ODP_SHM_NAME_LEN 32
#define ODP_SHM_NAME_LEN 64

#define ODP_SHM_IOVA_INVALID ((uint64_t)-1)
#define ODP_SHM_PA_INVALID ODP_SHM_IOVA_INVALID
Expand Down
89 changes: 44 additions & 45 deletions platform/linux-generic/odp_ishm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: BSD-3-Clause
* Copyright (c) 2016-2018 Linaro Limited
* Copyright (c) 2019 Nokia
* Copyright (c) 2019-2025 Nokia
*/

/* This file handles the internal shared memory: internal shared memory
Expand Down Expand Up @@ -33,12 +33,15 @@
* _odp_ishm_reserve(), _odp_ishm_free*() and _odp_ishm_lookup*()...
*/
#include <odp_posix_extensions.h>
#include <odp_config_internal.h>
#include <odp_global_data.h>

#include <odp/api/spinlock.h>
#include <odp/api/align.h>
#include <odp/api/shared_memory.h>
#include <odp/api/system_info.h>
#include <odp/api/debug.h>

#include <odp_config_internal.h>
#include <odp_global_data.h>
#include <odp_init_internal.h>
#include <odp_shm_internal.h>
#include <odp_debug_internal.h>
Expand Down Expand Up @@ -80,6 +83,9 @@
*/
#define ISHM_NAME_MAXLEN 128

ODP_STATIC_ASSERT(ISHM_NAME_MAXLEN >= ODP_SHM_NAME_LEN,
"ISHM_NAME_MAXLEN smaller than ODP_SHM_NAME_LEN");

/*
* Linux underlying file name: <directory>/odp-<odp_pid>-ishm-<name>
* The <name> part may be replaced by a sequence number if no specific
Expand Down Expand Up @@ -154,6 +160,7 @@ typedef struct ishm_fragment {
typedef enum {UNKNOWN, HUGE, NORMAL, EXTERNAL, CACHED} huge_flag_t;
typedef struct ishm_block {
char name[ISHM_NAME_MAXLEN]; /* name for the ishm block (if any) */
char param_name[ISHM_NAME_MAXLEN]; /* name given by application */
char filename[ISHM_FILENAME_MAXLEN]; /* name of the .../odp-* file */
char exptname[ISHM_FILENAME_MAXLEN]; /* name of the export file */
uint32_t user_flags; /* any flags the user want to remember. */
Expand Down Expand Up @@ -584,18 +591,14 @@ static void free_fragment(ishm_fragment_t *fragmnt)
}
}

static char *create_seq_string(char *output, size_t size)
{
snprintf(output, size, "%08" PRIu64, ishm_tbl->dev_seq++);

return output;
}

static int create_export_file(ishm_block_t *new_block, const char *name,
uint64_t len, uint32_t flags, uint32_t align,
static int create_export_file(ishm_block_t *new_block, uint64_t len, uint32_t flags, uint32_t align,
odp_bool_t single_va, uint64_t offset)
{
FILE *export_file;
const char *name = new_block->param_name;

if (!new_block->param_name[0])
name = new_block->name;

snprintf(new_block->exptname, ISHM_FILENAME_MAXLEN,
ISHM_EXPTNAME_FORMAT,
Expand Down Expand Up @@ -645,7 +648,6 @@ static int create_file(int block_index, huge_flag_t huge, uint64_t len,
char *name;
int fd;
ishm_block_t *new_block = NULL; /* entry in the main block table */
char seq_string[ISHM_FILENAME_MAXLEN]; /* used to construct filename*/
char filename[ISHM_FILENAME_MAXLEN]; /* filename in /dev/shm or
* /mnt/huge */
int oflag = O_RDWR | O_CREAT | O_TRUNC; /* flags for open */
Expand All @@ -659,9 +661,6 @@ static int create_file(int block_index, huge_flag_t huge, uint64_t len,
} else {
new_block = &ishm_tbl->block[block_index];
name = new_block->name;
if (!name || !name[0])
name = create_seq_string(seq_string,
ISHM_FILENAME_MAXLEN);
}

/* huge dir must be known to create files there!: */
Expand Down Expand Up @@ -721,8 +720,7 @@ static int create_file(int block_index, huge_flag_t huge, uint64_t len,
if (flags & _ODP_ISHM_EXPORT) {
memcpy(new_block->filename, filename, ISHM_FILENAME_MAXLEN);

create_export_file(new_block, name, len, flags, align, false,
0);
create_export_file(new_block, len, flags, align, false, 0);
} else {
new_block->exptname[0] = 0;
/* remove the file from the filesystem, keeping its fd open */
Expand Down Expand Up @@ -790,20 +788,14 @@ static void *do_map(int block_index, uint64_t len, uint32_t align,
/*
* Allocate block from pre-reserved single VA memory
*/
static void *alloc_single_va(const char *name, int new_index, uint64_t size,
uint32_t align, uint32_t flags, int *fd,
static void *alloc_single_va(int new_index, uint64_t size, uint32_t align, uint32_t flags, int *fd,
uint64_t *len_out)
{
uint64_t len;
uint64_t page_sz;
char *file_name = (char *)(uintptr_t)name;
void *addr;
ishm_block_t *new_block = &ishm_tbl->block[new_index];
ishm_fragment_t *fragment = NULL;
char seq_string[ISHM_FILENAME_MAXLEN];

if (!file_name || !file_name[0])
file_name = create_seq_string(seq_string, ISHM_FILENAME_MAXLEN);

/* Common fd for all single VA blocks */
*fd = ishm_tbl->single_va_fd;
Expand Down Expand Up @@ -837,8 +829,7 @@ static void *alloc_single_va(const char *name, int new_index, uint64_t size,
memcpy(new_block->filename, ishm_tbl->single_va_filename,
ISHM_FILENAME_MAXLEN);

create_export_file(new_block, file_name, len, flags, align,
true, offset);
create_export_file(new_block, len, flags, align, true, offset);
} else {
new_block->exptname[0] = 0;
}
Expand Down Expand Up @@ -912,13 +903,24 @@ static int find_block_by_name(const char *name)

for (i = 0; i < ISHM_MAX_NB_BLOCKS; i++) {
if ((ishm_tbl->block[i].len) &&
(strcmp(name, ishm_tbl->block[i].name) == 0))
(strcmp(name, ishm_tbl->block[i].param_name) == 0))
return i;
}

return -1;
}

/*
* Make application given memory block name unique
*/
static void make_name_unique(char *block_name, const char *name, size_t sz)
{
if (name)
_odp_snprint(block_name, sz, "%08" PRIu64 "-%s", ishm_tbl->dev_seq++, name);
else
_odp_snprint(block_name, sz, "%08" PRIu64 "", ishm_tbl->dev_seq++);
}

/*
* Search a given ishm block in the process local table. Return its index
* in the process table or -1 if not found (meaning that the ishm table
Expand Down Expand Up @@ -1088,11 +1090,13 @@ int _odp_ishm_reserve(const char *name, uint64_t size, int fd,

new_block = &ishm_tbl->block[new_index];

/* save block name (if any given): */
/* Save application given block name as-is and make internal block name used for mmap
unique */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to alloc_single_va() later on in this function still uses the application given non-unique name. I did not read carefully but it looks like it would be a problem for exported memory blocks as it would cause overlapping export file names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was not a problem but logic changed in V3.

if (name)
_odp_strcpy(new_block->name, name, ISHM_NAME_MAXLEN);
_odp_strcpy(new_block->param_name, name, ISHM_NAME_MAXLEN);
else
new_block->name[0] = 0;
new_block->param_name[0] = 0;
make_name_unique(new_block->name, name, ISHM_NAME_MAXLEN);

new_block->offset = 0;

Expand Down Expand Up @@ -1199,8 +1203,7 @@ int _odp_ishm_reserve(const char *name, uint64_t size, int fd,
use_single_va:
/* Reserve memory from single VA space */
if (fd < 0 && (flags & _ODP_ISHM_SINGLE_VA))
addr = alloc_single_va(name, new_index, size, align, flags, &fd,
&len);
addr = alloc_single_va(new_index, size, align, flags, &fd, &len);

/* if neither huge pages or normal pages works, we cannot proceed: */
if ((fd < 0) || (addr == NULL) || (len == 0)) {
Expand Down Expand Up @@ -1551,7 +1554,7 @@ int _odp_ishm_info(int block_index, _odp_ishm_info_t *info)
return -1;
}

info->name = ishm_tbl->block[block_index].name;
info->name = ishm_tbl->block[block_index].param_name;
info->addr = ishm_proctable->entry[proc_index].start;
info->size = ishm_tbl->block[block_index].user_len;
info->page_size = (ishm_tbl->block[block_index].huge == HUGE) ?
Expand Down Expand Up @@ -1906,7 +1909,7 @@ int _odp_ishm_term_global(void)
if (block->len != 0) {
_ODP_ERR("block '%s' (file %s) was never freed "
"(cleaning up...).\n",
block->name, block->filename);
block->param_name, block->filename);
delete_file(block);
}
}
Expand Down Expand Up @@ -1979,7 +1982,7 @@ int _odp_ishm_status(const char *title)
if (ishm_tbl->block[i].len <= 0)
continue;

str_len = strlen(ishm_tbl->block[i].name);
str_len = strlen(ishm_tbl->block[i].param_name);

if (max_name_len < str_len)
max_name_len = str_len;
Expand Down Expand Up @@ -2039,7 +2042,7 @@ int _odp_ishm_status(const char *title)
_ODP_PRINT("%2i %-*s %s%c %p-%p %-8" PRIu64 " "
"%-8" PRIu64 " %-3" PRIu64 " %-3" PRIu64 " "
"%-3d %s\n",
i, max_name_len, ishm_tbl->block[i].name,
i, max_name_len, ishm_tbl->block[i].param_name,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since .name was changed to .param_name. the corresponding field width parameter max_name_len should also be calculated from param_name instead of name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in V3.

flags, huge, start_addr, end_addr,
ishm_tbl->block[i].user_len,
ishm_tbl->block[i].len - ishm_tbl->block[i].user_len,
Expand Down Expand Up @@ -2136,7 +2139,8 @@ void _odp_ishm_print(int block_index)
block = &ishm_tbl->block[block_index];

_ODP_PRINT("\nSHM block info\n--------------\n");
_ODP_PRINT(" name: %s\n", block->name);
_ODP_PRINT(" name: %s\n", block->param_name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the internal unique name could still be printed too since this function appears to be meant for debugging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print added in V3.

_ODP_PRINT(" int. name: %s\n", block->name);
_ODP_PRINT(" file: %s\n", block->filename);
_ODP_PRINT(" expt: %s\n", block->exptname);
_ODP_PRINT(" user_flags: 0x%x\n", block->user_flags);
Expand Down Expand Up @@ -2174,7 +2178,7 @@ int32_t odp_system_meminfo(odp_system_meminfo_t *info, odp_system_memblock_t mem
int32_t max_num)
{
ishm_block_t *block;
int name_len, proc_index;
int proc_index;
int32_t i;
uintptr_t addr;
uint64_t len, lost, page_size;
Expand All @@ -2199,12 +2203,7 @@ int32_t odp_system_meminfo(odp_system_meminfo_t *info, odp_system_memblock_t mem
if (num < max_num) {
odp_system_memblock_t *mb = &memblock[num];

name_len = strlen(block->name);
if (name_len >= ODP_SYSTEM_MEMBLOCK_NAME_LEN)
name_len = ODP_SYSTEM_MEMBLOCK_NAME_LEN - 1;

memcpy(mb->name, block->name, name_len);
mb->name[name_len] = 0;
_odp_strcpy(mb->name, block->param_name, ODP_SYSTEM_MEMBLOCK_NAME_LEN);

addr = 0;
proc_index = procfind_block(i);
Expand Down
12 changes: 11 additions & 1 deletion platform/linux-generic/odp_shared_memory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: BSD-3-Clause
* Copyright (c) 2013-2018 Linaro Limited
* Copyright (c) 2019-2021 Nokia
* Copyright (c) 2019-2025 Nokia
*/

#include <odp_config_internal.h>
Expand Down Expand Up @@ -62,6 +62,11 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align,
uint32_t flgs = 0; /* internal ishm flags */
uint32_t supported_flgs = SUPPORTED_SHM_FLAGS;

if (name && strlen(name) + 1 > ODP_SHM_NAME_LEN) {
_ODP_ERR("SHM name too long (max %d)\n", ODP_SHM_NAME_LEN);
return ODP_SHM_INVALID;
}

if (flags & ~supported_flgs) {
_ODP_ERR("Unsupported SHM flag\n");
return ODP_SHM_INVALID;
Expand All @@ -82,6 +87,11 @@ odp_shm_t odp_shm_import(const char *remote_name,
{
int ret;

if (local_name && strlen(local_name) + 1 > ODP_SHM_NAME_LEN) {
_ODP_ERR("SHM local name too long (max %d)\n", ODP_SHM_NAME_LEN);
return ODP_SHM_INVALID;
}

ret = _odp_ishm_find_exported(remote_name, (pid_t)odp_inst,
local_name);

Expand Down