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

Changes to build the kmod with 5.1 kernels [SMAGENT-1643] #1413

Merged
merged 3 commits into from May 23, 2019

Conversation

Projects
None yet
6 participants
@nathan-b
Copy link
Contributor

commented May 21, 2019

  • The syscall_get_arguments function changed its parameters.
  • The mmap symbols changed header locations

nathan-b added some commits May 21, 2019

Changes to build the kmod with 5.1 kernels
* The syscall_get_arguments function changed its parameters.
* The mmap symbols changed header locations
@krishnan-ramkumar
Copy link
Contributor

left a comment

Really loved the Macro idea in ppm_fillers.c . Was it not possible to replicate that universally in our code base ? Would have resulted in a single source of fix/change.

Also, the header fix needs guards too, I think.

Show resolved Hide resolved driver/ppm_flag_helpers.h
@krishnan-ramkumar

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Also, I recommend building this change on a 4.x.x kernel too. Just to see it works; if you haven't tried already. 👍

@anoop-sysd anoop-sysd changed the title Changes to build the kmod with 5.1 kernels Changes to build the kmod with 5.1 kernels [SMAGENT-1643] May 22, 2019

@nathan-b

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Hi @krishnan-ramkumar, thanks for the review. I didn't want to do the macro trick on a larger scale because I felt it was cleaner to fix each call site individually on the files that had just a handfull of calls. Obviously going forward code will use the new function signature. Hand-written code crafted for the specific call site will always be faster and more efficient than the generic code I wrote in the macro. Maybe a micro-optimization, but that's my justification anyway ;)

unsigned long __user *scargs;
int socketcall_id;

syscall_get_arguments(current, regs, 0, 2, args);
#if (LINUX_VERSION_CODE < KERNEL_VERSION(5, 1, 0))

This comment has been minimized.

Copy link
@adalton

adalton May 22, 2019

Contributor

Just a thought, but maybe it'd be nicer to do:

#if (LINUX_VERSION_CODE < KERNEL_VERSION(5, 1, 0))
#define SYSCALL_GET_ARGUMENTS(current, regs, args) syscall_get_arguments((current), (regs), 0, 6, (args))
#else 
#define SYSCALL_GET_ARGUMENTS(current, regs, args) syscall_get_arguments((current), (regs), (args))
#endif 

And then, for each call to the API, just make it:

SYSCALL_GET_ARGUMENTS(current, regs, args);

That way you avoid duplicating the logic at all the call sites.

@gnosek

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

TBH I'd rather see our API changed in the other direction, i.e. get all the arguments once per function in an array and then refer to that array without further function calls.

I'm not sure if it will always be safe to get more args than the syscall actually uses (if there's an architecture where args get passed in memory, this would be risky), so we could end up with something like

static inline void ppm_syscall_get_arguments(struct task_struct *task, struct pt_regs *regs, unsigned long *args, int num_args)
{
#if (LINUX_VERSION_CODE < KERNEL_VERSION(5, 1, 0))
    syscall_get_arguments(task, regs, args); // ignore num_args, we know 6 is safe
#else 
    syscall_get_arguments(task, regs, 0, num_args, args);
#endif 
}

If we know always reading all 6 args is safe, then just:

static inline void ppm_syscall_get_arguments(struct task_struct *task, struct pt_regs *regs, unsigned long *args)
{
#if (LINUX_VERSION_CODE < KERNEL_VERSION(5, 1, 0))
    syscall_get_arguments(task, regs, args);
#else 
    syscall_get_arguments(task, regs, 0, 6, args);
#endif 
}
@abucodonosor

This comment has been minimized.

Copy link

commented May 22, 2019

syscall_get_arguments_deprecated() sounds a bit confusing to me.

syscall_get_arguments_combined() maybe ?

@nathan-b

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@gnosek The commit which changed the function signature reads as follows:

At Linux Plumbers, Andy Lutomirski approached me and pointed out that the function call syscall_get_arguments() implemented in x86 was horribly written and not optimized for the standard case of passing in 0 and 6 for the starting index and the number of system calls to get. When looking at all the users of this function, I discovered that all instances pass in only 0 and 6 for these arguments. Instead of having this function handle different cases that are never used, simply rewrite it to return the first 6 arguments of a system call.

In other words, the way we're using this API is non-standard anyway, and the usual case (from other consumers) is requesting all args.

@abucodonosor: I named it deprecated because I don't want anyone else to use it. Ideally we eventually change all these call sites to use the new function signature and then get rid of the deprecated wrapper.

For @adalton and @gnosek: I'm not convinced that we need to make a formal wrapper for this function. From reading the commit message, I don't think this will be a function signature that's likely to change again in the near future. If you see a benefit in providing a wrapper, I'm willing to consider it. But I feel like my solution is a reasonable way forward.

@gnosek

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Ideally we eventually change all these call sites

Supporting kernels < 5.1.0 won't go away for a long time

a function signature that's likely to change again in the near future

It has changed (hence this issue) so we have to do something and I feel that copy-pasting the same #if all over the place is not the right thing.

If you see a benefit in providing a wrapper, I'm willing to consider it.

Avoiding the same #if over and over is pretty nice.

BTW, e.g. kernel/seccomp.c is unconditionally getting all 6 args so all the ABIs must be sane enough.

@nathan-b

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@gnosek I will make the change, but under protest ;)

@gnosek

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Protest noted and duly ignored ;)

The ideal I'd hope to achieve at some point would be the code using only the latest released APIs with some compat.h encapsulating support for all past versions. I think xtables-addons does it this way and it looks pretty clean.

@nathan-b nathan-b requested a review from krishnan-ramkumar May 22, 2019

if (!args->is_socketcall) {
ppm_syscall_get_arguments(current, args->regs, syscall_args);
val = syscall_args[4];
} else

This comment has been minimized.

Copy link
@gnosek

gnosek May 22, 2019

Contributor

This pattern repeats several times, can we do something like this somewhere up? hopefully even outside the args->event_type checks

unsigned long *actual_args = &syscall_args;

if (!args->is_socketcall) {
    ppm_syscall_get_arguments(current, args->regs, syscall_args);
} else {
    actual_args = &args->socketcall_args;
}

and unconditionally refer to actual_args (hopefully with a better name) everywhere?

Yeah, I know, it's been there already but since we're here... ;)

This comment has been minimized.

Copy link
@nathan-b

nathan-b May 22, 2019

Author Contributor

It's unfortunately not as clean as your sample code makes it out to be. In the call site you refer to (inside compute_snaplen), val refers to different slots in syscall_args depending on some flags. So the code would actually look like:

unsigned long *actual_args = &syscall_args[4];

if (!args->is_socketcall) {
    ppm_syscall_get_arguments(current, args->regs, syscall_args);
} else {
    actual_args = &args->socketcall_args[4];
}

if (usrsockaddr == NULL) {
   actual_args = &syscall_args[5];
   if (!args->is_socketcall) {
       ppm_syscall_get_arguments(current, args->regs, syscall_args);
   } else {
       actual_args = &args->socketcall_args[5];
   }
}

Since we have to shuffle the index around, I don't feel as though we get as much benefit from a change like that. I don't feel that it outweighs the awkwardness of having actual_args being a pointer to a long. I also feel like that version is a little less readable because actual_args gets set to the correct index much earlier in the function instead of right at the relevant call site.

Perhaps I'm misunderstanding your comment though?

This comment has been minimized.

Copy link
@gnosek

gnosek May 22, 2019

Contributor

@nathan-b I had something like this in mind: bed44dd

(though it's obviously unfinished and there are args->is_socketcall checks in other files as well, but that's the general idea)

This comment has been minimized.

Copy link
@nathan-b

nathan-b May 22, 2019

Author Contributor

Gotcha. I'm on the fence about whether this is future work or something to be resolved while I'm already touching this code, but since I'm not allowed to check in right now anyway then maybe tomorrow I'll look at doing this.

This comment has been minimized.

Copy link
@gnosek

gnosek May 22, 2019

Contributor

yeah, no pressure, we can revisit this later and just do the minimal fix now

@krishnan-ramkumar
Copy link
Contributor

left a comment

LGTM

@nathan-b nathan-b merged commit a6ab1e6 into dev May 23, 2019

3 checks passed

Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
sign-off-checker The commit doesn't require sysdig sign-off CLA because it belongs to nathan-b part of draios/sysdig collaborators
Details

@nathan-b nathan-b deleted the linux_5_1_x branch May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.