tracing: A few last minute clean up and fixes

Linus,

A few clean ups and bug fixes

- Replace open coded "ARRAY_SIZE()" with macro

- Updates to uprobes

- Bug fix for perf event filter on error path

Please pull the latest ktest-v4.17-2 tree, which can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-ktest.git
ktest-v4.17-2

Tag SHA1: f4a56400c3b9f949cf5740153aab48a8240f88db Head SHA1: 0b3dec05dbbce023f4f25aba975b5d253c313ebb

Howard McLauchlan (1):
tracing/uprobe: Add support for overlayfs

Jérémy Lefaure (1):
tracing: Use ARRAY_SIZE() macro instead of open coding it

Ravi Bangoria (2):
trace_uprobe: Use %lx to display offset
trace_uprobe: Simplify probes_seq_show()

Steven Rostedt (VMware) (1):
tracing: Enforce passing in filter=NULL to create_filter()

----

 kernel/trace/trace_events_filter.c | 26 +++++++++++---------------
 kernel/trace/trace_uprobe.c        | 23 ++++-------------------
 2 files changed, 15 insertions(+), 34 deletions(-)

From: Howard McLauchlan <[email protected]>

uprobes cannot successfully attach to binaries located in a directory mounted with overlayfs.

To verify, create directories for mounting overlayfs (upper,lower,work,merge), move some binary into merge/ and use readelf to obtain some known instruction of the binary. I used /bin/true and the entry instruction(0x13b0):

	$ mount -t overlay overlay -o lowerdir=lower,upperdir=upper,workdir=work merge
	$ cd /sys/kernel/debug/tracing
	$ echo 'p:true_entry PATH_TO_MERGE/merge/true:0x13b0' > uprobe_events
	$ echo 1 > events/uprobes/true_entry/enable

This returns 'bash: echo: write error: Input/output error' and dmesg tells us 'event trace: Could not enable event true_entry'

This change makes create_trace_uprobe() look for the real inode of a dentry. In the case of normal filesystems, this simplifies to just returning the inode. In the case of overlayfs(and similar fs) we will obtain the underlying dentry and corresponding inode, upon which uprobes can successfully register.

Running the example above with the patch applied, we can see that the uprobe is enabled and will output to trace as expected.

---
 kernel/trace/trace_uprobe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 268029ae1be6..8b86d76c55ee 100644
@@ -446,7 +446,7 @@ static int create_trace_uprobe(int argc, char **argv)
 	if (ret)
 		goto fail_address_parse;
 
