This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: Tapset for Signals....


On Wednesday, August 16, 2006 1:36 AM, Li Guanglei wrote:
> Below are my questions while I am trying to add signal trace hooks
> into LKET:

Thanks for reviewing.

> 1. group_send_sig_info() will check the permissions for sending the
> signal before calling __group_send_sig_info(). So probing
> __group_send_sig_info() will omit the situation that a signal has been
> actually sent out but was rejected due to wrong permission.
> 
> But __group_send_sig_info() will be also called by the following
> functions besides group_send_sig_info(),:
>    1> do_notify_parent() when a process exits
>    2> check_process_timers() for POSIX timers
>    3> do_notify_parent_cldstop()
>    4> kill_proc_info_as_uid()
> 
> So which one do you prefer to probe, __group_send_sig_info or
> group_send_sig_info?

I prefer __group_signal_sig_info, because it catches the additional
signals that you noted.  Also, consider the parallel case of
specific_send_sig_info, which is called by sys_tkill and sys_tgkill.  In
that case, the permission check happens in the sys_* functions, before
specific_send_sig_info is called.  Thus on that path we are only
capturing the signals that will actually be sent (though possibly
ignored).

This establishes a pretty consistent view of what signal.send means: if
a process successfully sends a signal, that will trigger signal.send.
Signals that are rejected because of permissions will return a failure
to the sender, and thus shouldn't trigger signal.send.  Signals that are
ignored will still report success to the sender, even though the
recipient dropped it on the floor, so it should trigger signal.send.

If you want to find signals that are rejected due to permissions, then a
different probe point will be needed, like perhaps a return probe on
check_kill_permissions.

> 2. send_sigqueue() and send_group_sigqueue() is only used for POSIX
> timers. If we choose to probe them, then I think it's better to add a
> local variable like "send_to_queue=1" to indicate that the signal is
> generated by posix timers. Then by looking at "send_to_queue" and
> "shared" local variables, we can determine which function actually
> triggers the probe handler.

Ok, that's a good suggestion.

>> * handle_stop_signal: Your comment says "fires when a stop signal is
>> sent", but this is incorrect.  This function is called to _check_
>> whether this signal is a stop/cont, and if so take special action.
>> Thus, this will get called for almost every signal, many of which
>> will not be stop signals.
> 
> Yes. You are right. But the comment is still unchanged. :-)

Indeed -- I didn't take it on myself to fix *all* of my gripes.  :)  I
didn't fix this one because I'm not convinced that it's needed, or what
useful information can be gotten from probing handle_stop_signal.  If
you really want, we could make a probe that does what the comment says,
something like this:

probe signal.send_stop = signal.send {
    if (sig != SIGSTOP) next;
}

But users can do this themselves without much trouble, and with better
granularity.  It doesn't make sense for us to enumerate send_stop,
send_cont, send_int, etc., when the user can just check the sig value
manually.

>> * syscall duplication: some of your probes are duplicating points
>> from the syscall tapset (signal.syskill, etc.).  Do we really need
>> these?  If you really want these, it would be preferable to make use
>> 	of the syscall tapset, e.g.: probe signal.syskill = syscall.kill
> 
> Yes. Agree.

Agree that they're not needed?  Or do you want the latter, where we keep
the probepoint, but alias it to the syscall tapset.


Josh


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]