Skip to content
New issue

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

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(bpf): Match pname as long as 16 bytes #769

Merged
merged 4 commits into from
Mar 3, 2025

Conversation

jschwinger233
Copy link
Member

Background

之前为了修复 #736, #738 相当于把 dae pname 缩减到 15 字节了,但稍作修改也可让 dae 正确匹配 16 字节 pname。

核心 bug 出在 bpf_core_read_str (bpf_probe_read_user_str),内核的实现是强行在最后一个字节设为 \0:

long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
			      long count)
{
	long ret;

	if (unlikely(count <= 0))
		return 0;

	pagefault_disable();
	ret = strncpy_from_user(dst, unsafe_addr, count);
	pagefault_enable();

	if (ret >= count) {
		ret = count;
		dst[ret - 1] = '\0';
	} else if (ret > 0) {
		ret++;
	}

	return ret;
}

https://elixir.bootlin.com/linux/v6.6/source/mm/maccess.c#L177

解决办法就是不用 bpf_probe_read_user_str,两处内存都在 bpf 内核态栈里,手动拷贝一下就行了。不能直接用 __builtin_memcpy 是因为 verifier 要安全检查,手动 for 循环里加上 if (ctx.l + i < MAX_ARG_LEN) 糊弄一下 即可。

(性能也应该更快一点点点)

Checklist

Full Changelogs

  • [Implement ...]

Issue Reference

Closes #[issue number]

Test Result

@jschwinger233
Copy link
Member Author

jschwinger233 commented Mar 2, 2025

~~ 不好,单测挂了,我先 convert to draft 明天再说- - ~~

clang bug,指定 clang15 就好了...

@jschwinger233 jschwinger233 marked this pull request as draft March 2, 2025 19:01
@jschwinger233 jschwinger233 marked this pull request as ready for review March 3, 2025 12:40
@jschwinger233 jschwinger233 requested a review from a team as a code owner March 3, 2025 12:40
@mzz2017
Copy link
Contributor

mzz2017 commented Mar 3, 2025

image

Copy link
Contributor

@dae-prow dae-prow bot left a comment

Choose a reason for hiding this comment

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

🧪 Since the PR has been fully tested, please consider merging it.

@mzz2017 mzz2017 merged commit 43ec540 into daeuniverse:main Mar 3, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants