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

kpatch-build: strengthen conditions for changed sections #1303

Merged
merged 1 commit into from Sep 23, 2022

Conversation

anatasluo
Copy link
Contributor

@anatasluo anatasluo commented Sep 23, 2022

Based on this kpatch-build: rela section could disappear after patched.

I found another problem, considering the following example in X86.

original code:

__noreturn noinline int kpatch_func(void)
{
	while(1) { };
}
EXPORT_SYMBOL(kpatch_func);

patched code:

__noreturn notrace noinline int kpatch_func(void)
{
	asm(".byte 0xe8, 0x00, 0x00, 0x00, 0x00");
	while(1) { };
}
EXPORT_SYMBOL(kpatch_func);

These two functions are different, but the output of Kpatch is
ERROR: no functional changes found.

The problem is explained in the log:


If two sections want to be the same, they need to satisfy
two conditions:

  1. the result of memcpy is zero, which means they have the same content.

  2. they have the same relocation entries.

In one specific situation, two sections have the same content.
But one section has relocation entries while the other one has
no relocation entries. For example, in X86, consider the
following code:

original code

__noreturn noinline int kpatch_func(void)
{
	while(1) {};
}

patched code

__noreturn notrace noinline int kpatch_func(void)
{
	asm(".byte 0xe8, 0x00, 0x00, 0x00, 0x00");
	while(1){};
}

Since the original code has a fentry call, these two functions have
the same compile result. But obviously, they are different functions.
Currently, Kpatch would not find their differences since the patched
code has no relocation entries.

For the situation that one section has relocation entries while the
other one doesn't have, it should be set to be changed directly.


I am not sure if it is useful for Kpatch, so I open another PR.

@joe-lawrence
Copy link
Contributor

Hi @anatasluo : curious cases! Have you seen these occur in a real-life patch? FWIW, I don't think we've ever toggled trace/notrace in a patch, so maybe these are just a proof of concept.

So the summary is:

Small nit in the commit msg for this one: s/result of memcpy is zero/result of memcmp is zero/

@anatasluo
Copy link
Contributor Author

@joe-lawrence You are right, it is rare to make a patch like that in the kernel. Actually, we are trying to implement a livepatch mechanism in userspace. And we borrow many ideas from Kpatch. In userspace, there is no trace or return thunk mechanism. After theoretical analysis, we found these corner cases. And we use these cases to test our program so that any modification for the program works as expected. I hope these cases will not bother the community.

@anatasluo
Copy link
Contributor Author

@joe-lawrence About this case kpatch-build: strengthen conditions for changed sections, my beginning idea is: relocation entry will fill the content with zeros temporarily. For code like:

mov $0, $rax

It could be two situations:

  1. "$0" is the data "0".
  2. "$0" is filled by the relocation entry.

These two situations are different, but they have the same memory content.
So, to make the comparison more accurate, we need to ensure they both have the same relocation entry.

@jpoimboe
Copy link
Member

Looks good to me, can you rebase on master?

@joe-lawrence
Copy link
Contributor

Actually, we are trying to implement a livepatch mechanism in userspace. And we borrow many ideas from Kpatch. In userspace, there is no trace or return thunk mechanism. After theoretical analysis, we found these corner cases. And we use these cases to test our program so that any modification for the program works as expected. I hope these cases will not bother the community.

Sounds like a cool project. Thanks for upstreaming issues you encounter, reports and patches are welcome.

@anatasluo
Copy link
Contributor Author

@jpoimboe already rebase on master

If two sections want to be the same, they need to satisfy
two conditions:

1) the result of memcmp is zero, which means they
have the same content.

2) they have the same relocation entries.

In one specific situation, two sections have the same content.
But one section has relocation entries while the other one has
no relocation entries. For example, in X86, consider the
following code:

original code
```
__noreturn noinline int kpatch_func(void)
{
	while(1) {};
}
```

patched code
```
__noreturn notrace noinline int kpatch_func(void)
{
	asm(".byte 0xe8, 0x00, 0x00, 0x00, 0x00");
	while(1){};
}
```

Since the original code has a fentry call, these two functions have
the same compile result. But obviously, they are different functions.
Currently, kpatch would not find their differences since the patched
code has no relocation entries.

For the situation that one section has relocation entries while the
other one doesn't have, it should be set to be changed directly.

Cooperated-by: Zongwu Li <lizongwu@huawei.com>
Signed-off-by: Longjun Luo <luolongjuna@gmail.com>
@joe-lawrence joe-lawrence merged commit 7ec1ed6 into dynup:master Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants