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

create-diff-object static local variable correlation and inlining #1351

Closed
joe-lawrence opened this issue Jul 24, 2023 · 5 comments
Closed
Labels

Comments

@joe-lawrence
Copy link
Contributor

@ryanbsull noticed this when building a CVE-2023-35788 kpatch for some releases of RHEL on ppc64le:

ERROR: cls_flower.o: reference to static local variable __msg.59123 in fl_set_geneve_opt was removed

Which is interesting given the patch only adds a small conditional to fl_set_geneve_opt().

When comparing the original cls_flower.o to the patched cls_flower.o, it seems that the compiler has converted fl_set_geneve_opt() from a function to an inlined one (thanks -O3 optimization). In order to debug this, we looked at the symbol tables for each object file and compared them -- an equal number of __msg. variables, but no fl_set_geneve_opt symbol in the patched object.

Modifying the kpatch to mark fl_set_geneve_opt as noinline avoided the build error, but perhaps create-diff-object could save the user the investigation? Would something like the following (a rough draft) be worth adding to the static local correlation logic:

@@ -1408,10 +1408,14 @@ static void kpatch_correlate_static_local_variables(struct kpatch_elf *orig,
                        }
 
                        patched_sym = kpatch_find_static_twin(relasec, sym);
-                       if (!patched_sym)
+                       if (!patched_sym) {
+                               if (!find_section_by_name(&patched->sections, relasec->name))
+                                       log_normal("section %s was removed, %s may have been inlined?\n",
+                                                  relasec->name, kpatch_section_function_name(relasec));
                                DIFF_FATAL("reference to static local variable %s in %s was removed",
                                           sym->name,
                                           kpatch_section_function_name(relasec));
+                       }
 
                        patched_bundled = patched_sym == patched_sym->sec->sym;
                        if (bundled != patched_bundled)

which would emit additional guidance:

cls_flower.ORIG.o: section .rela.text.fl_set_geneve_opt was removed, fl_set_geneve_opt may have been inlined?
ERROR: cls_flower.ORIG.o: reference to static local variable __msg.59123 in fl_set_geneve_opt was removed
create-diff-object: unreconcilable difference

Alternatively we could document what to do in the patch author guide in case of this error message. It may be easy to work around an inline case like this one, but others may require a real workaround in the code.

repro-inlined-static-local.tar.gz

@jpoimboe
Copy link
Member

Seems reasonable to me.

@euspectre
Copy link
Contributor

Yes, printing the name of the possibly inlined function in such cases does help a lot, saves a significant amount of time when investigating build failures.

Thanks!

@joe-lawrence
Copy link
Contributor Author

Would a similar correlation problem would exist in the opposite case: function A was inlined with a static local that belonged to parent callers B and C, but then the patched version promotes A to a real function and now there is a new static local for A and the ones formerly associated with B and C are removed.

@github-actions
Copy link

This issue has been open for 30 days with no activity and no assignee. It will be closed in 7 days unless a comment is added.

@github-actions github-actions bot added the stale label Aug 25, 2023
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

This issue was closed because it was inactive for 7 days after being marked stale.

@github-actions github-actions bot closed this as completed Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants