hi,
this is RFC patchset for adding build id under inode's object.
The main change to previous post [1] is to use inode object instead of file
object for build id data.
However.. ;-) while using inode as build id storage place saves some memory
by keeping just one copy of the build id for all file instances, there seems
to be another problem.
The problem is that we read the build id when the file is mmap-ed.
Which is fine for our use case, because we only access build id data through
vma->vm_file->f_inode. But there are possible scenarios/windows where the
build id can be wrong when accessed in another way.
Like when the file is overwritten with another binary version with different
build id. This will result in having wrong build id data in inode until the
new file is mmap-ed.
- file open > inode->i_build_id == NULL
- file mmap
-> read build id > inode->i_build_id == build_id_1
[ file changed with same inode, inode keeps old i_build_id data ]
- file open > inode->i_build_id == build_id_1
- file mmap
-> read build id > inode->i_build_id == build_id_2
I guess we could release i_build_id when the last file's vma go out?
But I'm not sure how to solve this and still be able to access build id
easily just by accessing the inode->i_build_id field without any lock.
I'm inclined to go back and store build id under the file object, where the
build id would be correct (or missing).
thoughts?
thanks,
jirka
v2 changes:
- store build id under inode [Matthew Wilcox]
- use urandom_read and liburandom_read.so for test [Andrii]
- add libelf-based helper to fetch build ID from elf [Andrii]
- store build id or error we got when reading it [Andrii]
- use full name i_build_id [Andrii]
- several tests fixes [Andrii]
[1] https://lore.kernel.org/bpf/[email protected]/
---
Jiri Olsa (9):
mm: Store build id in inode object
bpf: Use file's inode object build id in stackmap
perf: Use file object build id in perf_event_mmap_event
libbpf: Allow to resolve binary path in current directory
selftests/bpf: Add read_buildid function
selftests/bpf: Add err.h header
selftests/bpf: Replace extract_build_id with read_build_id
selftests/bpf: Add inode_build_id test
selftests/bpf: Add iter_task_vma_buildid test
fs/inode.c | 12 +++++++++++
include/linux/buildid.h | 15 ++++++++++++++
include/linux/fs.h | 7 +++++++
kernel/bpf/stackmap.c | 24 +++++++++++++++++++++-
kernel/events/core.c | 46 +++++++++++++++++++++++++++++++++++++----
lib/buildid.c | 40 ++++++++++++++++++++++++++++++++++++
mm/Kconfig | 8 ++++++++
mm/mmap.c | 23 +++++++++++++++++++++
tools/lib/bpf/libbpf.c | 4 +++-
tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/prog_tests/inode_build_id.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c | 19 +++++++----------
tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c | 17 ++++++---------
tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/progs/err.h | 13 ++++++++++++
tools/testing/selftests/bpf/progs/inode_build_id.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/progs/profiler.inc.h | 3 +--
tools/testing/selftests/bpf/test_progs.c | 25 ----------------------
tools/testing/selftests/bpf/test_progs.h | 11 +++++++++-
tools/testing/selftests/bpf/trace_helpers.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/trace_helpers.h | 5 +++++
21 files changed, 581 insertions(+), 57 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/inode_build_id.c
create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
create mode 100644 tools/testing/selftests/bpf/progs/err.h
create mode 100644 tools/testing/selftests/bpf/progs/inode_build_id.c
Storing build id in file's inode object for elf executable with build
id defined. The build id is stored when file is mmaped.
This is enabled with new config option CONFIG_INODE_BUILD_ID.
The build id is valid only when the file with given inode is mmap-ed.
We store either the build id itself or the error we hit during
the retrieval.
Signed-off-by: Jiri Olsa <[email protected]>
---
fs/inode.c | 12 ++++++++++++
include/linux/buildid.h | 15 +++++++++++++++
include/linux/fs.h | 7 +++++++
lib/buildid.c | 40 ++++++++++++++++++++++++++++++++++++++++
mm/Kconfig | 8 ++++++++
mm/mmap.c | 23 +++++++++++++++++++++++
6 files changed, 105 insertions(+)
diff --git a/fs/inode.c b/fs/inode.c
index 4558dc2f1355..e56593e3c301 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -22,6 +22,7 @@
#include <linux/list_lru.h>
#include <linux/iversion.h>
#include <trace/events/writeback.h>
+#include <linux/buildid.h>
#include "internal.h"
/*
@@ -228,6 +229,10 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
#endif
inode->i_flctx = NULL;
+#ifdef CONFIG_INODE_BUILD_ID
+ inode->i_build_id = NULL;
+ spin_lock_init(&inode->i_build_id_lock);
+#endif
if (unlikely(security_inode_alloc(inode)))
return -ENOMEM;
this_cpu_inc(nr_inodes);
@@ -296,6 +301,11 @@ void __destroy_inode(struct inode *inode)
if (inode->i_default_acl && !is_uncached_acl(inode->i_default_acl))
posix_acl_release(inode->i_default_acl);
#endif
+#ifdef CONFIG_INODE_BUILD_ID
+ build_id_free(inode->i_build_id);
+ inode->i_build_id = NULL;
+#endif
+
this_cpu_dec(nr_inodes);
}
EXPORT_SYMBOL(__destroy_inode);
@@ -2242,6 +2252,8 @@ void __init inode_init(void)
SLAB_MEM_SPREAD|SLAB_ACCOUNT),
init_once);
+ build_id_init();
+
/* Hash may have been set up in inode_init_early */
if (!hashdist)
return;
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index 3b7a0ff4642f..485640da9393 100644
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -3,9 +3,15 @@
#define _LINUX_BUILDID_H
#include <linux/mm_types.h>
+#include <linux/slab.h>
#define BUILD_ID_SIZE_MAX 20
+struct build_id {
+ u32 sz;
+ char data[BUILD_ID_SIZE_MAX];
+};
+
int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
__u32 *size);
int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
@@ -17,4 +23,13 @@ void init_vmlinux_build_id(void);
static inline void init_vmlinux_build_id(void) { }
#endif
+#ifdef CONFIG_INODE_BUILD_ID
+void __init build_id_init(void);
+void build_id_free(struct build_id *bid);
+void vma_read_build_id(struct vm_area_struct *vma, struct build_id **bidp);
+#else
+static inline void __init build_id_init(void) { }
+static inline void build_id_free(struct build_id *bid) { }
+#endif /* CONFIG_INODE_BUILD_ID */
+
#endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2acc46fb5f97..72e63dcf86a1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -43,6 +43,7 @@
#include <linux/cred.h>
#include <linux/mnt_idmapping.h>
#include <linux/slab.h>
+#include <linux/buildid.h>
#include <asm/byteorder.h>
#include <uapi/linux/fs.h>
@@ -699,6 +700,12 @@ struct inode {
struct fsverity_info *i_verity_info;
#endif
+#ifdef CONFIG_INODE_BUILD_ID
+ /* Initialized and valid for executable elf files when mmap-ed. */
+ struct build_id *i_build_id;
+ spinlock_t i_build_id_lock;
+#endif
+
void *i_private; /* fs or device private pointer */
} __randomize_layout;
diff --git a/lib/buildid.c b/lib/buildid.c
index dfc62625cae4..2c824e3dcc29 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -5,6 +5,7 @@
#include <linux/elf.h>
#include <linux/kernel.h>
#include <linux/pagemap.h>
+#include <linux/slab.h>
#define BUILD_ID 3
@@ -189,3 +190,42 @@ void __init init_vmlinux_build_id(void)
build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
}
#endif
+
+#ifdef CONFIG_INODE_BUILD_ID
+
+/* SLAB cache for build_id structures */
+static struct kmem_cache *build_id_cachep;
+
+void vma_read_build_id(struct vm_area_struct *vma, struct build_id **bidp)
+{
+ struct build_id *bid = ERR_PTR(-ENOMEM);
+ int err;
+
+ if (!build_id_cachep)
+ goto out;
+ bid = kmem_cache_alloc(build_id_cachep, GFP_KERNEL);
+ if (!bid)
+ goto out;
+ err = build_id_parse(vma, bid->data, &bid->sz);
+ if (err) {
+ build_id_free(bid);
+ bid = ERR_PTR(err);
+ }
+out:
+ *bidp = bid;
+}
+
+void build_id_free(struct build_id *bid)
+{
+ if (IS_ERR_OR_NULL(bid))
+ return;
+ kmem_cache_free(build_id_cachep, bid);
+}
+
+void __init build_id_init(void)
+{
+ build_id_cachep = kmem_cache_create("build_id", sizeof(struct build_id), 0,
+ SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT, NULL);
+}
+
+#endif /* CONFIG_INODE_BUILD_ID */
diff --git a/mm/Kconfig b/mm/Kconfig
index ff7b209dec05..02f40d58ff74 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -1183,6 +1183,14 @@ config LRU_GEN_STATS
This option has a per-memcg and per-node memory overhead.
# }
+config INODE_BUILD_ID
+ bool "Store build id in inode object"
+ default n
+ help
+ Store build id in iinode object for elf executable with build id
+ defined. The build id is stored when file for the given inode is
+ mmap-ed.
+
source "mm/damon/Kconfig"
endmenu
diff --git a/mm/mmap.c b/mm/mmap.c
index 425a9349e610..e6c8ec05804f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2530,6 +2530,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
pgoff_t vm_pgoff;
int error;
MA_STATE(mas, &mm->mm_mt, addr, end - 1);
+ struct build_id *bid = NULL;
/* Check against address space limit. */
if (!may_expand_vm(mm, vm_flags, len >> PAGE_SHIFT)) {
@@ -2626,6 +2627,10 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
if (error)
goto unmap_and_free_vma;
+#ifdef CONFIG_INODE_BUILD_ID
+ if (vma->vm_flags & VM_EXEC)
+ vma_read_build_id(vma, &bid);
+#endif
/*
* Expansion is handled above, merging is handled below.
* Drivers should not alter the address of the VMA.
@@ -2690,6 +2695,23 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
goto free_vma;
}
+#ifdef CONFIG_INODE_BUILD_ID
+ if (bid) {
+ struct inode *inode = file_inode(file);
+
+ spin_lock(&inode->i_build_id_lock);
+ /*
+ * If there's already valid build_id in inode, release it
+ * and use the new one.
+ */
+ if (inode->i_build_id)
+ build_id_free(inode->i_build_id);
+
+ inode->i_build_id = bid;
+ spin_unlock(&inode->i_build_id_lock);
+ }
+#endif
+
if (vma->vm_file)
i_mmap_lock_write(vma->vm_file->f_mapping);
@@ -2759,6 +2781,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
mapping_unmap_writable(file->f_mapping);
free_vma:
vm_area_free(vma);
+ build_id_free(bid);
unacct_error:
if (charged)
vm_unacct_memory(charged);
--
2.39.2
Use build id from file's inode object in stackmap if it's available.
Signed-off-by: Jiri Olsa <[email protected]>
---
kernel/bpf/stackmap.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index aecea7451b61..9b9578e0cada 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -124,6 +124,28 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
return ERR_PTR(err);
}
+#ifdef CONFIG_INODE_BUILD_ID
+static int vma_get_build_id(struct vm_area_struct *vma, unsigned char *build_id)
+{
+ struct build_id *bid;
+
+ if (!vma->vm_file)
+ return -EINVAL;
+ bid = file_inode(vma->vm_file)->i_build_id;
+ if (IS_ERR_OR_NULL(bid))
+ return bid ? PTR_ERR(bid) : -ENOENT;
+ if (bid->sz > BUILD_ID_SIZE_MAX)
+ return -EINVAL;
+ memcpy(build_id, bid->data, bid->sz);
+ return 0;
+}
+#else
+static int vma_get_build_id(struct vm_area_struct *vma, unsigned char *build_id)
+{
+ return build_id_parse(vma, build_id, NULL);
+}
+#endif
+
static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
u64 *ips, u32 trace_nr, bool user)
{
@@ -156,7 +178,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
goto build_id_valid;
}
vma = find_vma(current->mm, ips[i]);
- if (!vma || build_id_parse(vma, id_offs[i].build_id, NULL)) {
+ if (!vma || vma_get_build_id(vma, id_offs[i].build_id)) {
/* per entry fall back to ips */
id_offs[i].status = BPF_STACK_BUILD_ID_IP;
id_offs[i].ip = ips[i];
--
2.39.2
Use build id from file's inode object when available for perf's MMAP2
event build id data.
Signed-off-by: Jiri Olsa <[email protected]>
---
kernel/events/core.c | 46 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7099c77bc53b..148f78a88492 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8527,6 +8527,9 @@ struct perf_mmap_event {
u32 prot, flags;
u8 build_id[BUILD_ID_SIZE_MAX];
u32 build_id_size;
+#ifdef CONFIG_INODE_BUILD_ID
+ struct build_id *i_build_id;
+#endif
struct {
struct perf_event_header header;
@@ -8539,6 +8542,41 @@ struct perf_mmap_event {
} event_id;
};
+#ifdef CONFIG_INODE_BUILD_ID
+static void build_id_read(struct perf_mmap_event *mmap_event)
+{
+ struct vm_area_struct *vma = mmap_event->vma;
+ struct inode *inode = NULL;
+
+ if (vma->vm_file)
+ inode = file_inode(vma->vm_file);
+ mmap_event->i_build_id = inode ? inode->i_build_id : NULL;
+}
+
+static bool has_build_id(struct perf_mmap_event *mmap_event)
+{
+ return !IS_ERR_OR_NULL(mmap_event->i_build_id);
+}
+
+#define build_id_data mmap_event->i_build_id->data
+#define build_id_size mmap_event->i_build_id->sz
+#else
+static void build_id_read(struct perf_mmap_event *mmap_event)
+{
+ struct vm_area_struct *vma = mmap_event->vma;
+
+ build_id_parse(vma, mmap_event->build_id, &mmap_event->build_id_size);
+}
+
+static bool has_build_id(struct perf_mmap_event *mmap_event)
+{
+ return mmap_event->build_id_size;
+}
+
+#define build_id_data mmap_event->build_id
+#define build_id_size mmap_event->build_id_size
+#endif
+
static int perf_event_mmap_match(struct perf_event *event,
void *data)
{
@@ -8583,7 +8621,7 @@ static void perf_event_mmap_output(struct perf_event *event,
mmap_event->event_id.pid = perf_event_pid(event, current);
mmap_event->event_id.tid = perf_event_tid(event, current);
- use_build_id = event->attr.build_id && mmap_event->build_id_size;
+ use_build_id = event->attr.build_id && has_build_id(mmap_event);
if (event->attr.mmap2 && use_build_id)
mmap_event->event_id.header.misc |= PERF_RECORD_MISC_MMAP_BUILD_ID;
@@ -8592,10 +8630,10 @@ static void perf_event_mmap_output(struct perf_event *event,
if (event->attr.mmap2) {
if (use_build_id) {
- u8 size[4] = { (u8) mmap_event->build_id_size, 0, 0, 0 };
+ u8 size[4] = { (u8) build_id_size, 0, 0, 0 };
__output_copy(&handle, size, 4);
- __output_copy(&handle, mmap_event->build_id, BUILD_ID_SIZE_MAX);
+ __output_copy(&handle, build_id_data, BUILD_ID_SIZE_MAX);
} else {
perf_output_put(&handle, mmap_event->maj);
perf_output_put(&handle, mmap_event->min);
@@ -8727,7 +8765,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
mmap_event->event_id.header.size = sizeof(mmap_event->event_id) + size;
if (atomic_read(&nr_build_id_events))
- build_id_parse(vma, mmap_event->build_id, &mmap_event->build_id_size);
+ build_id_read(mmap_event);
perf_iterate_sb(perf_event_mmap_output,
mmap_event,
--
2.39.2
Try to resolve uprobe/usdt binary path also in current directory,
it's used in the test code in following changes.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/lib/bpf/libbpf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 05c4db355f28..f72115e8b7f9 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10727,17 +10727,19 @@ static const char *arch_specific_lib_paths(void)
/* Get full path to program/shared library. */
static int resolve_full_path(const char *file, char *result, size_t result_sz)
{
- const char *search_paths[3] = {};
+ const char *search_paths[4] = {};
int i, perm;
if (str_has_sfx(file, ".so") || strstr(file, ".so.")) {
search_paths[0] = getenv("LD_LIBRARY_PATH");
search_paths[1] = "/usr/lib64:/usr/lib";
search_paths[2] = arch_specific_lib_paths();
+ search_paths[3] = ".";
perm = R_OK;
} else {
search_paths[0] = getenv("PATH");
search_paths[1] = "/usr/bin:/usr/sbin";
+ search_paths[2] = ".";
perm = R_OK | X_OK;
}
--
2.39.2
Adding read_build_id function that parses out build id from
specified binary.
It will replace extract_build_id and also be used in following
changes.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/testing/selftests/bpf/trace_helpers.c | 98 +++++++++++++++++++++
tools/testing/selftests/bpf/trace_helpers.h | 5 ++
2 files changed, 103 insertions(+)
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 09a16a77bae4..c10e16626cd3 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -11,6 +11,9 @@
#include <linux/perf_event.h>
#include <sys/mman.h>
#include "trace_helpers.h"
+#include <linux/limits.h>
+#include <libelf.h>
+#include <gelf.h>
#define DEBUGFS "/sys/kernel/debug/tracing/"
@@ -230,3 +233,98 @@ ssize_t get_rel_offset(uintptr_t addr)
fclose(f);
return -EINVAL;
}
+
+static int
+parse_build_id_buf(const void *note_start, Elf32_Word note_size,
+ char *build_id)
+{
+ Elf32_Word note_offs = 0, new_offs;
+
+ while (note_offs + sizeof(Elf32_Nhdr) < note_size) {
+ Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs);
+
+ if (nhdr->n_type == 3 &&
+ nhdr->n_namesz == sizeof("GNU") &&
+ !strcmp((char *)(nhdr + 1), "GNU") &&
+ nhdr->n_descsz > 0 &&
+ nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
+ memcpy(build_id, note_start + note_offs +
+ ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
+ nhdr->n_descsz);
+ memset(build_id + nhdr->n_descsz, 0,
+ BPF_BUILD_ID_SIZE - nhdr->n_descsz);
+ return (int) nhdr->n_descsz;
+ }
+
+ new_offs = note_offs + sizeof(Elf32_Nhdr) +
+ ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4);
+
+ if (new_offs >= note_size)
+ break;
+ note_offs = new_offs;
+ }
+
+ return -EINVAL;
+}
+
+/* Reads binary from *path* file and returns it in the *build_id*
+ * which is expected to be at least BPF_BUILD_ID_SIZE bytes.
+ * Returns size of build id on success. On error the error value
+ * is returned.
+ */
+int read_build_id(const char *path, char *build_id)
+{
+ int fd, err = -EINVAL;
+ Elf *elf = NULL;
+ GElf_Ehdr ehdr;
+ size_t max, i;
+
+ fd = open(path, O_RDONLY | O_CLOEXEC);
+ if (fd < 0)
+ return -errno;
+
+ (void)elf_version(EV_CURRENT);
+
+ elf = elf_begin(fd, ELF_C_READ, NULL);
+ if (!elf)
+ goto out;
+
+ if (elf_kind(elf) != ELF_K_ELF)
+ goto out;
+
+ if (gelf_getehdr(elf, &ehdr) == NULL)
+ goto out;
+
+ if (ehdr.e_ident[EI_CLASS] != ELFCLASS64)
+ goto out;
+
+ for (i = 0; i < ehdr.e_phnum; i++) {
+ GElf_Phdr mem, *phdr;
+ char *data;
+
+ phdr = gelf_getphdr(elf, i, &mem);
+ if (!phdr)
+ goto out;
+
+ if (phdr->p_type != PT_NOTE)
+ continue;
+
+ data = elf_rawfile(elf, &max);
+ if (!data)
+ goto out;
+
+ if (phdr->p_offset >= max ||
+ (phdr->p_offset + phdr->p_memsz >= max))
+ goto out;
+
+ err = parse_build_id_buf(data + phdr->p_offset, phdr->p_memsz, build_id);
+ if (err > 0)
+ goto out;
+ }
+
+out:
+ if (elf)
+ elf_end(elf);
+ close(fd);
+ return err;
+}
diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
index 53efde0e2998..50b2cc498ba7 100644
--- a/tools/testing/selftests/bpf/trace_helpers.h
+++ b/tools/testing/selftests/bpf/trace_helpers.h
@@ -4,6 +4,9 @@
#include <bpf/libbpf.h>
+#define ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a)-1)
+#define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask))
+
struct ksym {
long addr;
char *name;
@@ -23,4 +26,6 @@ void read_trace_pipe(void);
ssize_t get_uprobe_offset(const void *addr);
ssize_t get_rel_offset(uintptr_t addr);
+int read_build_id(const char *path, char *build_id);
+
#endif
--
2.39.2
Moving error macros from profiler.inc.h to new err.h header.
It will be used in following changes.
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/testing/selftests/bpf/progs/err.h | 13 +++++++++++++
tools/testing/selftests/bpf/progs/profiler.inc.h | 3 +--
2 files changed, 14 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/err.h
diff --git a/tools/testing/selftests/bpf/progs/err.h b/tools/testing/selftests/bpf/progs/err.h
new file mode 100644
index 000000000000..3ac6324da6fd
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/err.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ERR_H__
+#define __ERR_H__
+
+#define MAX_ERRNO 4095
+#define IS_ERR_VALUE(x) (unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO
+
+static inline int IS_ERR_OR_NULL(const void *ptr)
+{
+ return !ptr || IS_ERR_VALUE((unsigned long)ptr);
+}
+
+#endif /* __ERR_H__ */
diff --git a/tools/testing/selftests/bpf/progs/profiler.inc.h b/tools/testing/selftests/bpf/progs/profiler.inc.h
index 875513866032..f799d87e8700 100644
--- a/tools/testing/selftests/bpf/progs/profiler.inc.h
+++ b/tools/testing/selftests/bpf/progs/profiler.inc.h
@@ -6,6 +6,7 @@
#include <bpf/bpf_tracing.h>
#include "profiler.h"
+#include "err.h"
#ifndef NULL
#define NULL 0
@@ -16,7 +17,6 @@
#define O_DIRECTORY 00200000
#define __O_TMPFILE 020000000
#define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
-#define MAX_ERRNO 4095
#define S_IFMT 00170000
#define S_IFSOCK 0140000
#define S_IFLNK 0120000
@@ -34,7 +34,6 @@
#define S_ISBLK(m) (((m)&S_IFMT) == S_IFBLK)
#define S_ISFIFO(m) (((m)&S_IFMT) == S_IFIFO)
#define S_ISSOCK(m) (((m)&S_IFMT) == S_IFSOCK)
-#define IS_ERR_VALUE(x) (unsigned long)(void*)(x) >= (unsigned long)-MAX_ERRNO
#define KILL_DATA_ARRAY_SIZE 8
--
2.39.2
Replacing extract_build_id with read_build_id that parses out
build id directly from elf without using readelf tool.
Signed-off-by: Jiri Olsa <[email protected]>
---
.../bpf/prog_tests/stacktrace_build_id.c | 19 ++++++--------
.../bpf/prog_tests/stacktrace_build_id_nmi.c | 17 +++++--------
tools/testing/selftests/bpf/test_progs.c | 25 -------------------
tools/testing/selftests/bpf/test_progs.h | 1 -
4 files changed, 13 insertions(+), 49 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
index 9ad09a6c538a..9e4b76ee356f 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
@@ -7,13 +7,12 @@ void test_stacktrace_build_id(void)
int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd;
struct test_stacktrace_build_id *skel;
- int err, stack_trace_len;
+ int err, stack_trace_len, build_id_size;
__u32 key, prev_key, val, duration = 0;
- char buf[256];
- int i, j;
+ char buf[BPF_BUILD_ID_SIZE];
struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
int build_id_matches = 0;
- int retry = 1;
+ int i, retry = 1;
retry:
skel = test_stacktrace_build_id__open_and_load();
@@ -52,9 +51,10 @@ void test_stacktrace_build_id(void)
"err %d errno %d\n", err, errno))
goto cleanup;
- err = extract_build_id(buf, 256);
+ build_id_size = read_build_id("./urandom_read", buf);
+ err = build_id_size < 0 ? build_id_size : 0;
- if (CHECK(err, "get build_id with readelf",
+ if (CHECK(err, "read_build_id",
"err %d errno %d\n", err, errno))
goto cleanup;
@@ -64,8 +64,6 @@ void test_stacktrace_build_id(void)
goto cleanup;
do {
- char build_id[64];
-
err = bpf_map_lookup_elem(stackmap_fd, &key, id_offs);
if (CHECK(err, "lookup_elem from stackmap",
"err %d, errno %d\n", err, errno))
@@ -73,10 +71,7 @@ void test_stacktrace_build_id(void)
for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i)
if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID &&
id_offs[i].offset != 0) {
- for (j = 0; j < 20; ++j)
- sprintf(build_id + 2 * j, "%02x",
- id_offs[i].build_id[j] & 0xff);
- if (strstr(buf, build_id) != NULL)
+ if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0)
build_id_matches = 1;
}
prev_key = key;
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
index f4ea1a215ce4..8d84149ebcc7 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
@@ -28,11 +28,10 @@ void test_stacktrace_build_id_nmi(void)
.config = PERF_COUNT_HW_CPU_CYCLES,
};
__u32 key, prev_key, val, duration = 0;
- char buf[256];
- int i, j;
+ char buf[BPF_BUILD_ID_SIZE];
struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
- int build_id_matches = 0;
- int retry = 1;
+ int build_id_matches = 0, build_id_size;
+ int i, retry = 1;
attr.sample_freq = read_perf_max_sample_freq();
@@ -94,7 +93,8 @@ void test_stacktrace_build_id_nmi(void)
"err %d errno %d\n", err, errno))
goto cleanup;
- err = extract_build_id(buf, 256);
+ build_id_size = read_build_id("./urandom_read", buf);
+ err = build_id_size < 0 ? build_id_size : 0;
if (CHECK(err, "get build_id with readelf",
"err %d errno %d\n", err, errno))
@@ -106,8 +106,6 @@ void test_stacktrace_build_id_nmi(void)
goto cleanup;
do {
- char build_id[64];
-
err = bpf_map__lookup_elem(skel->maps.stackmap, &key, sizeof(key),
id_offs, sizeof(id_offs), 0);
if (CHECK(err, "lookup_elem from stackmap",
@@ -116,10 +114,7 @@ void test_stacktrace_build_id_nmi(void)
for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i)
if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID &&
id_offs[i].offset != 0) {
- for (j = 0; j < 20; ++j)
- sprintf(build_id + 2 * j, "%02x",
- id_offs[i].build_id[j] & 0xff);
- if (strstr(buf, build_id) != NULL)
+ if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0)
build_id_matches = 1;
}
prev_key = key;
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 6d5e3022c75f..9813d53c4878 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -591,31 +591,6 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
return err;
}
-int extract_build_id(char *build_id, size_t size)
-{
- FILE *fp;
- char *line = NULL;
- size_t len = 0;
-
- fp = popen("readelf -n ./urandom_read | grep 'Build ID'", "r");
- if (fp == NULL)
- return -1;
-
- if (getline(&line, &len, fp) == -1)
- goto err;
- pclose(fp);
-
- if (len > size)
- len = size;
- memcpy(build_id, line, len);
- build_id[len] = '\0';
- free(line);
- return 0;
-err:
- pclose(fp);
- return -1;
-}
-
static int finit_module(int fd, const char *param_values, int flags)
{
return syscall(__NR_finit_module, fd, param_values, flags);
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 9fbdc57c5b57..3825c2797a4b 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -404,7 +404,6 @@ static inline void *u64_to_ptr(__u64 ptr)
int bpf_find_map(const char *test, struct bpf_object *obj, const char *name);
int compare_map_keys(int map1_fd, int map2_fd);
int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len);
-int extract_build_id(char *build_id, size_t size);
int kern_sync_rcu(void);
int trigger_module_test_read(int read_sz);
int trigger_module_test_write(int write_sz);
--
2.39.2
The test attaches bpf program to sched_process_exec tracepoint
and gets build of executed file from bprm->file->f_inode object.
We use urandom_read as the test program and in addition we also
attach uprobe to liburandom_read.so:urandlib_read_without_sema
and retrieve and check build id of that shared library.
Signed-off-by: Jiri Olsa <[email protected]>
---
.../selftests/bpf/prog_tests/inode_build_id.c | 68 +++++++++++++++++++
.../selftests/bpf/progs/inode_build_id.c | 62 +++++++++++++++++
tools/testing/selftests/bpf/test_progs.h | 10 +++
3 files changed, 140 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/inode_build_id.c
create mode 100644 tools/testing/selftests/bpf/progs/inode_build_id.c
diff --git a/tools/testing/selftests/bpf/prog_tests/inode_build_id.c b/tools/testing/selftests/bpf/prog_tests/inode_build_id.c
new file mode 100644
index 000000000000..d0add90f187d
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/inode_build_id.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <unistd.h>
+#include <test_progs.h>
+#include "inode_build_id.skel.h"
+#include "trace_helpers.h"
+
+void test_inode_build_id(void)
+{
+ int go[2], err, child_pid, child_status, c = 1, sz;
+ char build_id[BPF_BUILD_ID_SIZE];
+ struct inode_build_id *skel;
+
+ skel = inode_build_id__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "inode_build_id__open_and_load"))
+ return;
+
+ if (!ASSERT_OK(pipe(go), "pipe"))
+ goto out;
+
+ child_pid = fork();
+ if (child_pid < 0)
+ goto out;
+
+ /* child */
+ if (child_pid == 0) {
+ /* wait for parent's pid update */
+ err = read(go[0], &c, 1);
+ if (!ASSERT_EQ(err, 1, "child_read_pipe"))
+ exit(err);
+
+ execle("./urandom_read", "urandom_read", NULL, NULL);
+ exit(errno);
+ }
+
+ /* parent, update child's pid and kick it */
+ skel->bss->pid = child_pid;
+
+ err = inode_build_id__attach(skel);
+ if (!ASSERT_OK(err, "inode_build_id__attach"))
+ goto out;
+
+ err = write(go[1], &c, 1);
+ if (!ASSERT_EQ(err, 1, "child_write_pipe"))
+ goto out;
+
+ /* wait for child to exit */
+ waitpid(child_pid, &child_status, 0);
+ if (!ASSERT_EQ(WEXITSTATUS(child_status), 0, "child_exit_value"))
+ goto out;
+
+ sz = read_build_id("./urandom_read", build_id);
+ if (!ASSERT_GT(sz, 0, "read_build_id"))
+ goto out;
+
+ ASSERT_EQ(skel->bss->build_id_bin_size, sz, "build_id_bin_size");
+ ASSERT_MEMEQ(skel->bss->build_id_bin, build_id, sz, "build_id_bin");
+
+ sz = read_build_id("./liburandom_read.so", build_id);
+ if (!ASSERT_GT(sz, 0, "read_build_id"))
+ goto out;
+
+ ASSERT_EQ(skel->bss->build_id_lib_size, sz, "build_id_lib_size");
+ ASSERT_MEMEQ(skel->bss->build_id_lib, build_id, sz, "build_id_lib");
+
+out:
+ inode_build_id__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/inode_build_id.c b/tools/testing/selftests/bpf/progs/inode_build_id.c
new file mode 100644
index 000000000000..eceb215b56b8
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/inode_build_id.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include "err.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <linux/string.h>
+
+char _license[] SEC("license") = "GPL";
+
+int pid;
+
+u32 build_id_bin_size;
+u32 build_id_lib_size;
+
+char build_id_bin[20];
+char build_id_lib[20];
+
+static int store_build_id(struct inode *inode, char *build_id, u32 *sz)
+{
+ struct build_id *bid;
+
+ bid = inode->i_build_id;
+ if (IS_ERR_OR_NULL(bid))
+ return 0;
+ *sz = bid->sz;
+ if (bid->sz > sizeof(bid->data))
+ return 0;
+ __builtin_memcpy(build_id, bid->data, sizeof(bid->data));
+ return 0;
+}
+
+SEC("tp_btf/sched_process_exec")
+int BPF_PROG(prog, struct task_struct *p, pid_t old_pid, struct linux_binprm *bprm)
+{
+ int cur_pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != cur_pid)
+ return 0;
+ if (!bprm->file || !bprm->file->f_inode)
+ return 0;
+ return store_build_id(bprm->file->f_inode, build_id_bin, &build_id_bin_size);
+}
+
+static long check_vma(struct task_struct *task, struct vm_area_struct *vma,
+ void *data)
+{
+ if (!vma || !vma->vm_file || !vma->vm_file->f_inode)
+ return 0;
+ return store_build_id(vma->vm_file->f_inode, build_id_lib, &build_id_lib_size);
+}
+
+SEC("uprobe/liburandom_read.so:urandlib_read_without_sema")
+int BPF_UPROBE(urandlib_read_without_sema)
+{
+ struct task_struct *task = bpf_get_current_task_btf();
+ int cur_pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != cur_pid)
+ return 0;
+ return bpf_find_vma(task, ctx->ip, check_vma, NULL, 0);
+}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 3825c2797a4b..8156d6d4cb3b 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -310,6 +310,16 @@ int test__join_cgroup(const char *path);
___ok; \
})
+#define ASSERT_MEMEQ(actual, expected, sz, name) ({ \
+ static int duration = 0; \
+ const char *___act = actual; \
+ const char *___exp = expected; \
+ bool ___ok = memcmp(___act, ___exp, sz) == 0; \
+ CHECK(!___ok, (name), \
+ "unexpected %s does not match\n", (name)); \
+ ___ok; \
+})
+
#define ASSERT_STRNEQ(actual, expected, len, name) ({ \
static int duration = 0; \
const char *___act = actual; \
--
2.39.2
Testing iterator access to build id in vma->vm_file->f_inode
object by storing each binary with buildid into map and checking
it against buildid retrieved in user space.
Signed-off-by: Jiri Olsa <[email protected]>
---
.../selftests/bpf/prog_tests/bpf_iter.c | 78 +++++++++++++++++++
.../bpf/progs/bpf_iter_task_vma_buildid.c | 60 ++++++++++++++
2 files changed, 138 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 1f02168103dd..c7dd89e7cad0 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -33,6 +33,7 @@
#include "bpf_iter_bpf_link.skel.h"
#include "bpf_iter_ksym.skel.h"
#include "bpf_iter_sockmap.skel.h"
+#include "bpf_iter_task_vma_buildid.skel.h"
static int duration;
@@ -1536,6 +1537,81 @@ static void test_task_vma_dead_task(void)
bpf_iter_task_vma__destroy(skel);
}
+#define D_PATH_BUF_SIZE 1024
+
+struct build_id {
+ u32 sz;
+ char data[BPF_BUILD_ID_SIZE];
+};
+
+static void test_task_vma_buildid(void)
+{
+ int err, iter_fd = -1, proc_maps_fd = -1, sz;
+ struct bpf_iter_task_vma_buildid *skel;
+ char key[D_PATH_BUF_SIZE], *prev_key;
+ char build_id[BPF_BUILD_ID_SIZE];
+ int len, files_fd, cnt = 0;
+ struct build_id val;
+ char c;
+
+ skel = bpf_iter_task_vma_buildid__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "bpf_iter_task_vma_buildid__open_and_load"))
+ return;
+
+ skel->links.proc_maps = bpf_program__attach_iter(
+ skel->progs.proc_maps, NULL);
+
+ if (!ASSERT_OK_PTR(skel->links.proc_maps, "bpf_program__attach_iter")) {
+ skel->links.proc_maps = NULL;
+ goto out;
+ }
+
+ iter_fd = bpf_iter_create(bpf_link__fd(skel->links.proc_maps));
+ if (!ASSERT_GE(iter_fd, 0, "create_iter"))
+ goto out;
+
+ /* trigger the iterator, there's no output, just map */
+ len = read(iter_fd, &c, 1);
+ ASSERT_EQ(len, 0, "len_check");
+
+ files_fd = bpf_map__fd(skel->maps.files);
+
+ prev_key = NULL;
+
+ while (true) {
+ err = bpf_map_get_next_key(files_fd, prev_key, &key);
+ if (err) {
+ if (errno == ENOENT)
+ err = 0;
+ break;
+ }
+ if (bpf_map_lookup_elem(files_fd, key, &val))
+ break;
+ if (!ASSERT_LE(val.sz, BPF_BUILD_ID_SIZE, "buildid_size"))
+ break;
+
+ sz = read_build_id(key, build_id);
+ /* If there's an error, the build id is not present or malformed, kernel
+ * should see the same result and bpf program pushed zero build id.
+ */
+ if (sz < 0) {
+ memset(build_id, 0x0, BPF_BUILD_ID_SIZE);
+ sz = BPF_BUILD_ID_SIZE;
+ }
+ ASSERT_EQ(val.sz, sz, "build_id_size");
+ ASSERT_MEMEQ(val.data, build_id, sz, "build_id_data");
+
+ prev_key = key;
+ cnt++;
+ }
+
+ printf("checked %d files\n", cnt);
+out:
+ close(proc_maps_fd);
+ close(iter_fd);
+ bpf_iter_task_vma_buildid__destroy(skel);
+}
+
void test_bpf_sockmap_map_iter_fd(void)
{
struct bpf_iter_sockmap *skel;
@@ -1659,6 +1735,8 @@ void test_bpf_iter(void)
test_task_vma();
if (test__start_subtest("task_vma_dead_task"))
test_task_vma_dead_task();
+ if (test__start_subtest("task_vma_buildid"))
+ test_task_vma_buildid();
if (test__start_subtest("task_btf"))
test_task_btf();
if (test__start_subtest("tcp4"))
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
new file mode 100644
index 000000000000..dc528a4783ec
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "bpf_iter.h"
+#include "err.h"
+#include <bpf/bpf_helpers.h>
+#include <string.h>
+
+char _license[] SEC("license") = "GPL";
+
+#define VM_EXEC 0x00000004
+#define D_PATH_BUF_SIZE 1024
+
+struct {
+ __uint(type, BPF_MAP_TYPE_HASH);
+ __uint(max_entries, 10000);
+ __type(key, char[D_PATH_BUF_SIZE]);
+ __type(value, struct build_id);
+} files SEC(".maps");
+
+static char path[D_PATH_BUF_SIZE];
+static struct build_id build_id;
+
+SEC("iter/task_vma")
+int proc_maps(struct bpf_iter__task_vma *ctx)
+{
+ struct vm_area_struct *vma = ctx->vma;
+ struct seq_file *seq = ctx->meta->seq;
+ struct task_struct *task = ctx->task;
+ unsigned long file_key;
+ struct inode *inode;
+ struct file *file;
+
+ if (task == (void *)0 || vma == (void *)0)
+ return 0;
+
+ if (!(vma->vm_flags & VM_EXEC))
+ return 0;
+
+ file = vma->vm_file;
+ if (!file)
+ return 0;
+
+ __builtin_memset(path, 0x0, D_PATH_BUF_SIZE);
+ bpf_d_path(&file->f_path, (char *) &path, D_PATH_BUF_SIZE);
+
+ if (bpf_map_lookup_elem(&files, &path))
+ return 0;
+
+ inode = file->f_inode;
+ if (IS_ERR_OR_NULL(inode->i_build_id)) {
+ /* On error return empty build id. */
+ __builtin_memset(&build_id.data, 0x0, sizeof(build_id.data));
+ build_id.sz = 20;
+ } else {
+ __builtin_memcpy(&build_id, inode->i_build_id, sizeof(*inode->i_build_id));
+ }
+
+ bpf_map_update_elem(&files, &path, &build_id, 0);
+ return 0;
+}
--
2.39.2
On Tue, 28 Feb 2023 10:31:58 +0100 Jiri Olsa <[email protected]> wrote:
> Storing build id in file's inode object for elf executable with build
> id defined. The build id is stored when file is mmaped.
>
> This is enabled with new config option CONFIG_INODE_BUILD_ID.
>
> The build id is valid only when the file with given inode is mmap-ed.
>
> We store either the build id itself or the error we hit during
> the retrieval.
>
> ...
>
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -699,6 +700,12 @@ struct inode {
> struct fsverity_info *i_verity_info;
> #endif
>
> +#ifdef CONFIG_INODE_BUILD_ID
> + /* Initialized and valid for executable elf files when mmap-ed. */
> + struct build_id *i_build_id;
> + spinlock_t i_build_id_lock;
> +#endif
> +
Remember we can have squillions of inodes in memory. So that's one
costly spinlock!
AFAICT this lock could be removed if mmap_region() were to use an
atomic exchange on inode->i_build_id?
If not, can we use an existing lock? i_lock would be appropriate
(don't forget to update its comment).
Also, the code in mmap_region() runs build_id_free() inside the locked
region, which seems unnecessary.
On Tue, Feb 28, 2023 at 10:31:57AM +0100, Jiri Olsa wrote:
> hi,
> this is RFC patchset for adding build id under inode's object.
>
> The main change to previous post [1] is to use inode object instead of file
> object for build id data.
Please explain what a "build id" is, the use case for it, why we
need to store it in VFS objects, what threat model it is protecting
the system against, etc.
>
> However.. ;-) while using inode as build id storage place saves some memory
> by keeping just one copy of the build id for all file instances, there seems
> to be another problem.
Yes, the problem being that we can cache hundreds of millions of
inodes in memory, and only a very small subset of them are going to
have open files associated with them. And an even smaller subset are
going to be mmapped.
So, in reality, this proposal won't save any memory at all - it
costs memory for every inode that is not currently being used as
a mmapped elf executable, right?
> The problem is that we read the build id when the file is mmap-ed.
Why? I'm completely clueless as to what this thing does or how it's
used....
> Which is fine for our use case,
Which is?
-Dave.
--
Dave Chinner
[email protected]
On Tue, Feb 28, 2023 at 11:13:10AM -0800, Andrew Morton wrote:
> On Tue, 28 Feb 2023 10:31:58 +0100 Jiri Olsa <[email protected]> wrote:
>
> > Storing build id in file's inode object for elf executable with build
> > id defined. The build id is stored when file is mmaped.
> >
> > This is enabled with new config option CONFIG_INODE_BUILD_ID.
> >
> > The build id is valid only when the file with given inode is mmap-ed.
> >
> > We store either the build id itself or the error we hit during
> > the retrieval.
> >
> > ...
> >
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -699,6 +700,12 @@ struct inode {
> > struct fsverity_info *i_verity_info;
> > #endif
> >
> > +#ifdef CONFIG_INODE_BUILD_ID
> > + /* Initialized and valid for executable elf files when mmap-ed. */
> > + struct build_id *i_build_id;
> > + spinlock_t i_build_id_lock;
> > +#endif
> > +
>
> Remember we can have squillions of inodes in memory. So that's one
> costly spinlock!
>
> AFAICT this lock could be removed if mmap_region() were to use an
> atomic exchange on inode->i_build_id?
right, that should work I'll check
>
> If not, can we use an existing lock? i_lock would be appropriate
> (don't forget to update its comment).
ok
>
> Also, the code in mmap_region() runs build_id_free() inside the locked
> region, which seems unnecessary.
>
ok, if the atomic exchange is doable, it'll take care of this
thanks,
jirka
Em Wed, Mar 01, 2023 at 09:07:14AM +1100, Dave Chinner escreveu:
> On Tue, Feb 28, 2023 at 10:31:57AM +0100, Jiri Olsa wrote:
> > this is RFC patchset for adding build id under inode's object.
> > The main change to previous post [1] is to use inode object instead of file
> > object for build id data.
>
> Please explain what a "build id" is, the use case for it, why we
> need to store it in VFS objects, what threat model it is protecting
> the system against, etc.
[root@quaco ~]# file /bin/bash
/bin/bash: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=160df51238a38ca27d03290f3ad5f7df75560ae0, for GNU/Linux 3.2.0, stripped
[root@quaco ~]# file /lib64/libc.so.6
/lib64/libc.so.6: ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=8257ee907646e9b057197533d1e4ac8ede7a9c5c, for GNU/Linux 3.2.0, not stripped
[root@quaco ~]#
Those BuildID[sha1]= bits, that is present in all binaries I think in
all distros for quite a while.
This page, from when this was initially designed, has a discussion about
it, why it is needed, etc:
https://fedoraproject.org/wiki/RolandMcGrath/BuildID
'perf record' will receive MMAP records, initially without build-ids,
now we have one that has, but collecting it when the mmap is executed
(and thus a PERF_RECORD_MMAP* record is emitted) may not work, thus this
work from Jiri.
- Arnaldo
> >
> > However.. ;-) while using inode as build id storage place saves some memory
> > by keeping just one copy of the build id for all file instances, there seems
> > to be another problem.
> Yes, the problem being that we can cache hundreds of millions of
> inodes in memory, and only a very small subset of them are going to
> have open files associated with them. And an even smaller subset are
> going to be mmapped.
> So, in reality, this proposal won't save any memory at all - it
> costs memory for every inode that is not currently being used as
> a mmapped elf executable, right?
>
> > The problem is that we read the build id when the file is mmap-ed.
>
> Why? I'm completely clueless as to what this thing does or how it's
> used....
>
> > Which is fine for our use case,
>
> Which is?
>
> -Dave.
> --
> Dave Chinner
> [email protected]
On Wed, Mar 01, 2023 at 09:07:14AM +1100, Dave Chinner wrote:
> On Tue, Feb 28, 2023 at 10:31:57AM +0100, Jiri Olsa wrote:
> > hi,
> > this is RFC patchset for adding build id under inode's object.
> >
> > The main change to previous post [1] is to use inode object instead of file
> > object for build id data.
>
> Please explain what a "build id" is, the use case for it, why we
> need to store it in VFS objects, what threat model it is protecting
> the system against, etc.
hum I still did not get your email from mailing list, just saw it
from Arnaldo's reply and downloaded it from lore
our use case is for hubble/tetragon [1] and we are asked to report
buildid of executed binary.. but the monitoring process is running
in its own pod and can't access the the binaries outside of it, so
we need to be able to read it in kernel
we want to read build id from BPF program attached to sched_exec
tracepoint, and from BPF iterator
we considered adding BPF helper and then kfunc for that, but it turned
out it'd be usefull for other use cases (like retrieving build id from
atomic context [2]) to have the build id stored in file (or inode) object
[1] https://github.com/cilium/tetragon/
[2] https://lore.kernel.org/bpf/CA+khW7juLEcrTOd7iKG3C_WY8L265XKNo0iLzV1fE=o-cyeHcQ@mail.gmail.com/
>
> >
> > However.. ;-) while using inode as build id storage place saves some memory
> > by keeping just one copy of the build id for all file instances, there seems
> > to be another problem.
>
> Yes, the problem being that we can cache hundreds of millions of
> inodes in memory, and only a very small subset of them are going to
> have open files associated with them. And an even smaller subset are
> going to be mmapped.
ok, file seems like better option now
>
> So, in reality, this proposal won't save any memory at all - it
> costs memory for every inode that is not currently being used as
> a mmapped elf executable, right?
right
>
> > The problem is that we read the build id when the file is mmap-ed.
>
> Why? I'm completely clueless as to what this thing does or how it's
> used....
we need the build id only when the file is mmap-ed, so it seemed like
the best way to read it when the file is mmaped
>
> > Which is fine for our use case,
>
> Which is?
please see above
thanks,
jirka
On Wed, Mar 01, 2023 at 12:41:20PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 01, 2023 at 09:07:14AM +1100, Dave Chinner escreveu:
> > On Tue, Feb 28, 2023 at 10:31:57AM +0100, Jiri Olsa wrote:
> > > this is RFC patchset for adding build id under inode's object.
>
> > > The main change to previous post [1] is to use inode object instead of file
> > > object for build id data.
> >
> > Please explain what a "build id" is, the use case for it, why we
> > need to store it in VFS objects, what threat model it is protecting
> > the system against, etc.
>
> [root@quaco ~]# file /bin/bash
> /bin/bash: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=160df51238a38ca27d03290f3ad5f7df75560ae0, for GNU/Linux 3.2.0, stripped
> [root@quaco ~]# file /lib64/libc.so.6
> /lib64/libc.so.6: ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=8257ee907646e9b057197533d1e4ac8ede7a9c5c, for GNU/Linux 3.2.0, not stripped
> [root@quaco ~]#
>
> Those BuildID[sha1]= bits, that is present in all binaries I think in
> all distros for quite a while.
>
> This page, from when this was initially designed, has a discussion about
> it, why it is needed, etc:
>
> https://fedoraproject.org/wiki/RolandMcGrath/BuildID
>
> 'perf record' will receive MMAP records, initially without build-ids,
> now we have one that has, but collecting it when the mmap is executed
> (and thus a PERF_RECORD_MMAP* record is emitted) may not work, thus this
> work from Jiri.
thanks for the pointers
build id is unique id for binary that's been used to identify
correct binary version for related stuff.. like binary's debuginfo
in perf or match binary with stack trace entries in bpf stackmap
jirka
>
> - Arnaldo
>
> > >
> > > However.. ;-) while using inode as build id storage place saves some memory
> > > by keeping just one copy of the build id for all file instances, there seems
> > > to be another problem.
>
> > Yes, the problem being that we can cache hundreds of millions of
> > inodes in memory, and only a very small subset of them are going to
> > have open files associated with them. And an even smaller subset are
> > going to be mmapped.
>
> > So, in reality, this proposal won't save any memory at all - it
> > costs memory for every inode that is not currently being used as
> > a mmapped elf executable, right?
> >
> > > The problem is that we read the build id when the file is mmap-ed.
> >
> > Why? I'm completely clueless as to what this thing does or how it's
> > used....
> >
> > > Which is fine for our use case,
> >
> > Which is?
> >
> > -Dave.
> > --
> > Dave Chinner
> > [email protected]
On Tue, Feb 28, 2023 at 1:33 AM Jiri Olsa <[email protected]> wrote:
>
> Try to resolve uprobe/usdt binary path also in current directory,
> it's used in the test code in following changes.
nope, that's not what shell is doing, so let's not invent new rules
here. If some tests need something like that, utilize LD_LIBRARY_PATH
or even better just specify './library.so'
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/lib/bpf/libbpf.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 05c4db355f28..f72115e8b7f9 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10727,17 +10727,19 @@ static const char *arch_specific_lib_paths(void)
> /* Get full path to program/shared library. */
> static int resolve_full_path(const char *file, char *result, size_t result_sz)
> {
> - const char *search_paths[3] = {};
> + const char *search_paths[4] = {};
> int i, perm;
>
> if (str_has_sfx(file, ".so") || strstr(file, ".so.")) {
> search_paths[0] = getenv("LD_LIBRARY_PATH");
> search_paths[1] = "/usr/lib64:/usr/lib";
> search_paths[2] = arch_specific_lib_paths();
> + search_paths[3] = ".";
> perm = R_OK;
> } else {
> search_paths[0] = getenv("PATH");
> search_paths[1] = "/usr/bin:/usr/sbin";
> + search_paths[2] = ".";
> perm = R_OK | X_OK;
> }
>
> --
> 2.39.2
>
On Tue, Feb 28, 2023 at 1:33 AM Jiri Olsa <[email protected]> wrote:
>
> Adding read_build_id function that parses out build id from
> specified binary.
>
> It will replace extract_build_id and also be used in following
> changes.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/testing/selftests/bpf/trace_helpers.c | 98 +++++++++++++++++++++
> tools/testing/selftests/bpf/trace_helpers.h | 5 ++
> 2 files changed, 103 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index 09a16a77bae4..c10e16626cd3 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c
> @@ -11,6 +11,9 @@
> #include <linux/perf_event.h>
> #include <sys/mman.h>
> #include "trace_helpers.h"
> +#include <linux/limits.h>
> +#include <libelf.h>
> +#include <gelf.h>
>
> #define DEBUGFS "/sys/kernel/debug/tracing/"
>
> @@ -230,3 +233,98 @@ ssize_t get_rel_offset(uintptr_t addr)
> fclose(f);
> return -EINVAL;
> }
> +
> +static int
> +parse_build_id_buf(const void *note_start, Elf32_Word note_size,
> + char *build_id)
> +{
> + Elf32_Word note_offs = 0, new_offs;
> +
> + while (note_offs + sizeof(Elf32_Nhdr) < note_size) {
> + Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs);
> +
> + if (nhdr->n_type == 3 &&
> + nhdr->n_namesz == sizeof("GNU") &&
> + !strcmp((char *)(nhdr + 1), "GNU") &&
> + nhdr->n_descsz > 0 &&
> + nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
> + memcpy(build_id, note_start + note_offs +
> + ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
> + nhdr->n_descsz);
> + memset(build_id + nhdr->n_descsz, 0,
> + BPF_BUILD_ID_SIZE - nhdr->n_descsz);
I won't count :) but if something fits within 100 characters, please
keep it on single line
> + return (int) nhdr->n_descsz;
> + }
> +
> + new_offs = note_offs + sizeof(Elf32_Nhdr) +
> + ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4);
> +
> + if (new_offs >= note_size)
> + break;
> + note_offs = new_offs;
> + }
> +
> + return -EINVAL;
> +}
> +
> +/* Reads binary from *path* file and returns it in the *build_id*
> + * which is expected to be at least BPF_BUILD_ID_SIZE bytes.
> + * Returns size of build id on success. On error the error value
> + * is returned.
> + */
> +int read_build_id(const char *path, char *build_id)
> +{
> + int fd, err = -EINVAL;
> + Elf *elf = NULL;
> + GElf_Ehdr ehdr;
> + size_t max, i;
> +
> + fd = open(path, O_RDONLY | O_CLOEXEC);
> + if (fd < 0)
> + return -errno;
> +
> + (void)elf_version(EV_CURRENT);
> +
> + elf = elf_begin(fd, ELF_C_READ, NULL);
> + if (!elf)
> + goto out;
> +
> + if (elf_kind(elf) != ELF_K_ELF)
> + goto out;
> +
> + if (gelf_getehdr(elf, &ehdr) == NULL)
> + goto out;
> +
> + if (ehdr.e_ident[EI_CLASS] != ELFCLASS64)
> + goto out;
> +
> + for (i = 0; i < ehdr.e_phnum; i++) {
> + GElf_Phdr mem, *phdr;
> + char *data;
> +
> + phdr = gelf_getphdr(elf, i, &mem);
> + if (!phdr)
> + goto out;
> +
> + if (phdr->p_type != PT_NOTE)
> + continue;
> +
> + data = elf_rawfile(elf, &max);
> + if (!data)
> + goto out;
> +
> + if (phdr->p_offset >= max ||
> + (phdr->p_offset + phdr->p_memsz >= max))
> + goto out;
> +
> + err = parse_build_id_buf(data + phdr->p_offset, phdr->p_memsz, build_id);
> + if (err > 0)
> + goto out;
> + }
> +
> +out:
> + if (elf)
> + elf_end(elf);
> + close(fd);
> + return err;
> +}
> diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
> index 53efde0e2998..50b2cc498ba7 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.h
> +++ b/tools/testing/selftests/bpf/trace_helpers.h
> @@ -4,6 +4,9 @@
>
> #include <bpf/libbpf.h>
>
> +#define ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a)-1)
> +#define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask))
nit: I know these are macros, but why would you first use __ALIGN_MASK
and then #define it? swap them?
> +
> struct ksym {
> long addr;
> char *name;
> @@ -23,4 +26,6 @@ void read_trace_pipe(void);
> ssize_t get_uprobe_offset(const void *addr);
> ssize_t get_rel_offset(uintptr_t addr);
>
> +int read_build_id(const char *path, char *build_id);
> +
> #endif
> --
> 2.39.2
>
On Tue, Feb 28, 2023 at 1:33 AM Jiri Olsa <[email protected]> wrote:
>
> Replacing extract_build_id with read_build_id that parses out
> build id directly from elf without using readelf tool.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
small nit below, but looks good. Thanks for clean up!
Acked-by: Andrii Nakryiko <[email protected]>
> .../bpf/prog_tests/stacktrace_build_id.c | 19 ++++++--------
> .../bpf/prog_tests/stacktrace_build_id_nmi.c | 17 +++++--------
> tools/testing/selftests/bpf/test_progs.c | 25 -------------------
> tools/testing/selftests/bpf/test_progs.h | 1 -
> 4 files changed, 13 insertions(+), 49 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> index 9ad09a6c538a..9e4b76ee356f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> @@ -7,13 +7,12 @@ void test_stacktrace_build_id(void)
>
> int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd;
> struct test_stacktrace_build_id *skel;
> - int err, stack_trace_len;
> + int err, stack_trace_len, build_id_size;
> __u32 key, prev_key, val, duration = 0;
> - char buf[256];
> - int i, j;
> + char buf[BPF_BUILD_ID_SIZE];
> struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
> int build_id_matches = 0;
> - int retry = 1;
> + int i, retry = 1;
>
> retry:
> skel = test_stacktrace_build_id__open_and_load();
> @@ -52,9 +51,10 @@ void test_stacktrace_build_id(void)
> "err %d errno %d\n", err, errno))
> goto cleanup;
>
> - err = extract_build_id(buf, 256);
> + build_id_size = read_build_id("./urandom_read", buf);
nit: "urandom_read" vs "./urandom_read" matters only when executing
binary, not when opening a file. So all these "./" just creates
unnecessary confusion, IMO
> + err = build_id_size < 0 ? build_id_size : 0;
>
> - if (CHECK(err, "get build_id with readelf",
> + if (CHECK(err, "read_build_id",
> "err %d errno %d\n", err, errno))
> goto cleanup;
>
> @@ -64,8 +64,6 @@ void test_stacktrace_build_id(void)
> goto cleanup;
>
> do {
> - char build_id[64];
> -
> err = bpf_map_lookup_elem(stackmap_fd, &key, id_offs);
> if (CHECK(err, "lookup_elem from stackmap",
> "err %d, errno %d\n", err, errno))
> @@ -73,10 +71,7 @@ void test_stacktrace_build_id(void)
> for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i)
> if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID &&
> id_offs[i].offset != 0) {
> - for (j = 0; j < 20; ++j)
> - sprintf(build_id + 2 * j, "%02x",
> - id_offs[i].build_id[j] & 0xff);
> - if (strstr(buf, build_id) != NULL)
> + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0)
> build_id_matches = 1;
> }
> prev_key = key;
> diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> index f4ea1a215ce4..8d84149ebcc7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> @@ -28,11 +28,10 @@ void test_stacktrace_build_id_nmi(void)
> .config = PERF_COUNT_HW_CPU_CYCLES,
> };
> __u32 key, prev_key, val, duration = 0;
> - char buf[256];
> - int i, j;
> + char buf[BPF_BUILD_ID_SIZE];
> struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
> - int build_id_matches = 0;
> - int retry = 1;
> + int build_id_matches = 0, build_id_size;
> + int i, retry = 1;
>
> attr.sample_freq = read_perf_max_sample_freq();
>
> @@ -94,7 +93,8 @@ void test_stacktrace_build_id_nmi(void)
> "err %d errno %d\n", err, errno))
> goto cleanup;
>
> - err = extract_build_id(buf, 256);
> + build_id_size = read_build_id("./urandom_read", buf);
> + err = build_id_size < 0 ? build_id_size : 0;
>
> if (CHECK(err, "get build_id with readelf",
> "err %d errno %d\n", err, errno))
> @@ -106,8 +106,6 @@ void test_stacktrace_build_id_nmi(void)
> goto cleanup;
>
> do {
> - char build_id[64];
> -
> err = bpf_map__lookup_elem(skel->maps.stackmap, &key, sizeof(key),
> id_offs, sizeof(id_offs), 0);
> if (CHECK(err, "lookup_elem from stackmap",
> @@ -116,10 +114,7 @@ void test_stacktrace_build_id_nmi(void)
> for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i)
> if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID &&
> id_offs[i].offset != 0) {
> - for (j = 0; j < 20; ++j)
> - sprintf(build_id + 2 * j, "%02x",
> - id_offs[i].build_id[j] & 0xff);
> - if (strstr(buf, build_id) != NULL)
> + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0)
> build_id_matches = 1;
> }
> prev_key = key;
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 6d5e3022c75f..9813d53c4878 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -591,31 +591,6 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
> return err;
> }
>
> -int extract_build_id(char *build_id, size_t size)
> -{
> - FILE *fp;
> - char *line = NULL;
> - size_t len = 0;
> -
> - fp = popen("readelf -n ./urandom_read | grep 'Build ID'", "r");
> - if (fp == NULL)
> - return -1;
> -
> - if (getline(&line, &len, fp) == -1)
> - goto err;
> - pclose(fp);
> -
> - if (len > size)
> - len = size;
> - memcpy(build_id, line, len);
> - build_id[len] = '\0';
> - free(line);
> - return 0;
> -err:
> - pclose(fp);
> - return -1;
> -}
> -
> static int finit_module(int fd, const char *param_values, int flags)
> {
> return syscall(__NR_finit_module, fd, param_values, flags);
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 9fbdc57c5b57..3825c2797a4b 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -404,7 +404,6 @@ static inline void *u64_to_ptr(__u64 ptr)
> int bpf_find_map(const char *test, struct bpf_object *obj, const char *name);
> int compare_map_keys(int map1_fd, int map2_fd);
> int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len);
> -int extract_build_id(char *build_id, size_t size);
> int kern_sync_rcu(void);
> int trigger_module_test_read(int read_sz);
> int trigger_module_test_write(int write_sz);
> --
> 2.39.2
>
On Tue, Feb 28, 2023 at 1:34 AM Jiri Olsa <[email protected]> wrote:
>
> Testing iterator access to build id in vma->vm_file->f_inode
> object by storing each binary with buildid into map and checking
> it against buildid retrieved in user space.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> .../selftests/bpf/prog_tests/bpf_iter.c | 78 +++++++++++++++++++
> .../bpf/progs/bpf_iter_task_vma_buildid.c | 60 ++++++++++++++
> 2 files changed, 138 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 1f02168103dd..c7dd89e7cad0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -33,6 +33,7 @@
> #include "bpf_iter_bpf_link.skel.h"
> #include "bpf_iter_ksym.skel.h"
> #include "bpf_iter_sockmap.skel.h"
> +#include "bpf_iter_task_vma_buildid.skel.h"
>
> static int duration;
>
> @@ -1536,6 +1537,81 @@ static void test_task_vma_dead_task(void)
> bpf_iter_task_vma__destroy(skel);
> }
>
> +#define D_PATH_BUF_SIZE 1024
> +
> +struct build_id {
> + u32 sz;
> + char data[BPF_BUILD_ID_SIZE];
> +};
> +
> +static void test_task_vma_buildid(void)
> +{
> + int err, iter_fd = -1, proc_maps_fd = -1, sz;
> + struct bpf_iter_task_vma_buildid *skel;
> + char key[D_PATH_BUF_SIZE], *prev_key;
> + char build_id[BPF_BUILD_ID_SIZE];
> + int len, files_fd, cnt = 0;
> + struct build_id val;
> + char c;
> +
> + skel = bpf_iter_task_vma_buildid__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "bpf_iter_task_vma_buildid__open_and_load"))
> + return;
> +
> + skel->links.proc_maps = bpf_program__attach_iter(
> + skel->progs.proc_maps, NULL);
> +
> + if (!ASSERT_OK_PTR(skel->links.proc_maps, "bpf_program__attach_iter")) {
> + skel->links.proc_maps = NULL;
> + goto out;
> + }
> +
> + iter_fd = bpf_iter_create(bpf_link__fd(skel->links.proc_maps));
> + if (!ASSERT_GE(iter_fd, 0, "create_iter"))
> + goto out;
> +
> + /* trigger the iterator, there's no output, just map */
> + len = read(iter_fd, &c, 1);
> + ASSERT_EQ(len, 0, "len_check");
> +
> + files_fd = bpf_map__fd(skel->maps.files);
> +
> + prev_key = NULL;
> +
> + while (true) {
> + err = bpf_map_get_next_key(files_fd, prev_key, &key);
> + if (err) {
> + if (errno == ENOENT)
> + err = 0;
> + break;
> + }
> + if (bpf_map_lookup_elem(files_fd, key, &val))
> + break;
> + if (!ASSERT_LE(val.sz, BPF_BUILD_ID_SIZE, "buildid_size"))
> + break;
> +
> + sz = read_build_id(key, build_id);
> + /* If there's an error, the build id is not present or malformed, kernel
> + * should see the same result and bpf program pushed zero build id.
> + */
> + if (sz < 0) {
> + memset(build_id, 0x0, BPF_BUILD_ID_SIZE);
> + sz = BPF_BUILD_ID_SIZE;
> + }
> + ASSERT_EQ(val.sz, sz, "build_id_size");
> + ASSERT_MEMEQ(val.data, build_id, sz, "build_id_data");
> +
> + prev_key = key;
> + cnt++;
> + }
> +
> + printf("checked %d files\n", cnt);
> +out:
> + close(proc_maps_fd);
> + close(iter_fd);
> + bpf_iter_task_vma_buildid__destroy(skel);
> +}
> +
> void test_bpf_sockmap_map_iter_fd(void)
> {
> struct bpf_iter_sockmap *skel;
> @@ -1659,6 +1735,8 @@ void test_bpf_iter(void)
> test_task_vma();
> if (test__start_subtest("task_vma_dead_task"))
> test_task_vma_dead_task();
> + if (test__start_subtest("task_vma_buildid"))
> + test_task_vma_buildid();
> if (test__start_subtest("task_btf"))
> test_task_btf();
> if (test__start_subtest("tcp4"))
> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
> new file mode 100644
> index 000000000000..dc528a4783ec
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "bpf_iter.h"
> +#include "err.h"
> +#include <bpf/bpf_helpers.h>
> +#include <string.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +#define VM_EXEC 0x00000004
> +#define D_PATH_BUF_SIZE 1024
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_HASH);
> + __uint(max_entries, 10000);
> + __type(key, char[D_PATH_BUF_SIZE]);
> + __type(value, struct build_id);
> +} files SEC(".maps");
> +
> +static char path[D_PATH_BUF_SIZE];
> +static struct build_id build_id;
> +
> +SEC("iter/task_vma")
> +int proc_maps(struct bpf_iter__task_vma *ctx)
> +{
> + struct vm_area_struct *vma = ctx->vma;
> + struct seq_file *seq = ctx->meta->seq;
> + struct task_struct *task = ctx->task;
> + unsigned long file_key;
> + struct inode *inode;
> + struct file *file;
> +
> + if (task == (void *)0 || vma == (void *)0)
> + return 0;
> +
> + if (!(vma->vm_flags & VM_EXEC))
> + return 0;
> +
> + file = vma->vm_file;
> + if (!file)
> + return 0;
> +
> + __builtin_memset(path, 0x0, D_PATH_BUF_SIZE);
> + bpf_d_path(&file->f_path, (char *) &path, D_PATH_BUF_SIZE);
> +
> + if (bpf_map_lookup_elem(&files, &path))
> + return 0;
> +
> + inode = file->f_inode;
> + if (IS_ERR_OR_NULL(inode->i_build_id)) {
> + /* On error return empty build id. */
> + __builtin_memset(&build_id.data, 0x0, sizeof(build_id.data));
> + build_id.sz = 20;
let's replace `#define BUILD_ID_SIZE_MAX 20` in
include/linux/buildid.h with `enum { BUILD_ID_SIZE_MAX = 20 };`. This
will "expose" this constant into BTF and thus vmlinux.h, so we won't
have to hard-code anything. BPF users would be grateful as well.
No downsides of doing this.
> + } else {
> + __builtin_memcpy(&build_id, inode->i_build_id, sizeof(*inode->i_build_id));
> + }
> +
> + bpf_map_update_elem(&files, &path, &build_id, 0);
> + return 0;
> +}
> --
> 2.39.2
>
On Tue, Mar 07, 2023 at 05:19:00PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 28, 2023 at 1:33 AM Jiri Olsa <[email protected]> wrote:
> >
> > Try to resolve uprobe/usdt binary path also in current directory,
> > it's used in the test code in following changes.
>
> nope, that's not what shell is doing, so let's not invent new rules
> here. If some tests need something like that, utilize LD_LIBRARY_PATH
> or even better just specify './library.so'
ok, that fixed that:
SEC("uprobe/./liburandom_read.so:urandlib_read_without_sema")
thanks,
jirka
>
> >
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > tools/lib/bpf/libbpf.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 05c4db355f28..f72115e8b7f9 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -10727,17 +10727,19 @@ static const char *arch_specific_lib_paths(void)
> > /* Get full path to program/shared library. */
> > static int resolve_full_path(const char *file, char *result, size_t result_sz)
> > {
> > - const char *search_paths[3] = {};
> > + const char *search_paths[4] = {};
> > int i, perm;
> >
> > if (str_has_sfx(file, ".so") || strstr(file, ".so.")) {
> > search_paths[0] = getenv("LD_LIBRARY_PATH");
> > search_paths[1] = "/usr/lib64:/usr/lib";
> > search_paths[2] = arch_specific_lib_paths();
> > + search_paths[3] = ".";
> > perm = R_OK;
> > } else {
> > search_paths[0] = getenv("PATH");
> > search_paths[1] = "/usr/bin:/usr/sbin";
> > + search_paths[2] = ".";
> > perm = R_OK | X_OK;
> > }
> >
> > --
> > 2.39.2
> >
On Tue, Mar 07, 2023 at 05:22:51PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 28, 2023 at 1:33 AM Jiri Olsa <[email protected]> wrote:
> >
> > Adding read_build_id function that parses out build id from
> > specified binary.
> >
> > It will replace extract_build_id and also be used in following
> > changes.
> >
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > tools/testing/selftests/bpf/trace_helpers.c | 98 +++++++++++++++++++++
> > tools/testing/selftests/bpf/trace_helpers.h | 5 ++
> > 2 files changed, 103 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> > index 09a16a77bae4..c10e16626cd3 100644
> > --- a/tools/testing/selftests/bpf/trace_helpers.c
> > +++ b/tools/testing/selftests/bpf/trace_helpers.c
> > @@ -11,6 +11,9 @@
> > #include <linux/perf_event.h>
> > #include <sys/mman.h>
> > #include "trace_helpers.h"
> > +#include <linux/limits.h>
> > +#include <libelf.h>
> > +#include <gelf.h>
> >
> > #define DEBUGFS "/sys/kernel/debug/tracing/"
> >
> > @@ -230,3 +233,98 @@ ssize_t get_rel_offset(uintptr_t addr)
> > fclose(f);
> > return -EINVAL;
> > }
> > +
> > +static int
> > +parse_build_id_buf(const void *note_start, Elf32_Word note_size,
> > + char *build_id)
> > +{
> > + Elf32_Word note_offs = 0, new_offs;
> > +
> > + while (note_offs + sizeof(Elf32_Nhdr) < note_size) {
> > + Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs);
> > +
> > + if (nhdr->n_type == 3 &&
> > + nhdr->n_namesz == sizeof("GNU") &&
> > + !strcmp((char *)(nhdr + 1), "GNU") &&
> > + nhdr->n_descsz > 0 &&
> > + nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
> > + memcpy(build_id, note_start + note_offs +
> > + ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
> > + nhdr->n_descsz);
> > + memset(build_id + nhdr->n_descsz, 0,
> > + BPF_BUILD_ID_SIZE - nhdr->n_descsz);
>
> I won't count :) but if something fits within 100 characters, please
> keep it on single line
copy&paste from kernel code ;-) I'll reformat that
SNIP
> > diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
> > index 53efde0e2998..50b2cc498ba7 100644
> > --- a/tools/testing/selftests/bpf/trace_helpers.h
> > +++ b/tools/testing/selftests/bpf/trace_helpers.h
> > @@ -4,6 +4,9 @@
> >
> > #include <bpf/libbpf.h>
> >
> > +#define ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a)-1)
> > +#define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask))
>
> nit: I know these are macros, but why would you first use __ALIGN_MASK
> and then #define it? swap them?
same reason as above, I'll swap that
thanks,
jirka
On Tue, Mar 07, 2023 at 05:26:32PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 28, 2023 at 1:33 AM Jiri Olsa <[email protected]> wrote:
> >
> > Replacing extract_build_id with read_build_id that parses out
> > build id directly from elf without using readelf tool.
> >
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
>
> small nit below, but looks good. Thanks for clean up!
>
> Acked-by: Andrii Nakryiko <[email protected]>
>
> > .../bpf/prog_tests/stacktrace_build_id.c | 19 ++++++--------
> > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 17 +++++--------
> > tools/testing/selftests/bpf/test_progs.c | 25 -------------------
> > tools/testing/selftests/bpf/test_progs.h | 1 -
> > 4 files changed, 13 insertions(+), 49 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> > index 9ad09a6c538a..9e4b76ee356f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
> > @@ -7,13 +7,12 @@ void test_stacktrace_build_id(void)
> >
> > int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd;
> > struct test_stacktrace_build_id *skel;
> > - int err, stack_trace_len;
> > + int err, stack_trace_len, build_id_size;
> > __u32 key, prev_key, val, duration = 0;
> > - char buf[256];
> > - int i, j;
> > + char buf[BPF_BUILD_ID_SIZE];
> > struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
> > int build_id_matches = 0;
> > - int retry = 1;
> > + int i, retry = 1;
> >
> > retry:
> > skel = test_stacktrace_build_id__open_and_load();
> > @@ -52,9 +51,10 @@ void test_stacktrace_build_id(void)
> > "err %d errno %d\n", err, errno))
> > goto cleanup;
> >
> > - err = extract_build_id(buf, 256);
> > + build_id_size = read_build_id("./urandom_read", buf);
>
> nit: "urandom_read" vs "./urandom_read" matters only when executing
> binary, not when opening a file. So all these "./" just creates
> unnecessary confusion, IMO
right, I'll remove it also in the other test
thanks,
jirka
>
> > + err = build_id_size < 0 ? build_id_size : 0;
> >
> > - if (CHECK(err, "get build_id with readelf",
> > + if (CHECK(err, "read_build_id",
> > "err %d errno %d\n", err, errno))
> > goto cleanup;
> >
> > @@ -64,8 +64,6 @@ void test_stacktrace_build_id(void)
> > goto cleanup;
> >
> > do {
> > - char build_id[64];
> > -
> > err = bpf_map_lookup_elem(stackmap_fd, &key, id_offs);
> > if (CHECK(err, "lookup_elem from stackmap",
> > "err %d, errno %d\n", err, errno))
> > @@ -73,10 +71,7 @@ void test_stacktrace_build_id(void)
> > for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i)
> > if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID &&
> > id_offs[i].offset != 0) {
> > - for (j = 0; j < 20; ++j)
> > - sprintf(build_id + 2 * j, "%02x",
> > - id_offs[i].build_id[j] & 0xff);
> > - if (strstr(buf, build_id) != NULL)
> > + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0)
> > build_id_matches = 1;
> > }
> > prev_key = key;
> > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> > index f4ea1a215ce4..8d84149ebcc7 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> > @@ -28,11 +28,10 @@ void test_stacktrace_build_id_nmi(void)
> > .config = PERF_COUNT_HW_CPU_CYCLES,
> > };
> > __u32 key, prev_key, val, duration = 0;
> > - char buf[256];
> > - int i, j;
> > + char buf[BPF_BUILD_ID_SIZE];
> > struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
> > - int build_id_matches = 0;
> > - int retry = 1;
> > + int build_id_matches = 0, build_id_size;
> > + int i, retry = 1;
> >
> > attr.sample_freq = read_perf_max_sample_freq();
> >
> > @@ -94,7 +93,8 @@ void test_stacktrace_build_id_nmi(void)
> > "err %d errno %d\n", err, errno))
> > goto cleanup;
> >
> > - err = extract_build_id(buf, 256);
> > + build_id_size = read_build_id("./urandom_read", buf);
> > + err = build_id_size < 0 ? build_id_size : 0;
> >
> > if (CHECK(err, "get build_id with readelf",
> > "err %d errno %d\n", err, errno))
> > @@ -106,8 +106,6 @@ void test_stacktrace_build_id_nmi(void)
> > goto cleanup;
> >
> > do {
> > - char build_id[64];
> > -
> > err = bpf_map__lookup_elem(skel->maps.stackmap, &key, sizeof(key),
> > id_offs, sizeof(id_offs), 0);
> > if (CHECK(err, "lookup_elem from stackmap",
> > @@ -116,10 +114,7 @@ void test_stacktrace_build_id_nmi(void)
> > for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i)
> > if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID &&
> > id_offs[i].offset != 0) {
> > - for (j = 0; j < 20; ++j)
> > - sprintf(build_id + 2 * j, "%02x",
> > - id_offs[i].build_id[j] & 0xff);
> > - if (strstr(buf, build_id) != NULL)
> > + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0)
> > build_id_matches = 1;
> > }
> > prev_key = key;
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 6d5e3022c75f..9813d53c4878 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -591,31 +591,6 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
> > return err;
> > }
> >
> > -int extract_build_id(char *build_id, size_t size)
> > -{
> > - FILE *fp;
> > - char *line = NULL;
> > - size_t len = 0;
> > -
> > - fp = popen("readelf -n ./urandom_read | grep 'Build ID'", "r");
> > - if (fp == NULL)
> > - return -1;
> > -
> > - if (getline(&line, &len, fp) == -1)
> > - goto err;
> > - pclose(fp);
> > -
> > - if (len > size)
> > - len = size;
> > - memcpy(build_id, line, len);
> > - build_id[len] = '\0';
> > - free(line);
> > - return 0;
> > -err:
> > - pclose(fp);
> > - return -1;
> > -}
> > -
> > static int finit_module(int fd, const char *param_values, int flags)
> > {
> > return syscall(__NR_finit_module, fd, param_values, flags);
> > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > index 9fbdc57c5b57..3825c2797a4b 100644
> > --- a/tools/testing/selftests/bpf/test_progs.h
> > +++ b/tools/testing/selftests/bpf/test_progs.h
> > @@ -404,7 +404,6 @@ static inline void *u64_to_ptr(__u64 ptr)
> > int bpf_find_map(const char *test, struct bpf_object *obj, const char *name);
> > int compare_map_keys(int map1_fd, int map2_fd);
> > int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len);
> > -int extract_build_id(char *build_id, size_t size);
> > int kern_sync_rcu(void);
> > int trigger_module_test_read(int read_sz);
> > int trigger_module_test_write(int write_sz);
> > --
> > 2.39.2
> >
On Tue, Mar 07, 2023 at 05:32:17PM -0800, Andrii Nakryiko wrote:
SNIP
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
> > new file mode 100644
> > index 000000000000..dc528a4783ec
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_vma_buildid.c
> > @@ -0,0 +1,60 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include "bpf_iter.h"
> > +#include "err.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <string.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +#define VM_EXEC 0x00000004
> > +#define D_PATH_BUF_SIZE 1024
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_HASH);
> > + __uint(max_entries, 10000);
> > + __type(key, char[D_PATH_BUF_SIZE]);
> > + __type(value, struct build_id);
> > +} files SEC(".maps");
> > +
> > +static char path[D_PATH_BUF_SIZE];
> > +static struct build_id build_id;
> > +
> > +SEC("iter/task_vma")
> > +int proc_maps(struct bpf_iter__task_vma *ctx)
> > +{
> > + struct vm_area_struct *vma = ctx->vma;
> > + struct seq_file *seq = ctx->meta->seq;
> > + struct task_struct *task = ctx->task;
> > + unsigned long file_key;
> > + struct inode *inode;
> > + struct file *file;
> > +
> > + if (task == (void *)0 || vma == (void *)0)
> > + return 0;
> > +
> > + if (!(vma->vm_flags & VM_EXEC))
> > + return 0;
> > +
> > + file = vma->vm_file;
> > + if (!file)
> > + return 0;
> > +
> > + __builtin_memset(path, 0x0, D_PATH_BUF_SIZE);
> > + bpf_d_path(&file->f_path, (char *) &path, D_PATH_BUF_SIZE);
> > +
> > + if (bpf_map_lookup_elem(&files, &path))
> > + return 0;
> > +
> > + inode = file->f_inode;
> > + if (IS_ERR_OR_NULL(inode->i_build_id)) {
> > + /* On error return empty build id. */
> > + __builtin_memset(&build_id.data, 0x0, sizeof(build_id.data));
> > + build_id.sz = 20;
>
> let's replace `#define BUILD_ID_SIZE_MAX 20` in
> include/linux/buildid.h with `enum { BUILD_ID_SIZE_MAX = 20 };`. This
> will "expose" this constant into BTF and thus vmlinux.h, so we won't
> have to hard-code anything. BPF users would be grateful as well.
>
> No downsides of doing this.
ok works nicely.. thanks
jirka