-	inode = igrab(d_inode(path.dentry));
+	inode = igrab(d_real_inode(path.dentry));
 	path_put(&path);
 
 	if (!inode || !S_ISREG(inode->i_mode)) {

2.16.3

From: Ravi Bangoria <[email protected]>

tu->offset is unsigned long, not a pointer, thus %lx should be used to print it, not the %px.

---
 kernel/trace/trace_uprobe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8b86d76c55ee..d7d3c9237f64 100644
@@ -608,7 +608,7 @@ static int probes_seq_show(struct seq_file *m, void *v)
 
 	/* Don't print "0x  (null)" when offset is 0 */
 	if (tu->offset) {
-		seq_printf(m, "0x%px", (void *)tu->offset);
+		seq_printf(m, "0x%0*lx", (int)(sizeof(void *) * 2), tu->offset);
 	} else {
 		switch (sizeof(void *)) {
 		case 4:

2.16.3

From: Ravi Bangoria <[email protected]>

Simplify probes_seq_show() function. No change in output before and after patch.

---
 kernel/trace/trace_uprobe.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

index d7d3c9237f64..21604754bb79 100644
@@ -602,24 +602,9 @@ static int probes_seq_show(struct seq_file *m, void *v)
 	char c = is_ret_probe(tu) ? 'r' : 'p';
 	int i;
 
-	seq_printf(m, "%c:%s/%s", c, tu->tp.call.class->system,
-			trace_event_name(&tu->tp.call));
-	seq_printf(m, " %s:", tu->filename);
-
-	/* Don't print "0x  (null)" when offset is 0 */
-	if (tu->offset) {
-		seq_printf(m, "0x%0*lx", (int)(sizeof(void *) * 2), tu->offset);
-	} else {
-		switch (sizeof(void *)) {
-		case 4:
-			seq_printf(m, "0x00000000");
-			break;
-		case 8:
-		default:
-			seq_printf(m, "0x0000000000000000");
-			break;
-		}
-	}
+	seq_printf(m, "%c:%s/%s %s:0x%0*lx", c, tu->tp.call.class->system,
+			trace_event_name(&tu->tp.call), tu->filename,
+			(int)(sizeof(void *) * 2), tu->offset);
 
 	for (i = 0; i < tu->tp.nr_args; i++)
 		seq_printf(m, " %s=%s", tu->tp.args[i].name, tu->tp.args[i].comm);

2.16.3

From: "Steven Rostedt (VMware)" <[email protected]>

There's some inconsistency with what to set the output parameter filterp when passing to create_filter(..., struct event_filter **filterp).

Whatever filterp points to, should be NULL when calling this function. The create_filter() calls create_filter_start() with a pointer to a local "filter" variable that is set to NULL. The create_filter_start() has a WARN_ON() if the passed in pointer isn't pointing to a value set to NULL.

Ideally, create_filter() should pass the filterp variable it received to create_filter_start() and not hide it as with a local variable, this allowed create_filter() to fail, and not update the passed in filter, and the caller of create_filter() then tried to free filter, which was never initialized to anything, causing memory corruption.

---
 kernel/trace/trace_events_filter.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

index 5eba1cec945c..9b4716bb8bb0 100644
@@ -1704,18 +1704,16 @@ static int create_filter(struct trace_event_call *call,
 			 struct event_filter **filterp)
 {
 	struct filter_parse_error *pe = NULL;
-	struct event_filter *filter = NULL;
 	int err;
 
-	err = create_filter_start(filter_string, set_str, &pe, &filter);
+	err = create_filter_start(filter_string, set_str, &pe, filterp);
 	if (err)
 		return err;
 
-	err = process_preds(call, filter_string, filter, pe);
+	err = process_preds(call, filter_string, *filterp, pe);
 	if (err && set_str)
-		append_filter_err(pe, filter);
+		append_filter_err(pe, *filterp);
 
-	*filterp = filter;
 	return err;
 }
 

@@ -1739,24 +1737,22 @@ static int create_system_filter(struct trace_subsystem_dir *dir,
 				struct trace_array *tr,
 				char *filter_str, struct event_filter **filterp)
 {
-	struct event_filter *filter = NULL;
 	struct filter_parse_error *pe = NULL;
 	int err;
 
-	err = create_filter_start(filter_str, true, &pe, &filter);
+	err = create_filter_start(filter_str, true, &pe, filterp);
 	if (!err) {
 		err = process_system_preds(dir, tr, pe, filter_str);
 		if (!err) {
 			/* System filters just show a default message */
-			kfree(filter->filter_string);
-			filter->filter_string = NULL;
+			kfree((*filterp)->filter_string);
+			(*filterp)->filter_string = NULL;
 		} else {
-			append_filter_err(pe, filter);
+			append_filter_err(pe, *filterp);
 		}
 	}
 	create_filter_finish(pe);
 
-	*filterp = filter;
 	return err;
 }
 

@@ -1764,7 +1760,7 @@ static int create_system_filter(struct trace_subsystem_dir *dir,
 int apply_event_filter(struct trace_event_file *file, char *filter_string)
 {
 	struct trace_event_call *call = file->event_call;
-	struct event_filter *filter;
+	struct event_filter *filter = NULL;
 	int err;
 
 	if (!strcmp(strstrip(filter_string), "0")) {

@@ -1817,7 +1813,7 @@ int apply_subsystem_event_filter(struct trace_subsystem_dir *dir,
 {
 	struct event_subsystem *system = dir->subsystem;
 	struct trace_array *tr = dir->tr;
-	struct event_filter *filter;
+	struct event_filter *filter = NULL;
 	int err = 0;
 
 	mutex_lock(&event_mutex);

@@ -2024,7 +2020,7 @@ int ftrace_profile_set_filter(struct perf_event *event, int event_id,
 			      char *filter_str)
 {
 	int err;
-	struct event_filter *filter;
+	struct event_filter *filter = NULL;
 	struct trace_event_call *call;
 
 	mutex_lock(&event_mutex);

2.16.3

That doesn't exist, and makes no sense. Did you mean tracing, not ktest?

And even if you did, why did your scripts not notice that the branch you point to doesn't actually exist?

Linus

Oh, it's more f'ed up than that. That "ktest-v4.17-2" tag does exist - in the tracing tree.

So the tag is mis-named, but in the right tree, and then your pull request points at the wrong tree.

Do you want to fix that all up, or should I just pull from that messed-up thing?

Linus

Grumble, no don't pull it. Let me fix it.

That's what I get when I up-arrow commands. I'll retag it and make sure it's fine, and send you another pull request.

Oh, and I just received my nitrokey and I'm playing with it. One thing I found is that it doesn't like keys sized 3072 (gives me a bad key error), which is my current subkey. So I created a new subkey:

Primary key fingerprint: 5ED9 A48F C54C 0A22 D1D0 804C EBC2 6CDB 5A56 DE73

     Subkey fingerprint: B5D7 BDD5 67E0 67A3 EE0C  9FBE 3F0B D661 FC59 E3D3

That is 4096. I pushed it to the key servers about a half hour ago, hopefully, it has made its way around, as I'm going to start using it.

-- Steve

I semi-heartily recommend using an ECDSA key instead (ED25519), because it will be much faster and we know they work fine for Linus since jejb uses them to sign his tags. My understanding is that 4096-bit RSA keys are pretty slow on Nitrokey Start devices.

If you want to stick to RSA instead of the new-fangled ECC (if you can call a standard from 1980s new-fangled, that is), there is really no benefit to having a signing subkey stronger than 2048 bits, especially for the purposes of signing git objects -- which are only as strong as SHA1.

Regards,

Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation

Don't use this key.

Konstantin wanted me to make a ECC key, which I did. Here's the new key:

Subkey fingerprint: 514B 0EDE 3C38 7F94 4FB3 7993 29E5 7410 9AEB FAAA

-- Steve

Nice!

You don't actually have to tell people the fingerprint, as the subkey will inherit the trust/validity of your master key. For the recipients, it's sufficient to just refresh your key:

gpg2 --refresh-key [email protected]

To validate ECC tags, you will need to tell git to always use gpg2, since gpg1 doesn't know what ECC is:

git config --global gpg.program gpg2
git config --global gpgv.program gpgv2

Regards,

Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation