draios / sysdig

Linux system exploration and troubleshooting tool with first class support for containers
http://www.sysdig.com/
Other
7.73k stars 726 forks source link

implement signal capture #282

Closed ldegio closed 9 years ago

ldegio commented 9 years ago

intercepting the trace_signal_deliver tracepoint (https://www.kernel.org/doc/htmldocs/tracepoint/API-trace-signal-deliver.html) should make it possible to expose signal reception in the sysdig event stream.

jarun commented 9 years ago

I am very much interested in implementing this enhancement. I need some guidance. I went through the following docs to understand the concept of Linux tracepoints: https://www.kernel.org/doc/Documentation/trace/tracepoints.txt https://www.kernel.org/doc/Documentation/trace/events.txt

As per my understanding the task is to enable the signal_deliver tracepoint and then log the details of the generated (or rather delivered?) signal.

Can someone please redirect me to an already implemented trace handling code in sysdig which I can refer to for further understanding?

ldegio commented 9 years ago

Please take a look at this page, which should be a good starting reference https://github.com/draios/sysdig/wiki/Adding-an-event-to-sysdig

jarun commented 9 years ago

Thank you so much!

jarun commented 9 years ago

In progress. Currently trying to understand how to populate the ppm_event_info struct.

jarun commented 9 years ago

Two questions:

  1. In Step 3 in the guide (as well as in the code), TRACEPOINT_PROBE is added for sched_switch_probe(). However, the closest function in the kernel is probe_sched_switch():
    http://lxr.free-electrons.com/source/kernel/trace/trace_sched_switch.c#L53
    Does the code need to change?
  2. As explained in Step 3, is it necessary to add TRACEPOINT_PROBE for each event? I don't see it for many PPMs. In which case to add it and in which case not to add it?
ldegio commented 9 years ago

On 12/23/2014 1:41 PM, Arun Prakash Jana wrote:

Two questions:

1.

In Step 3 in the guide (as well as in the code), TRACEPOINT_PROBE
is added for sched_switch_probe(). However, the closest function
in the kernel is probe_sched_switch():

http://lxr.free-electrons.com/source/kernel/trace/trace_sched_switch.c#L53

Does the code need to change?

TRACEPOINT_PROBE(sched_switch_probe... is used to declare a handler function for the sched_switch_probe tracepoint. This handler function is passed to ret = compat_register_trace(sched_switch_probe, "sched_switch", tp_sched_switch);

After that it's going to be called from here http://lxr.free-electrons.com/source/kernel/sched/core.c#L2213 every time there's a context switch.

1.

As explained in Step 3, is it necessary to add TRACEPOINT_PROBE
for each event? I don't see it for many PPMs. In which case to add
it and in which case not to add it?

You don't see many because most of the events we support are system calls, which are intercepted using just a couple of trace points: sys_enter and sys_exit. The tracepoint you want to use for signals, however, is the one I mention in the first message of this issue: trace_signal_deliver.

Loris

— Reply to this email directly or view it on GitHub https://github.com/draios/sysdig/issues/282#issuecomment-68001656.

jarun commented 9 years ago

Thank you for clarifying :).

jarun commented 9 years ago

In progress. Was spending time with my 11-month old during the vacations. My primary goal is to print some debug info when a signal is delivered to a process. Once I am there, my understanding is that I'll have to write something similar to the record_event function for syscalls.

jarun commented 9 years ago

The primary changes to print debug info when a signal is delivered is now in my local development branch: https://github.com/jarun/sysdig/commit/00d678135ffd09a67fb2ee99d94f5d7dcbfaf22d Note: There is 1 macro related and 2 space related issues which I will modify in next commit.

I have tested it as below from build directory: $ cmake -DCMAKE_BUILD_TYPE=Debug .. $ sudo insmod driver/sysdig-probe.ko verbose=1 $ sudo userspace/sysdig/sysdig evt.type=switch $ dmesg

I can see "sysdig_probe: signal_deliver_probe called" in the output.

For implementing the function signal_deliver_probe(), can you please let me know which information you would like to capture in the trace?

ldegio commented 9 years ago

I would say, if available, it would be nice to see

Anything other than this that I'm missing?

jarun commented 9 years ago

Thanks! I'll start with these and discuss if I find anything that will add value to the info.

jarun commented 9 years ago

In progress.

jarun commented 9 years ago

Pushed the primary modification for the driver logic just now: https://github.com/jarun/sysdig/compare/mods

Few questions:

  1. I have extended the event_filler_arguments structure in driver/ppm_events.h to include the signal number, source and dest PIDs. Is that permitted?
  2. In driver/main.c, record_signal(), I am setting the source PID as 0 if the signal is not any of sig_kill, sig_rt or sig_child. My reference is http://lxr.free-electrons.com/source/include/uapi/asm-generic/siginfo.h#L48 for the siginfo structure. Instead of 0, what should be the right way to deal with this?
ldegio commented 9 years ago

I have a question: why having the record_signal function, which largely duplicates the record_event functionality (and complexity) instead of just extending record_event?

Note that record_event is currently designed to be the only entry point to event capture, and currently handles both system call and context switch events.

jarun commented 9 years ago

While I considered it, I kept it separate due to the following reasons:

  1. trace_signal_deliver has a complementary tracepoint: trace_signal_generate() which we may want to cater to later.
  2. The arguments to record_signal() and record_event() are different. So either we have to add additional arguments to record_event() [which also means changing the call to record_event() in other places] or do some casting to pass the pointers of different types. I am not sure if these are good ideas and what should be the right course. In addition, I had no idea that record_event() is designed to be the unique entrypoint.
  3. These are very early stage commits and can be changed any time now that I have the basic understanding of how things work in this area. I didn't want to change the existing code too much without being sure that I understand what's going on. So nothing's final and I would be glad to receive your feedbacks.

Please let me know how you want to proceed with using record_event(). As I mentioned above, there are 2 ways.

Also, please provide your feedback on the other two questions I posted in my earlier comment.

jarun commented 9 years ago

Hi, waiting for your response to my last comment...

ldegio commented 9 years ago

sorry for the delay.

Regarding #1, I think this actually makes a point toward having a single entry point for event interception. Otherwise, trace_signal_generate() might introduce yet another entry point.

2 This is a valid concern. If you note, record_ev currently ha both a set of arguments for system calls (regs and id) and a set of argments for context switches (sched_prev and sched_next). Obviously, as we add support for more tracepoints, this is not going to scale. I have a couple of proposals:

I prefer the first solution, but I'm open to listening to opinions.

jarun commented 9 years ago

Thanks for the pointers! I also think your first proposal "converting the record_event arguments into a struct that can be extended more easily, and only requires some of the fields (depending on the event category) to be present" is the best. I will introduce a new union to begin with (with 3 structs for system calls, context switches and signal delivery). Hopefully this will be a modular and easy to extend + handle approach.

jarun commented 9 years ago

Hi,

I have made the changes to the driver as we discussed. Can you please review the changes and let me know if you'ld like to change anything? https://github.com/jarun/sysdig/compare/mods