-
Notifications
You must be signed in to change notification settings - Fork 697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix address calculation on direct address when using uprobe #1495
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: umutbasal <[email protected]>
// address calculates the address of a symbol in the executable. | ||
// | ||
// opts must not be nil. | ||
func (ex *Executable) address(symbol string, address, offset uint64) (uint64, error) { | ||
if address > 0 { | ||
return address + offset, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you share an example for which the current code doesn't work? If address
is not 0 here, it comes from UprobeOptions
, which is meant to override the address found in the elf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to get the address of a symbol from the disassembler and enter it manually, but it didn't work. After debugging I found that this offset calculation was done only after retrieving the address from the symbol table. The code that works using the same address in the BCC library did not work here. It seems like bcc make this calculation in both methods(name/addr). https://github.com/iovisor/bcc/blob/master/libbpf-tools/uprobe_helpers.c#L284. I'm not sure if this should be included in this package or the dependent projects, but since it works this way in bcc, I wanted to share it.
an example symbol from go binary
symbol addr: 0x1974f0
file offset: 0x1874f0
# objdump -D -F -C gobinary
00000000001974f0 <crypto/tls.(*Conn).writeRecordLocked> (File Offset: 0x1874f0):
1974f0: f9400b90 ldr x16, [x28, #16]
1974f4: d10283f1 sub x17, sp, #0xa0
1974f8: eb10023f cmp x17, x16
for the current version, we need to pass the file offset to opts.Address, because it does not calculate itself, but calculates the offset when giving the symbol name.
type UprobeOptions struct {
// Symbol address. Must be provided in case of external symbols (shared libs).
// If set, overrides the address eventually parsed from the executable.
Address uint64
Maybe an explanation in the comments will be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with BCC, but I don't think I understand why the address should be calculated if it's provided externally.
There have been some confusion in the past about the naming, maybe what's called address/offset here is not the same in BCC? Do you have some snippets from your tests both from BCC and go-ebpf?
I'm interested in what was passed to exec.Uprobe()
and what was provided in UprobeOptions
(with the BCC counterpart)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
difference between two lib, objdump is same as above.
bcc
b.attach_uprobe(name="gobinary", addr=1668336, fn_name="traceWRL") # works
cilium/ebpf
uprobe, err := ex.Uprobe(fn, objs.UprobeGoTLSWrite, &link.UprobeOptions{
Address: 1668336, // 0x1974f0 do not work
//Address: 1602800, // 0x1874f0 address after calculation that works
})
Symbol addres I expected to work is 1668336 from symbol table.
So it needs clarification for address, should we need to pass symtab.Sym.Value or file offset calc taken from stackoverflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, seems like there's a difference between ebpf-go and BCC/bpftrace. Libbpf seems to be doing like ebpf-go, passing the value directly to perf_event_open: https://github.com/libbpf/libbpf/blob/42065ea6627ff6e1ab4c65e51042a70fbf30ff7c/src/libbpf.c#L12111 and https://github.com/libbpf/libbpf/blob/42065ea6627ff6e1ab4c65e51042a70fbf30ff7c/src/libbpf.c#L10906; I'm not sure we should change the behavior as it would also be a breaking change.
I'll do some other tests tomorrow to make sure libbpf behaves the same and then we can eventually improve the documentation of UprobeOptions.Address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmat11 Reminder to figure this out when you're back.
@umutbasal Alternatively, do you have any suggestions on how to document this better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, lost track of this, will check out when I have some time
Address calculation of the symbols occurs only with Executable.load(). while the symbol name is provided.
This PR fixes by ensuring that the symbol address calculation occurs correctly even when the symbol name didn't provided.