| Message ID | ZEV/EzOM+TJomP66@eg (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] libbpf: Improve version handling when attaching uprobe | expand |
On 4/23/23 11:55 AM, Espen Grindhaug wrote: > This change fixes the handling of versions in elf_find_func_offset. > In the previous implementation, we incorrectly assumed that the Could you give more explanation/example in the commit message what does 'incorrectly' mean here? In which situations the current libbpf implementation will not be correct? > version information would be present in the string found in the > string table. > > We now look up the correct version string in the version symbol > table before constructing the full name and then comparing. > > This patch adds support for both name@version and name@@version to > match output of the various elf parsers. > > Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> [...]
On Wed, Apr 26, 2023 at 02:47:27PM -0700, Yonghong Song wrote: > > > On 4/23/23 11:55 AM, Espen Grindhaug wrote: > > This change fixes the handling of versions in elf_find_func_offset. > > In the previous implementation, we incorrectly assumed that the > > Could you give more explanation/example in the commit message > what does 'incorrectly' mean here? In which situations the > current libbpf implementation will not be correct? > How about something like this? libbpf: Improve version handling when attaching uprobe This change fixes the handling of versions in elf_find_func_offset. For example, let's assume we are trying to attach an uprobe to pthread_create in glibc. Prior to this commit, it would fail with an error message saying 'elf: ambiguous match [...]', this is because there are two entries in the symbol table with that name. $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create 0000000000094cc0 T pthread_create@GLIBC_2.2.5 0000000000094cc0 T pthread_create@@GLIBC_2.34 So we go ahead and modify our code to attach to 'pthread_create@@GLIBC_2.34', and this also fails, but this time with the error 'elf: failed to find symbol [...]'. This fails because we incorrectly assumed that the version information would be present in the string found in the string table, but there is only the string 'pthread_create'. This patch reworks how we compare the symbol name provided by the user if it is qualified with a version (using @ or @@). We now look up the correct version string in the version symbol table before constructing the full name, as also done above by nm, before comparing. > > version information would be present in the string found in the > > string table. > > > > We now look up the correct version string in the version symbol > > table before constructing the full name and then comparing. > > > > This patch adds support for both name@version and name@@version to > > match output of the various elf parsers. > > > > Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> > > [...]
On 4/27/23 12:19 PM, Espen Grindhaug wrote: > On Wed, Apr 26, 2023 at 02:47:27PM -0700, Yonghong Song wrote: >> >> >> On 4/23/23 11:55 AM, Espen Grindhaug wrote: >>> This change fixes the handling of versions in elf_find_func_offset. >>> In the previous implementation, we incorrectly assumed that the >> >> Could you give more explanation/example in the commit message >> what does 'incorrectly' mean here? In which situations the >> current libbpf implementation will not be correct? >> > > How about something like this? > > > libbpf: Improve version handling when attaching uprobe > > This change fixes the handling of versions in elf_find_func_offset. > > For example, let's assume we are trying to attach an uprobe to pthread_create in > glibc. Prior to this commit, it would fail with an error message saying 'elf: > ambiguous match [...]', this is because there are two entries in the symbol > table with that name. > > $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create > 0000000000094cc0 T pthread_create@GLIBC_2.2.5 > 0000000000094cc0 T pthread_create@@GLIBC_2.34 > > So we go ahead and modify our code to attach to 'pthread_create@@GLIBC_2.34', > and this also fails, but this time with the error 'elf: failed to find symbol > [...]'. This fails because we incorrectly assumed that the version information > would be present in the string found in the string table, but there is only the > string 'pthread_create'. I tried one example with my centos8 libpthread library. $ llvm-readelf -s /lib64/libc-2.28.so | grep pthread_cond_signal 39: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 pthread_cond_signal@@GLIBC_2.3.2 40: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 pthread_cond_signal@GLIBC_2.2.5 3160: 0000000000096250 43 FUNC LOCAL DEFAULT 14 __pthread_cond_signal_2_0 3589: 0000000000095f70 43 FUNC LOCAL DEFAULT 14 __pthread_cond_signal 5522: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 pthread_cond_signal@@GLIBC_2.3.2 5545: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 pthread_cond_signal@GLIBC_2.2.5 $ nm -D /lib64/libc-2.28.so | grep pthread_cond_signal 0000000000095f70 T pthread_cond_signal@@GLIBC_2.3.2 0000000000096250 T pthread_cond_signal@GLIBC_2.2.5 $ Note that two pthread_cond_signal functions have different addresses, which is expected as they implemented for different versions. But in your case, > $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create > 0000000000094cc0 T pthread_create@GLIBC_2.2.5 > 0000000000094cc0 T pthread_create@@GLIBC_2.34 Two functions have the same address which is very weird and I suspect some issues here at least needs some investigation. Second, for the symbol table, the following is ELF encoding, typedef struct { Elf64_Word st_name; unsigned char st_info; unsigned char st_other; Elf64_Half st_shndx; Elf64_Addr st_value; Elf64_Xword st_size; } Elf64_Sym; where st_name An index into the object file's symbol string table, which holds the character representations of the symbol names. If the value is nonzero, the value represents a string table index that gives the symbol name. Otherwise, the symbol table entry has no name. So, the function name (including @..., @@...) should be in string table which is the same for the above two pthread_cond_signal symbols. I think it is worthwhile to debug why in your situation pthread_create@GLIBC_2.2.5 and pthread_create@@GLIBC_2.34 do not have them in the string table. > > This patch reworks how we compare the symbol name provided by the user if it is > qualified with a version (using @ or @@). We now look up the correct version > string in the version symbol table before constructing the full name, as also > done above by nm, before comparing. > >>> version information would be present in the string found in the >>> string table. >>> >>> We now look up the correct version string in the version symbol >>> table before constructing the full name and then comparing. >>> >>> This patch adds support for both name@version and name@@version to >>> match output of the various elf parsers. >>> >>> Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> >> >> [...]
On Thu, Apr 27, 2023 at 06:19:29PM -0700, Yonghong Song wrote: > > > On 4/27/23 12:19 PM, Espen Grindhaug wrote: > > On Wed, Apr 26, 2023 at 02:47:27PM -0700, Yonghong Song wrote: > > > > > > > > > On 4/23/23 11:55 AM, Espen Grindhaug wrote: > > > > This change fixes the handling of versions in elf_find_func_offset. > > > > In the previous implementation, we incorrectly assumed that the > > > > > > Could you give more explanation/example in the commit message > > > what does 'incorrectly' mean here? In which situations the > > > current libbpf implementation will not be correct? > > > > > > > How about something like this? > > > > > > libbpf: Improve version handling when attaching uprobe > > > > This change fixes the handling of versions in elf_find_func_offset. > > > > For example, let's assume we are trying to attach an uprobe to pthread_create in > > glibc. Prior to this commit, it would fail with an error message saying 'elf: > > ambiguous match [...]', this is because there are two entries in the symbol > > table with that name. > > > > $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create > > 0000000000094cc0 T pthread_create@GLIBC_2.2.5 > > 0000000000094cc0 T pthread_create@@GLIBC_2.34 > > > > So we go ahead and modify our code to attach to 'pthread_create@@GLIBC_2.34', > > and this also fails, but this time with the error 'elf: failed to find symbol > > [...]'. This fails because we incorrectly assumed that the version information > > would be present in the string found in the string table, but there is only the > > string 'pthread_create'. > > I tried one example with my centos8 libpthread library. > > $ llvm-readelf -s /lib64/libc-2.28.so | grep pthread_cond_signal > 39: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 > pthread_cond_signal@@GLIBC_2.3.2 > 40: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 > pthread_cond_signal@GLIBC_2.2.5 > 3160: 0000000000096250 43 FUNC LOCAL DEFAULT 14 > __pthread_cond_signal_2_0 > 3589: 0000000000095f70 43 FUNC LOCAL DEFAULT 14 > __pthread_cond_signal > 5522: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 > pthread_cond_signal@@GLIBC_2.3.2 > 5545: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 > pthread_cond_signal@GLIBC_2.2.5 > $ nm -D /lib64/libc-2.28.so | grep pthread_cond_signal > 0000000000095f70 T pthread_cond_signal@@GLIBC_2.3.2 > 0000000000096250 T pthread_cond_signal@GLIBC_2.2.5 > $ > > Note that two pthread_cond_signal functions have different addresses, > which is expected as they implemented for different versions. > > But in your case, > > $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create > > 0000000000094cc0 T pthread_create@GLIBC_2.2.5 > > 0000000000094cc0 T pthread_create@@GLIBC_2.34 > > Two functions have the same address which is very weird and I suspect > some issues here at least needs some investigation. > I am no expert on this, but as far as I can tell, this is normal, although much more common on my Ubuntu machine than my Fedora machine. Script to find duplicates: nm -D /usr/lib64/libc-2.33.so | awk ' { addr = $1; symbol = $3; sub(/[@].*$/, "", symbol); if (addr == prev_addr && symbol == prev_symbol) { if (prev_symbol_printed == 0) { print prev_line; prev_symbol_printed = 1; } print; } else { prev_symbol_printed = 0; } prev_addr = addr; prev_symbol = symbol; prev_line = $0; }' > Second, for the symbol table, the following is ELF encoding, > > typedef struct { > Elf64_Word st_name; > unsigned char st_info; > unsigned char st_other; > Elf64_Half st_shndx; > Elf64_Addr st_value; > Elf64_Xword st_size; > } Elf64_Sym; > > where > st_name > > An index into the object file's symbol string table, which holds the > character representations of the symbol names. If the value is nonzero, the > value represents a string table index that gives the symbol name. Otherwise, > the symbol table entry has no name. > > So, the function name (including @..., @@...) should be in string table > which is the same for the above two pthread_cond_signal symbols. > > I think it is worthwhile to debug why in your situation > pthread_create@GLIBC_2.2.5 and pthread_create@@GLIBC_2.34 do not > have them in the string table. > I think you are mistaken here; the strings in the strings table don't contain the version. Take a look at this partial dump of the strings table. $ readelf -W -p .dynstr /usr/lib64/libc-2.33.so String dump of section '.dynstr': [ 1] xdrmem_create [ f] __wctomb_chk [ 1c] getmntent [ 26] __freelocale [ 33] __rawmemchr [ 3f] _IO_vsprintf [ 4c] getutent [ 55] __file_change_detection_for_path (...) [ 350e] memrchr [ 3516] pthread_cond_signal [ 352a] __close (...) [ 61b6] GLIBC_2.2.5 [ 61c2] GLIBC_2.2.6 [ 61ce] GLIBC_2.3 [ 61d8] GLIBC_2.3.2 [ 61e4] GLIBC_2.3.3 As you can see, the strings have no versions, and the version strings themselves are also in this table as entries at the end of the table. > > > > This patch reworks how we compare the symbol name provided by the user if it is > > qualified with a version (using @ or @@). We now look up the correct version > > string in the version symbol table before constructing the full name, as also > > done above by nm, before comparing. > > > > > > version information would be present in the string found in the > > > > string table. > > > > > > > > We now look up the correct version string in the version symbol > > > > table before constructing the full name and then comparing. > > > > > > > > This patch adds support for both name@version and name@@version to > > > > match output of the various elf parsers. > > > > > > > > Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> > > > > > > [...]
On 5/1/23 6:00 AM, Espen Grindhaug wrote: > On Thu, Apr 27, 2023 at 06:19:29PM -0700, Yonghong Song wrote: >> >> >> On 4/27/23 12:19 PM, Espen Grindhaug wrote: >>> On Wed, Apr 26, 2023 at 02:47:27PM -0700, Yonghong Song wrote: >>>> >>>> >>>> On 4/23/23 11:55 AM, Espen Grindhaug wrote: >>>>> This change fixes the handling of versions in elf_find_func_offset. >>>>> In the previous implementation, we incorrectly assumed that the >>>> >>>> Could you give more explanation/example in the commit message >>>> what does 'incorrectly' mean here? In which situations the >>>> current libbpf implementation will not be correct? >>>> >>> >>> How about something like this? >>> >>> >>> libbpf: Improve version handling when attaching uprobe >>> >>> This change fixes the handling of versions in elf_find_func_offset. >>> >>> For example, let's assume we are trying to attach an uprobe to pthread_create in >>> glibc. Prior to this commit, it would fail with an error message saying 'elf: >>> ambiguous match [...]', this is because there are two entries in the symbol >>> table with that name. >>> >>> $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create >>> 0000000000094cc0 T pthread_create@GLIBC_2.2.5 >>> 0000000000094cc0 T pthread_create@@GLIBC_2.34 >>> >>> So we go ahead and modify our code to attach to 'pthread_create@@GLIBC_2.34', >>> and this also fails, but this time with the error 'elf: failed to find symbol >>> [...]'. This fails because we incorrectly assumed that the version information >>> would be present in the string found in the string table, but there is only the >>> string 'pthread_create'. >> >> I tried one example with my centos8 libpthread library. >> >> $ llvm-readelf -s /lib64/libc-2.28.so | grep pthread_cond_signal >> 39: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 >> pthread_cond_signal@@GLIBC_2.3.2 >> 40: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 >> pthread_cond_signal@GLIBC_2.2.5 >> 3160: 0000000000096250 43 FUNC LOCAL DEFAULT 14 >> __pthread_cond_signal_2_0 >> 3589: 0000000000095f70 43 FUNC LOCAL DEFAULT 14 >> __pthread_cond_signal >> 5522: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 >> pthread_cond_signal@@GLIBC_2.3.2 >> 5545: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 >> pthread_cond_signal@GLIBC_2.2.5 >> $ nm -D /lib64/libc-2.28.so | grep pthread_cond_signal >> 0000000000095f70 T pthread_cond_signal@@GLIBC_2.3.2 >> 0000000000096250 T pthread_cond_signal@GLIBC_2.2.5 >> $ >> >> Note that two pthread_cond_signal functions have different addresses, >> which is expected as they implemented for different versions. >> >> But in your case, >>> $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create >>> 0000000000094cc0 T pthread_create@GLIBC_2.2.5 >>> 0000000000094cc0 T pthread_create@@GLIBC_2.34 >> >> Two functions have the same address which is very weird and I suspect >> some issues here at least needs some investigation. >> > > I am no expert on this, but as far as I can tell, this is normal, > although much more common on my Ubuntu machine than my Fedora machine. > > Script to find duplicates: > > nm -D /usr/lib64/libc-2.33.so | awk ' > { > addr = $1; > symbol = $3; > sub(/[@].*$/, "", symbol); > > if (addr == prev_addr && symbol == prev_symbol) { > if (prev_symbol_printed == 0) { > print prev_line; > prev_symbol_printed = 1; > } > print; > } else { > prev_symbol_printed = 0; > } > prev_addr = addr; > prev_symbol = symbol; > prev_line = $0; > }' > > >> Second, for the symbol table, the following is ELF encoding, >> >> typedef struct { >> Elf64_Word st_name; >> unsigned char st_info; >> unsigned char st_other; >> Elf64_Half st_shndx; >> Elf64_Addr st_value; >> Elf64_Xword st_size; >> } Elf64_Sym; >> >> where >> st_name >> >> An index into the object file's symbol string table, which holds the >> character representations of the symbol names. If the value is nonzero, the >> value represents a string table index that gives the symbol name. Otherwise, >> the symbol table entry has no name. >> >> So, the function name (including @..., @@...) should be in string table >> which is the same for the above two pthread_cond_signal symbols. >> >> I think it is worthwhile to debug why in your situation >> pthread_create@GLIBC_2.2.5 and pthread_create@@GLIBC_2.34 do not >> have them in the string table. >> > > I think you are mistaken here; the strings in the strings table don't contain > the version. Take a look at this partial dump of the strings table. > > $ readelf -W -p .dynstr /usr/lib64/libc-2.33.so > > String dump of section '.dynstr': > [ 1] xdrmem_create > [ f] __wctomb_chk > [ 1c] getmntent > [ 26] __freelocale > [ 33] __rawmemchr > [ 3f] _IO_vsprintf > [ 4c] getutent > [ 55] __file_change_detection_for_path > (...) > [ 350e] memrchr > [ 3516] pthread_cond_signal > [ 352a] __close > (...) > [ 61b6] GLIBC_2.2.5 > [ 61c2] GLIBC_2.2.6 > [ 61ce] GLIBC_2.3 > [ 61d8] GLIBC_2.3.2 > [ 61e4] GLIBC_2.3.3 > > As you can see, the strings have no versions, and the version strings > themselves are also in this table as entries at the end of the table. I see you search .dynstr section. Do you think whether we should search .strtab instead since it contains versioned symbols? > >>> >>> This patch reworks how we compare the symbol name provided by the user if it is >>> qualified with a version (using @ or @@). We now look up the correct version >>> string in the version symbol table before constructing the full name, as also >>> done above by nm, before comparing. >>> >>>>> version information would be present in the string found in the >>>>> string table. >>>>> >>>>> We now look up the correct version string in the version symbol >>>>> table before constructing the full name and then comparing. >>>>> >>>>> This patch adds support for both name@version and name@@version to >>>>> match output of the various elf parsers. >>>>> >>>>> Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> >>>> >>>> [...]
On Mon, May 01, 2023 at 08:23:35AM -0700, Yonghong Song wrote: > > > On 5/1/23 6:00 AM, Espen Grindhaug wrote: > > On Thu, Apr 27, 2023 at 06:19:29PM -0700, Yonghong Song wrote: > > > > > > > > > On 4/27/23 12:19 PM, Espen Grindhaug wrote: > > > > On Wed, Apr 26, 2023 at 02:47:27PM -0700, Yonghong Song wrote: > > > > > > > > > > > > > > > On 4/23/23 11:55 AM, Espen Grindhaug wrote: > > > > > > This change fixes the handling of versions in elf_find_func_offset. > > > > > > In the previous implementation, we incorrectly assumed that the > > > > > > > > > > Could you give more explanation/example in the commit message > > > > > what does 'incorrectly' mean here? In which situations the > > > > > current libbpf implementation will not be correct? > > > > > > > > > > > > > How about something like this? > > > > > > > > > > > > libbpf: Improve version handling when attaching uprobe > > > > > > > > This change fixes the handling of versions in elf_find_func_offset. > > > > > > > > For example, let's assume we are trying to attach an uprobe to pthread_create in > > > > glibc. Prior to this commit, it would fail with an error message saying 'elf: > > > > ambiguous match [...]', this is because there are two entries in the symbol > > > > table with that name. > > > > > > > > $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create > > > > 0000000000094cc0 T pthread_create@GLIBC_2.2.5 > > > > 0000000000094cc0 T pthread_create@@GLIBC_2.34 > > > > > > > > So we go ahead and modify our code to attach to 'pthread_create@@GLIBC_2.34', > > > > and this also fails, but this time with the error 'elf: failed to find symbol > > > > [...]'. This fails because we incorrectly assumed that the version information > > > > would be present in the string found in the string table, but there is only the > > > > string 'pthread_create'. > > > > > > I tried one example with my centos8 libpthread library. > > > > > > $ llvm-readelf -s /lib64/libc-2.28.so | grep pthread_cond_signal > > > 39: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 > > > pthread_cond_signal@@GLIBC_2.3.2 > > > 40: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 > > > pthread_cond_signal@GLIBC_2.2.5 > > > 3160: 0000000000096250 43 FUNC LOCAL DEFAULT 14 > > > __pthread_cond_signal_2_0 > > > 3589: 0000000000095f70 43 FUNC LOCAL DEFAULT 14 > > > __pthread_cond_signal > > > 5522: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 > > > pthread_cond_signal@@GLIBC_2.3.2 > > > 5545: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 > > > pthread_cond_signal@GLIBC_2.2.5 > > > $ nm -D /lib64/libc-2.28.so | grep pthread_cond_signal > > > 0000000000095f70 T pthread_cond_signal@@GLIBC_2.3.2 > > > 0000000000096250 T pthread_cond_signal@GLIBC_2.2.5 > > > $ > > > > > > Note that two pthread_cond_signal functions have different addresses, > > > which is expected as they implemented for different versions. > > > > > > But in your case, > > > > $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create > > > > 0000000000094cc0 T pthread_create@GLIBC_2.2.5 > > > > 0000000000094cc0 T pthread_create@@GLIBC_2.34 > > > > > > Two functions have the same address which is very weird and I suspect > > > some issues here at least needs some investigation. > > > > > > > I am no expert on this, but as far as I can tell, this is normal, > > although much more common on my Ubuntu machine than my Fedora machine. > > > > Script to find duplicates: > > > > nm -D /usr/lib64/libc-2.33.so | awk ' > > { > > addr = $1; > > symbol = $3; > > sub(/[@].*$/, "", symbol); > > > > if (addr == prev_addr && symbol == prev_symbol) { > > if (prev_symbol_printed == 0) { > > print prev_line; > > prev_symbol_printed = 1; > > } > > print; > > } else { > > prev_symbol_printed = 0; > > } > > prev_addr = addr; > > prev_symbol = symbol; > > prev_line = $0; > > }' > > > > > > > Second, for the symbol table, the following is ELF encoding, > > > > > > typedef struct { > > > Elf64_Word st_name; > > > unsigned char st_info; > > > unsigned char st_other; > > > Elf64_Half st_shndx; > > > Elf64_Addr st_value; > > > Elf64_Xword st_size; > > > } Elf64_Sym; > > > > > > where > > > st_name > > > > > > An index into the object file's symbol string table, which holds the > > > character representations of the symbol names. If the value is nonzero, the > > > value represents a string table index that gives the symbol name. Otherwise, > > > the symbol table entry has no name. > > > > > > So, the function name (including @..., @@...) should be in string table > > > which is the same for the above two pthread_cond_signal symbols. > > > > > > I think it is worthwhile to debug why in your situation > > > pthread_create@GLIBC_2.2.5 and pthread_create@@GLIBC_2.34 do not > > > have them in the string table. > > > > > > > I think you are mistaken here; the strings in the strings table don't contain > > the version. Take a look at this partial dump of the strings table. > > > > $ readelf -W -p .dynstr /usr/lib64/libc-2.33.so > > > > String dump of section '.dynstr': > > [ 1] xdrmem_create > > [ f] __wctomb_chk > > [ 1c] getmntent > > [ 26] __freelocale > > [ 33] __rawmemchr > > [ 3f] _IO_vsprintf > > [ 4c] getutent > > [ 55] __file_change_detection_for_path > > (...) > > [ 350e] memrchr > > [ 3516] pthread_cond_signal > > [ 352a] __close > > (...) > > [ 61b6] GLIBC_2.2.5 > > [ 61c2] GLIBC_2.2.6 > > [ 61ce] GLIBC_2.3 > > [ 61d8] GLIBC_2.3.2 > > [ 61e4] GLIBC_2.3.3 > > > > As you can see, the strings have no versions, and the version strings > > themselves are also in this table as entries at the end of the table. > > I see you search .dynstr section. Do you think whether we should > search .strtab instead since it contains versioned symbols? > I searched .dynstr since my libc files only have that section, but I do see your point. If const char *binary_path points to an executable and not an .so file, then we would find some versioned symbols in the .strtab section. However, since libbpf supports using the .so as binary_path, would we not need the functionality to build the complete name regardless? Adding a check to not build the full name if it already contains an '@' is probably a good idea, though. > > > > > > > > > > This patch reworks how we compare the symbol name provided by the user if it is > > > > qualified with a version (using @ or @@). We now look up the correct version > > > > string in the version symbol table before constructing the full name, as also > > > > done above by nm, before comparing. > > > > > > > > > > version information would be present in the string found in the > > > > > > string table. > > > > > > > > > > > > We now look up the correct version string in the version symbol > > > > > > table before constructing the full name and then comparing. > > > > > > > > > > > > This patch adds support for both name@version and name@@version to > > > > > > match output of the various elf parsers. > > > > > > > > > > > > Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> > > > > > > > > > > [...]
On 5/1/23 9:30 AM, Espen Grindhaug wrote: > On Mon, May 01, 2023 at 08:23:35AM -0700, Yonghong Song wrote: >> >> >> On 5/1/23 6:00 AM, Espen Grindhaug wrote: >>> On Thu, Apr 27, 2023 at 06:19:29PM -0700, Yonghong Song wrote: >>>> >>>> >>>> On 4/27/23 12:19 PM, Espen Grindhaug wrote: >>>>> On Wed, Apr 26, 2023 at 02:47:27PM -0700, Yonghong Song wrote: >>>>>> >>>>>> >>>>>> On 4/23/23 11:55 AM, Espen Grindhaug wrote: >>>>>>> This change fixes the handling of versions in elf_find_func_offset. >>>>>>> In the previous implementation, we incorrectly assumed that the >>>>>> >>>>>> Could you give more explanation/example in the commit message >>>>>> what does 'incorrectly' mean here? In which situations the >>>>>> current libbpf implementation will not be correct? >>>>>> >>>>> >>>>> How about something like this? >>>>> >>>>> >>>>> libbpf: Improve version handling when attaching uprobe >>>>> >>>>> This change fixes the handling of versions in elf_find_func_offset. >>>>> >>>>> For example, let's assume we are trying to attach an uprobe to pthread_create in >>>>> glibc. Prior to this commit, it would fail with an error message saying 'elf: >>>>> ambiguous match [...]', this is because there are two entries in the symbol >>>>> table with that name. >>>>> >>>>> $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create >>>>> 0000000000094cc0 T pthread_create@GLIBC_2.2.5 >>>>> 0000000000094cc0 T pthread_create@@GLIBC_2.34 >>>>> >>>>> So we go ahead and modify our code to attach to 'pthread_create@@GLIBC_2.34', >>>>> and this also fails, but this time with the error 'elf: failed to find symbol >>>>> [...]'. This fails because we incorrectly assumed that the version information >>>>> would be present in the string found in the string table, but there is only the >>>>> string 'pthread_create'. >>>> >>>> I tried one example with my centos8 libpthread library. >>>> >>>> $ llvm-readelf -s /lib64/libc-2.28.so | grep pthread_cond_signal >>>> 39: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 >>>> pthread_cond_signal@@GLIBC_2.3.2 >>>> 40: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 >>>> pthread_cond_signal@GLIBC_2.2.5 >>>> 3160: 0000000000096250 43 FUNC LOCAL DEFAULT 14 >>>> __pthread_cond_signal_2_0 >>>> 3589: 0000000000095f70 43 FUNC LOCAL DEFAULT 14 >>>> __pthread_cond_signal >>>> 5522: 0000000000095f70 43 FUNC GLOBAL DEFAULT 14 >>>> pthread_cond_signal@@GLIBC_2.3.2 >>>> 5545: 0000000000096250 43 FUNC GLOBAL DEFAULT 14 >>>> pthread_cond_signal@GLIBC_2.2.5 >>>> $ nm -D /lib64/libc-2.28.so | grep pthread_cond_signal >>>> 0000000000095f70 T pthread_cond_signal@@GLIBC_2.3.2 >>>> 0000000000096250 T pthread_cond_signal@GLIBC_2.2.5 >>>> $ >>>> >>>> Note that two pthread_cond_signal functions have different addresses, >>>> which is expected as they implemented for different versions. >>>> >>>> But in your case, >>>>> $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep pthread_create >>>>> 0000000000094cc0 T pthread_create@GLIBC_2.2.5 >>>>> 0000000000094cc0 T pthread_create@@GLIBC_2.34 >>>> >>>> Two functions have the same address which is very weird and I suspect >>>> some issues here at least needs some investigation. >>>> >>> >>> I am no expert on this, but as far as I can tell, this is normal, >>> although much more common on my Ubuntu machine than my Fedora machine. >>> >>> Script to find duplicates: >>> >>> nm -D /usr/lib64/libc-2.33.so | awk ' >>> { >>> addr = $1; >>> symbol = $3; >>> sub(/[@].*$/, "", symbol); >>> >>> if (addr == prev_addr && symbol == prev_symbol) { >>> if (prev_symbol_printed == 0) { >>> print prev_line; >>> prev_symbol_printed = 1; >>> } >>> print; >>> } else { >>> prev_symbol_printed = 0; >>> } >>> prev_addr = addr; >>> prev_symbol = symbol; >>> prev_line = $0; >>> }' >>> >>> >>>> Second, for the symbol table, the following is ELF encoding, >>>> >>>> typedef struct { >>>> Elf64_Word st_name; >>>> unsigned char st_info; >>>> unsigned char st_other; >>>> Elf64_Half st_shndx; >>>> Elf64_Addr st_value; >>>> Elf64_Xword st_size; >>>> } Elf64_Sym; >>>> >>>> where >>>> st_name >>>> >>>> An index into the object file's symbol string table, which holds the >>>> character representations of the symbol names. If the value is nonzero, the >>>> value represents a string table index that gives the symbol name. Otherwise, >>>> the symbol table entry has no name. >>>> >>>> So, the function name (including @..., @@...) should be in string table >>>> which is the same for the above two pthread_cond_signal symbols. >>>> >>>> I think it is worthwhile to debug why in your situation >>>> pthread_create@GLIBC_2.2.5 and pthread_create@@GLIBC_2.34 do not >>>> have them in the string table. >>>> >>> >>> I think you are mistaken here; the strings in the strings table don't contain >>> the version. Take a look at this partial dump of the strings table. >>> >>> $ readelf -W -p .dynstr /usr/lib64/libc-2.33.so >>> >>> String dump of section '.dynstr': >>> [ 1] xdrmem_create >>> [ f] __wctomb_chk >>> [ 1c] getmntent >>> [ 26] __freelocale >>> [ 33] __rawmemchr >>> [ 3f] _IO_vsprintf >>> [ 4c] getutent >>> [ 55] __file_change_detection_for_path >>> (...) >>> [ 350e] memrchr >>> [ 3516] pthread_cond_signal >>> [ 352a] __close >>> (...) >>> [ 61b6] GLIBC_2.2.5 >>> [ 61c2] GLIBC_2.2.6 >>> [ 61ce] GLIBC_2.3 >>> [ 61d8] GLIBC_2.3.2 >>> [ 61e4] GLIBC_2.3.3 >>> >>> As you can see, the strings have no versions, and the version strings >>> themselves are also in this table as entries at the end of the table. >> >> I see you search .dynstr section. Do you think whether we should >> search .strtab instead since it contains versioned symbols? >> > > I searched .dynstr since my libc files only have that section, but I do see > your point. If const char *binary_path points to an executable and not an > .so file, then we would find some versioned symbols in the .strtab section. > However, since libbpf supports using the .so as binary_path, would we not > need the functionality to build the complete name regardless? Okay, so you do not have .strtab section, the section probably removed with `llvm-strip --strip-all <binary>`. In this particular case, I think your approach to search SHT_GNU_versym and SHT_GNU_verdef for versioned symbols probably is the right choice. Please do add such information in the commit message. > > Adding a check to not build the full name if it already contains an '@' is > probably a good idea, though. If you search strtab, you will find name with '@', but this won't be the case if you using SHT_GNU_versym/SHT_GNU_verdef. Since both dynstr and strtab are searched, I guess adding this check is a good idea if the version in strtab case is not NULL. > >>> >>>>> >>>>> This patch reworks how we compare the symbol name provided by the user if it is >>>>> qualified with a version (using @ or @@). We now look up the correct version >>>>> string in the version symbol table before constructing the full name, as also >>>>> done above by nm, before comparing. >>>>> >>>>>>> version information would be present in the string found in the >>>>>>> string table. >>>>>>> >>>>>>> We now look up the correct version string in the version symbol >>>>>>> table before constructing the full name and then comparing. >>>>>>> >>>>>>> This patch adds support for both name@version and name@@version to >>>>>>> match output of the various elf parsers. >>>>>>> >>>>>>> Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> >>>>>> >>>>>> [...]
On Sun, Apr 23, 2023 at 11:55 AM Espen Grindhaug <espen.grindhaug@gmail.com> wrote: > > This change fixes the handling of versions in elf_find_func_offset. > In the previous implementation, we incorrectly assumed that the > version information would be present in the string found in the > string table. > > We now look up the correct version string in the version symbol > table before constructing the full name and then comparing. > > This patch adds support for both name@version and name@@version to > match output of the various elf parsers. > > Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> > --- > Changes since v1: > - Added test > --- > tools/lib/bpf/libbpf.c | 148 +++++++++++++++--- > .../selftests/bpf/prog_tests/attach_probe.c | 37 +++++ > .../selftests/bpf/progs/test_attach_probe.c | 15 ++ > 3 files changed, 181 insertions(+), 19 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 49cd304ae3bc..ef5e11ce6241 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -10620,31 +10620,94 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe, > } > > /* Return next ELF section of sh_type after scn, or first of that type if scn is NULL. */ > -static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn) > +static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn, size_t *idx) > { > while ((scn = elf_nextscn(elf, scn)) != NULL) { > GElf_Shdr sh; > > if (!gelf_getshdr(scn, &sh)) > continue; > - if (sh.sh_type == sh_type) > + if (sh.sh_type == sh_type) { > + if (idx) > + *idx = sh.sh_link; > return scn; > + } > + } > + return NULL; > +} > + > +static Elf_Data *elf_find_data_by_type(Elf *elf, int sh_type, size_t *idx) > +{ > + Elf_Scn *scn = elf_find_next_scn_by_type(elf, sh_type, NULL, idx); > + > + if (scn) > + return elf_getdata(scn, NULL); > + > + return NULL; > +} > + > +static Elf64_Verdef *elf_verdef_by_offset(Elf_Data *data, size_t offset) > +{ > + if (offset + sizeof(Elf64_Verdef) > data->d_size) > + return NULL; > + > + return (Elf64_Verdef *)((char *) data->d_buf + offset); > +} > + > +static Elf64_Versym *elf_versym_by_idx(Elf_Data *data, size_t idx) > +{ > + if (idx >= data->d_size / sizeof(Elf64_Versym)) > + return NULL; > + > + return (Elf64_Versym *)(data->d_buf + idx * sizeof(Elf64_Versym)); > +} > + > +static Elf64_Verdaux *elf_verdaux_by_offset(Elf_Data *data, size_t offset) > +{ > + if (offset + sizeof(Elf64_Verdaux) > data->d_size) > + return NULL; > + > + return (Elf64_Verdaux *)((char *) data->d_buf + offset); > +} > + > +#define ELF_VERSYM_HIDDEN 0x8000 > +#define ELF_VERSYM_IDX_MASK 0x7fff > + > +static Elf64_Verdaux *elf_get_verdaux_by_versym(Elf_Data *verdef_data, Elf64_Versym *versym) > +{ > + size_t offset = 0; > + > + while (offset + sizeof(Elf64_Verdef) <= verdef_data->d_size) { > + Elf64_Verdef *verdef = elf_verdef_by_offset(verdef_data, offset); > + > + if (!verdef) > + break; > + > + if (verdef->vd_ndx == (*versym & ELF_VERSYM_IDX_MASK)) > + return elf_verdaux_by_offset(verdef_data, offset + verdef->vd_aux); > + > + if (verdef->vd_next == 0) > + break; > + > + offset += verdef->vd_next; > } > return NULL; > } So all of the above is quite cryptic and unclear. I, for example, don't really know how symbol versioning information is laid out in ELF, and it's very hard to follow the logic here. Please add comments explaining the format and how different sections are related to each other. > > /* Find offset of function name in the provided ELF object. "binary_path" is > * the path to the ELF binary represented by "elf", and only used for error > - * reporting matters. "name" matches symbol name or name@@LIB for library > - * functions. > + * reporting matters. "name" matches symbol name, name@LIB or name@@LIB for > + * library functions. > */ > static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name) > { > int i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB }; > bool is_shared_lib, is_name_qualified; > long ret = -ENOENT; > - size_t name_len; > GElf_Ehdr ehdr; > + Elf_Data *versym_data = NULL; > + Elf_Data *verdef_data = NULL; > + size_t verdef_stridx = 0; > > if (!gelf_getehdr(elf, &ehdr)) { > pr_warn("elf: failed to get ehdr from %s: %s\n", binary_path, elf_errmsg(-1)); > @@ -10654,9 +10717,12 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char * > /* for shared lib case, we do not need to calculate relative offset */ > is_shared_lib = ehdr.e_type == ET_DYN; > > - name_len = strlen(name); > - /* Does name specify "@@LIB"? */ > - is_name_qualified = strstr(name, "@@") != NULL; > + /* Does name specify "@@LIB" or "@LIB"? */ > + is_name_qualified = strstr(name, "@") != NULL; > + > + /* Extract version definition and version symbol table */ > + versym_data = elf_find_data_by_type(elf, SHT_GNU_versym, NULL); > + verdef_data = elf_find_data_by_type(elf, SHT_GNU_verdef, &verdef_stridx); > > /* Search SHT_DYNSYM, SHT_SYMTAB for symbol. This search order is used because if > * a binary is stripped, it may only have SHT_DYNSYM, and a fully-statically > @@ -10671,10 +10737,10 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char * > const char *sname; > GElf_Shdr sh; > > - scn = elf_find_next_scn_by_type(elf, sh_types[i], NULL); > + scn = elf_find_next_scn_by_type(elf, sh_types[i], NULL, NULL); > if (!scn) { > pr_debug("elf: failed to find symbol table ELF sections in '%s'\n", > - binary_path); > + binary_path); please don't change unrelated lines of code > continue; > } > if (!gelf_getshdr(scn, &sh)) > @@ -10705,16 +10771,60 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char * > if (!sname) > continue; > > - curr_bind = GELF_ST_BIND(sym.st_info); > + if (is_name_qualified) { > + Elf64_Versym *versym; > + Elf64_Verdaux *verdaux; why are we assuming 64-bit binaries? > + int res; > + char full_name[256]; > > - /* User can specify func, func@@LIB or func@@LIB_VERSION. */ > - if (strncmp(sname, name, name_len) != 0) > - continue; > - /* ...but we don't want a search for "foo" to match 'foo2" also, so any > - * additional characters in sname should be of the form "@@LIB". > - */ > - if (!is_name_qualified && sname[name_len] != '\0' && sname[name_len] != '@') > - continue; > + /* check that name at least starts with sname before building > + * the full name > + */ > + if (strncmp(name, sname, strlen(sname)) != 0) > + continue; > + > + if (!versym_data || !verdef_data) { > + pr_warn("elf: failed to find version definition or version symbol table in '%s'\n", > + binary_path); > + break; > + } > + > + versym = elf_versym_by_idx(versym_data, idx); > + if (!versym) { > + pr_warn("elf: failed to lookup versym for '%s' in '%s'\n", > + sname, binary_path); > + continue; > + } > + > + verdaux = elf_get_verdaux_by_versym(verdef_data, versym); > + if (!verdaux) { > + pr_warn("elf: failed to lookup verdaux for '%s' in '%s'\n", > + sname, binary_path); > + continue; > + } > + > + res = snprintf(full_name, sizeof(full_name), > + (*versym & ELF_VERSYM_HIDDEN) ? "%s@%s" : > + "%s@@%s", > + sname, > + elf_strptr(elf, verdef_stridx, > + verdaux->vda_name)); > + > + if (res < 0 || res >= sizeof(full_name)) { > + pr_warn("elf: failed to build full name for '%s' in '%s'\n", > + sname, binary_path); > + continue; > + } maybe this version fetching part can be extracted into a helper to keep the main logic more straightforward? > + > + if (strcmp(full_name, name) != 0) > + continue; > + } else { > + /* If name is not qualified, we want to match the symbol name */ > + if (strcmp(sname, name) != 0) > + continue; > + } > + > + curr_bind = GELF_ST_BIND(sym.st_info); > > if (ret >= 0) { > /* handle multiple matches */ > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c > index 7175af39134f..c3f33f7e9d12 100644 > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c please split selftests into a separate patch Also, it's not great that we rely on specific GLIBC versions for testing. There is no need to rely on glibc here at all. We have our own test executable and shared library, urandom_read and liburandom_read.so, which are used exactly for these sort of tests. While I was trying to understand all this versioning some more, I've wrote a bunch of code already. Please use it as a base and switch tests to these new urandom_api() (v1 and v2) functions: diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index c49e5403ad0e..ff8685aac28f 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -181,11 +181,12 @@ endif # Filter out -static for liburandom_read.so and its dependent targets so that static builds # do not fail. Static builds leave urandom_read relying on system-wide shared libraries. -$(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c +$(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c liburandom_read.map $(call msg,LIB,,$@) $(Q)$(CLANG) $(filter-out -static,$(CFLAGS) $(LDFLAGS)) \ - $^ $(filter-out -static,$(LDLIBS)) \ + $(filter %.c,$^) $(filter-out -static,$(LDLIBS)) \ -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \ + -Wl,--version-script=liburandom_read.map \ -fPIC -shared -o $@ $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so diff --git a/tools/testing/selftests/bpf/urandom_read.c b/tools/testing/selftests/bpf/urandom_read.c index e92644d0fa75..36ccfeb63443 100644 --- a/tools/testing/selftests/bpf/urandom_read.c +++ b/tools/testing/selftests/bpf/urandom_read.c @@ -21,6 +21,10 @@ void urand_read_without_sema(int iter_num, int iter_cnt, int read_sz); void urandlib_read_with_sema(int iter_num, int iter_cnt, int read_sz); void urandlib_read_without_sema(int iter_num, int iter_cnt, int read_sz); +int urandlib_api(void); +__asm__(".symver urandlib_api_old,urandlib_api@LIBURANDOM_READ_1.0.0"); +int urandlib_api_old(void); + unsigned short urand_read_with_sema_semaphore SEC(".probes"); static __attribute__((noinline)) @@ -83,6 +87,9 @@ int main(int argc, char *argv[]) urandom_read(fd, count); + urandlib_api(); + urandlib_api_old(); + close(fd); return 0; } diff --git a/tools/testing/selftests/bpf/urandom_read_lib1.c b/tools/testing/selftests/bpf/urandom_read_lib1.c index 86186e24b740..eb02b8a9032c 100644 --- a/tools/testing/selftests/bpf/urandom_read_lib1.c +++ b/tools/testing/selftests/bpf/urandom_read_lib1.c @@ -11,3 +11,37 @@ void urandlib_read_with_sema(int iter_num, int iter_cnt, int read_sz) { STAP_PROBE3(urandlib, read_with_sema, iter_num, iter_cnt, read_sz); } + +/* Symbol versioning is different between static and shared library. + * Properly versioned symbols are needed for shared library, but + * only the symbol of the new version is needed for static library. + * Starting with GNU C 10, use symver attribute instead of .symver assembler + * directive, which works better with GCC LTO builds. + */ +#if defined(__GNUC__) && __GNUC__ >= 10 + +#define DEFAULT_VERSION(internal_name, api_name, version) \ + __attribute__((symver(#api_name "@@" #version))) +#define OMPAT_VERSION(internal_name, api_name, version) \ + __attribute__((symver(#api_name "@" #version))) + +#else + +#define COMPAT_VERSION(internal_name, api_name, version) \ + asm(".symver " #internal_name "," #api_name "@" #version); +#define DEFAULT_VERSION(internal_name, api_name, version) \ + asm(".symver " #internal_name "," #api_name "@@" #version); + +#endif + +COMPAT_VERSION(urandlib_api_v1, urandlib_api, LIBURANDOM_READ_1.0.0) +int urandlib_api_v1(void) +{ + return 1; +} + +DEFAULT_VERSION(urandlib_api_v2, urandlib_api, LIBURANDOM_READ_2.0.0) +int urandlib_api_v2(void) +{ + return 2; +} > @@ -192,6 +192,41 @@ static void test_uprobe_lib(struct test_attach_probe *skel) > ASSERT_EQ(skel->bss->uretprobe_byname2_res, 8, "check_uretprobe_byname2_res"); > } > > +static void test_uprobe_lib_with_versions(struct test_attach_probe *skel) > +{ > + DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts); > + char absolute_path[256]; > + > + /* test attach with a versioned name. > + * realpath has two implementations in libc, only the default version will be used. > + */ > + uprobe_opts.func_name = "realpath@@GLIBC_2.3"; > + uprobe_opts.retprobe = false; > + skel->links.handle_uprobe_byversionedname_a = > + bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byversionedname_a, > + 0 /* this pid */, > + "libc.so.6", > + 0, &uprobe_opts); > + if (!ASSERT_OK_PTR(skel->links.handle_uprobe_byversionedname_a, "attach_handle_uprobe_byversionedname_a")) > + return; > + > + uprobe_opts.func_name = "realpath@GLIBC_2.2.5"; > + uprobe_opts.retprobe = false; > + skel->links.handle_uprobe_byversionedname_b = > + bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byversionedname_b, > + 0 /* this pid */, > + "libc.so.6", > + 0, &uprobe_opts); > + if (!ASSERT_OK_PTR(skel->links.handle_uprobe_byversionedname_b, "attach_handle_uprobe_byversionedname_b")) > + return; > + > + /* trigger & validate probes */ > + realpath("/", absolute_path); > + > + ASSERT_EQ(skel->bss->uprobe_byversionedname_a_res, 13, "check_uprobe_byversionedname_a_res"); > + ASSERT_NEQ(skel->bss->uprobe_byversionedname_b_res, 14, "check_uprobe_byversionedname_b_res"); > +} > + > static void test_uprobe_ref_ctr(struct test_attach_probe *skel) > { > DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts); > @@ -316,6 +351,8 @@ void test_attach_probe(void) > test_kprobe_sleepable(); > if (test__start_subtest("uprobe-lib")) > test_uprobe_lib(skel); > + if (test__start_subtest("uprobe-lib-with-versions")) > + test_uprobe_lib_with_versions(skel); > if (test__start_subtest("uprobe-sleepable")) > test_uprobe_sleepable(skel); > if (test__start_subtest("uprobe-ref_ctr")) > diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c > index 68466a6ad18c..079b58901ff8 100644 > --- a/tools/testing/selftests/bpf/progs/test_attach_probe.c > +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c > @@ -17,6 +17,8 @@ int uprobe_byname3_sleepable_res = 0; > int uprobe_byname3_res = 0; > int uretprobe_byname3_sleepable_res = 0; > int uretprobe_byname3_res = 0; > +int uprobe_byversionedname_a_res = 0; > +int uprobe_byversionedname_b_res = 0; > void *user_ptr = 0; > > SEC("ksyscall/nanosleep") > @@ -121,5 +123,18 @@ int handle_uretprobe_byname3(struct pt_regs *ctx) > return 0; > } > > +SEC("uprobe") > +int BPF_UPROBE(handle_uprobe_byversionedname_a, const char *a, char *b) > +{ > + uprobe_byversionedname_a_res = 13; > + return 0; > +} > + > +SEC("uprobe") > +int BPF_UPROBE(handle_uprobe_byversionedname_b, const char *a, char *b) > +{ > + uprobe_byversionedname_b_res = 14; > + return 0; > +} > > char _license[] SEC("license") = "GPL"; > -- > 2.34.1 >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 49cd304ae3bc..ef5e11ce6241 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10620,31 +10620,94 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe, } /* Return next ELF section of sh_type after scn, or first of that type if scn is NULL. */ -static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn) +static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn, size_t *idx) { while ((scn = elf_nextscn(elf, scn)) != NULL) { GElf_Shdr sh; if (!gelf_getshdr(scn, &sh)) continue; - if (sh.sh_type == sh_type) + if (sh.sh_type == sh_type) { + if (idx) + *idx = sh.sh_link; return scn; + } + } + return NULL; +} + +static Elf_Data *elf_find_data_by_type(Elf *elf, int sh_type, size_t *idx) +{ + Elf_Scn *scn = elf_find_next_scn_by_type(elf, sh_type, NULL, idx); + + if (scn) + return elf_getdata(scn, NULL); + + return NULL; +} + +static Elf64_Verdef *elf_verdef_by_offset(Elf_Data *data, size_t offset) +{ + if (offset + sizeof(Elf64_Verdef) > data->d_size) + return NULL; + + return (Elf64_Verdef *)((char *) data->d_buf + offset); +} + +static Elf64_Versym *elf_versym_by_idx(Elf_Data *data, size_t idx) +{ + if (idx >= data->d_size / sizeof(Elf64_Versym)) + return NULL; + + return (Elf64_Versym *)(data->d_buf + idx * sizeof(Elf64_Versym)); +} + +static Elf64_Verdaux *elf_verdaux_by_offset(Elf_Data *data, size_t offset) +{ + if (offset + sizeof(Elf64_Verdaux) > data->d_size) + return NULL; + + return (Elf64_Verdaux *)((char *) data->d_buf + offset); +} + +#define ELF_VERSYM_HIDDEN 0x8000 +#define ELF_VERSYM_IDX_MASK 0x7fff + +static Elf64_Verdaux *elf_get_verdaux_by_versym(Elf_Data *verdef_data, Elf64_Versym *versym) +{ + size_t offset = 0; + + while (offset + sizeof(Elf64_Verdef) <= verdef_data->d_size) { + Elf64_Verdef *verdef = elf_verdef_by_offset(verdef_data, offset); + + if (!verdef) + break; + + if (verdef->vd_ndx == (*versym & ELF_VERSYM_IDX_MASK)) + return elf_verdaux_by_offset(verdef_data, offset + verdef->vd_aux); + + if (verdef->vd_next == 0) + break; + + offset += verdef->vd_next; } return NULL; } /* Find offset of function name in the provided ELF object. "binary_path" is * the path to the ELF binary represented by "elf", and only used for error - * reporting matters. "name" matches symbol name or name@@LIB for library - * functions. + * reporting matters. "name" matches symbol name, name@LIB or name@@LIB for + * library functions. */ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name) { int i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB }; bool is_shared_lib, is_name_qualified; long ret = -ENOENT; - size_t name_len; GElf_Ehdr ehdr; + Elf_Data *versym_data = NULL; + Elf_Data *verdef_data = NULL; + size_t verdef_stridx = 0; if (!gelf_getehdr(elf, &ehdr)) { pr_warn("elf: failed to get ehdr from %s: %s\n", binary_path, elf_errmsg(-1)); @@ -10654,9 +10717,12 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char * /* for shared lib case, we do not need to calculate relative offset */ is_shared_lib = ehdr.e_type == ET_DYN; - name_len = strlen(name); - /* Does name specify "@@LIB"? */ - is_name_qualified = strstr(name, "@@") != NULL; + /* Does name specify "@@LIB" or "@LIB"? */ + is_name_qualified = strstr(name, "@") != NULL; + + /* Extract version definition and version symbol table */ + versym_data = elf_find_data_by_type(elf, SHT_GNU_versym, NULL); + verdef_data = elf_find_data_by_type(elf, SHT_GNU_verdef, &verdef_stridx); /* Search SHT_DYNSYM, SHT_SYMTAB for symbol. This search order is used because if * a binary is stripped, it may only have SHT_DYNSYM, and a fully-statically @@ -10671,10 +10737,10 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char * const char *sname; GElf_Shdr sh; - scn = elf_find_next_scn_by_type(elf, sh_types[i], NULL); + scn = elf_find_next_scn_by_type(elf, sh_types[i], NULL, NULL); if (!scn) { pr_debug("elf: failed to find symbol table ELF sections in '%s'\n", - binary_path); + binary_path); continue; } if (!gelf_getshdr(scn, &sh)) @@ -10705,16 +10771,60 @@ static long elf_find_func_offset(Elf *elf, const char *binary_path, const char * if (!sname) continue; - curr_bind = GELF_ST_BIND(sym.st_info); + if (is_name_qualified) { + Elf64_Versym *versym; + Elf64_Verdaux *verdaux; + int res; + char full_name[256]; - /* User can specify func, func@@LIB or func@@LIB_VERSION. */ - if (strncmp(sname, name, name_len) != 0) - continue; - /* ...but we don't want a search for "foo" to match 'foo2" also, so any - * additional characters in sname should be of the form "@@LIB". - */ - if (!is_name_qualified && sname[name_len] != '\0' && sname[name_len] != '@') - continue; + /* check that name at least starts with sname before building + * the full name + */ + if (strncmp(name, sname, strlen(sname)) != 0) + continue; + + if (!versym_data || !verdef_data) { + pr_warn("elf: failed to find version definition or version symbol table in '%s'\n", + binary_path); + break; + } + + versym = elf_versym_by_idx(versym_data, idx); + if (!versym) { + pr_warn("elf: failed to lookup versym for '%s' in '%s'\n", + sname, binary_path); + continue; + } + + verdaux = elf_get_verdaux_by_versym(verdef_data, versym); + if (!verdaux) { + pr_warn("elf: failed to lookup verdaux for '%s' in '%s'\n", + sname, binary_path); + continue; + } + + res = snprintf(full_name, sizeof(full_name), + (*versym & ELF_VERSYM_HIDDEN) ? "%s@%s" : + "%s@@%s", + sname, + elf_strptr(elf, verdef_stridx, + verdaux->vda_name)); + + if (res < 0 || res >= sizeof(full_name)) { + pr_warn("elf: failed to build full name for '%s' in '%s'\n", + sname, binary_path); + continue; + } + + if (strcmp(full_name, name) != 0) + continue; + } else { + /* If name is not qualified, we want to match the symbol name */ + if (strcmp(sname, name) != 0) + continue; + } + + curr_bind = GELF_ST_BIND(sym.st_info); if (ret >= 0) { /* handle multiple matches */ diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c index 7175af39134f..c3f33f7e9d12 100644 --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c @@ -192,6 +192,41 @@ static void test_uprobe_lib(struct test_attach_probe *skel) ASSERT_EQ(skel->bss->uretprobe_byname2_res, 8, "check_uretprobe_byname2_res"); } +static void test_uprobe_lib_with_versions(struct test_attach_probe *skel) +{ + DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts); + char absolute_path[256]; + + /* test attach with a versioned name. + * realpath has two implementations in libc, only the default version will be used. + */ + uprobe_opts.func_name = "realpath@@GLIBC_2.3"; + uprobe_opts.retprobe = false; + skel->links.handle_uprobe_byversionedname_a = + bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byversionedname_a, + 0 /* this pid */, + "libc.so.6", + 0, &uprobe_opts); + if (!ASSERT_OK_PTR(skel->links.handle_uprobe_byversionedname_a, "attach_handle_uprobe_byversionedname_a")) + return; + + uprobe_opts.func_name = "realpath@GLIBC_2.2.5"; + uprobe_opts.retprobe = false; + skel->links.handle_uprobe_byversionedname_b = + bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byversionedname_b, + 0 /* this pid */, + "libc.so.6", + 0, &uprobe_opts); + if (!ASSERT_OK_PTR(skel->links.handle_uprobe_byversionedname_b, "attach_handle_uprobe_byversionedname_b")) + return; + + /* trigger & validate probes */ + realpath("/", absolute_path); + + ASSERT_EQ(skel->bss->uprobe_byversionedname_a_res, 13, "check_uprobe_byversionedname_a_res"); + ASSERT_NEQ(skel->bss->uprobe_byversionedname_b_res, 14, "check_uprobe_byversionedname_b_res"); +} + static void test_uprobe_ref_ctr(struct test_attach_probe *skel) { DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts); @@ -316,6 +351,8 @@ void test_attach_probe(void) test_kprobe_sleepable(); if (test__start_subtest("uprobe-lib")) test_uprobe_lib(skel); + if (test__start_subtest("uprobe-lib-with-versions")) + test_uprobe_lib_with_versions(skel); if (test__start_subtest("uprobe-sleepable")) test_uprobe_sleepable(skel); if (test__start_subtest("uprobe-ref_ctr")) diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c index 68466a6ad18c..079b58901ff8 100644 --- a/tools/testing/selftests/bpf/progs/test_attach_probe.c +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c @@ -17,6 +17,8 @@ int uprobe_byname3_sleepable_res = 0; int uprobe_byname3_res = 0; int uretprobe_byname3_sleepable_res = 0; int uretprobe_byname3_res = 0; +int uprobe_byversionedname_a_res = 0; +int uprobe_byversionedname_b_res = 0; void *user_ptr = 0; SEC("ksyscall/nanosleep") @@ -121,5 +123,18 @@ int handle_uretprobe_byname3(struct pt_regs *ctx) return 0; } +SEC("uprobe") +int BPF_UPROBE(handle_uprobe_byversionedname_a, const char *a, char *b) +{ + uprobe_byversionedname_a_res = 13; + return 0; +} + +SEC("uprobe") +int BPF_UPROBE(handle_uprobe_byversionedname_b, const char *a, char *b) +{ + uprobe_byversionedname_b_res = 14; + return 0; +} char _license[] SEC("license") = "GPL";
This change fixes the handling of versions in elf_find_func_offset. In the previous implementation, we incorrectly assumed that the version information would be present in the string found in the string table. We now look up the correct version string in the version symbol table before constructing the full name and then comparing. This patch adds support for both name@version and name@@version to match output of the various elf parsers. Signed-off-by: Espen Grindhaug <espen.grindhaug@gmail.com> --- Changes since v1: - Added test --- tools/lib/bpf/libbpf.c | 148 +++++++++++++++--- .../selftests/bpf/prog_tests/attach_probe.c | 37 +++++ .../selftests/bpf/progs/test_attach_probe.c | 15 ++ 3 files changed, 181 insertions(+), 19 deletions(-)