linux-kernel - Re: Q: tracing: can we change trace_signal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1321905799.20742.20.camel@frodo>
Date:	Mon, 21 Nov 2011 15:03:19 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>, Jiri Olsa <jolsa@...hat.com>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Seiji Aguchi <saguchi@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: Q: tracing: can we change trace_signal_generate() signature?

On Mon, 2011-11-21 at 20:19 +0100, Oleg Nesterov wrote:
> Hello,
> 
> Is it possible to change trace_signal_generate()'s args or this
> is the part of the kernel ABI?

As Linus said. It's only part of the ABI if a tool is using it. If you
change it and no one complains, then it should be good to go.

> 
> We have the "feature request". The customer wants to know was the
> signal delivered or not, and why. We could add another trace_()
> into __send_signal() but this looks ugly to me.
> 
> So. Can't we add
> 
> 	enum {
> 		TRACE_SIGNAL_DELIVERED,
> 		TRACE_SIGNAL_IGNORED_OR_BLOCKED,
> 		TRACE_SIGNAL_ALREADY_PENDING,
> 	}
> 
> and move trace_signal_generate() to the end of __send_signal()
> with the additional argument(s) to avoid the new tracepoint?
> 
> If yes, then can't we also kill trace_signal_overflow_fail()
> and trace_signal_lose_info()? We can simply add more
> TRACE_SIGNAL_'s instead, this certainly looks better imho.

Again, if no tool relies on it, it should be fine.

If we were finally able to get a library for tools to read tracepoints,
then we could add and move them around with no issue.

> 
> IOW. Ignoring the changes in include/trace/events/signal.h,
> can the patch below work or the changes like this are not
> allowed?

I say change it and see who screams.

> 
> See also https://bugzilla.redhat.com/show_bug.cgi?id=738720
> 
> Thanks,
> 
> Oleg.
> 
> 
> --- x/kernel/signal.c
> +++ x/kernel/signal.c
> @@ -1019,19 +1019,27 @@ static inline int legacy_queue(struct sigpending *signals, int sig)
>  	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
>  }
>  
> +enum {
> +	TRACE_SIGNAL_DELIVERED,
> +	TRACE_SIGNAL_IGNORED_OR_BLOCKED,
> +	TRACE_SIGNAL_ALREADY_PENDING,
> +	TRACE_SIGNAL_OVERFLOW_FAIL,
> +	TRACE_SIGNAL_LOSE_INFO,
> +};
> +
>  static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
>  			int group, int from_ancestor_ns)
>  {
>  	struct sigpending *pending;
>  	struct sigqueue *q;
>  	int override_rlimit;
> -
> -	trace_signal_generate(sig, info, t);
> +	int ret = 0, result;
>  
>  	assert_spin_locked(&t->sighand->siglock);
>  
> +	result = TRACE_SIGNAL_IGNORED_OR_BLOCKED;
>  	if (!prepare_signal(sig, t, from_ancestor_ns))
> -		return 0;
> +		goto ret;
>  
>  	pending = group ? &t->signal->shared_pending : &t->pending;
>  	/*
> @@ -1039,8 +1047,11 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
>  	 * exactly one non-rt signal, so that we can get more
>  	 * detailed information about the cause of the signal.
>  	 */
> +	result = TRACE_SIGNAL_ALREADY_PENDING;
>  	if (legacy_queue(pending, sig))
> -		return 0;
> +		goto ret;
> +
> +	result = TRACE_SIGNAL_DELIVERED;
>  	/*
>  	 * fast-pathed signals for kernel-internal things like SIGSTOP
>  	 * or SIGKILL.
> @@ -1095,14 +1106,15 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
>  			 * signal was rt and sent by user using something
>  			 * other than kill().
>  			 */
> -			trace_signal_overflow_fail(sig, group, info);
> -			return -EAGAIN;
> +			result = TRACE_SIGNAL_OVERFLOW_FAIL;
> +			ret = -EAGAIN;
> +			goto ret;
>  		} else {
>  			/*
>  			 * This is a silent loss of information.  We still
>  			 * send the signal, but the *info bits are lost.
>  			 */
> -			trace_signal_lose_info(sig, group, info);
> +			result = TRACE_SIGNAL_LOSE_INFO;

Hmm, all this result manipulation added for tracing that doesn't occur
in 99.99% of all machines?

-- Steve


>  		}
>  	}
>  
> @@ -1110,7 +1122,9 @@ out_set:
>  	signalfd_notify(t, sig);
>  	sigaddset(&pending->signal, sig);
>  	complete_signal(sig, t, group);
> -	return 0;
> +ret:
> +	trace_signal_generate(sig, info, t, group, result);
> +	return ret;
>  }
>  
>  static int send_signal(int sig, struct siginfo *info, struct task_struct *t,


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/