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
| ||
|
Date: Thu, 21 Mar 2013 12:03:52 +0900 From: Keun-O Park <kpark3469@...il.com> To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com> Cc: Steven Rostedt <rostedt@...dmis.org>, keun-o.park@...driver.com, linux-kernel@...r.kernel.org Subject: Re: [PATCH] tracepoints: prevents null probe from being added On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote: > * Keun-O Park (kpark3469@...il.com) wrote: >> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <rostedt@...dmis.org> wrote: >> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote: >> >> * Steven Rostedt (rostedt@...dmis.org) wrote: >> >> > On Wed, 2013-03-20 at 12:18 +0900, kpark3469@...il.com wrote: >> >> > > From: Sahara <keun-o.park@...driver.com> >> >> > > >> >> > > Somehow tracepoint_entry_add/remove_probe functions allow a null probe >> >> > > function. >> >> > >> >> > You actually hit this in practice, or is this just something that you >> >> > observe from code review? >> >> > >> >> > > Especially on getting a null probe in remove function, it seems >> >> > > to be used to remove all probe functions in the entry. >> >> > >> >> > Hmm, that actually sounds like a feature. >> >> >> >> Yep. It's been a long time since I wrote this code, but the removal code >> >> seems to use NULL probe pointer to remove all probes for a given >> >> tracepoint. >> >> >> >> I'd be tempted to just validate non-NULL probe within >> >> tracepoint_entry_add_probe() and let other sites as is, just in case >> >> anyone would be using this feature. >> >> >> >> I cannot say that I have personally used this "remove all" feature much >> >> though. >> >> >> > >> > I agree. I don't see anything wrong in leaving the null probe feature in >> > the removal code. But updating the add code looks like a proper change. >> > >> > -- Steve >> > >> > >> >> Hello Steve & Mathieu, >> If we want to leave the null probe feature enabled, I think it would >> be better modifying the code like the following for code efficiency. >> >> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry, >> int nr_probes = 0; >> struct tracepoint_func *old, *new; >> >> - WARN_ON(!probe); >> + if (WARN_ON(!probe)) >> + return ERR_PTR(-EINVAL); >> >> debug_print_probes(entry); >> old = entry->funcs; >> @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *ent >> >> debug_print_probes(entry); >> /* (N -> M), (N > 1, M >= 0) probes */ >> - for (nr_probes = 0; old[nr_probes].func; nr_probes++) { >> - if (!probe || >> - (old[nr_probes].func == probe && >> - old[nr_probes].data == data)) >> - nr_del++; >> + if (probe) { >> + for (nr_probes = 0; old[nr_probes].func; nr_probes++) { >> + if (old[nr_probes].func == probe && >> + old[nr_probes].data == data) >> + nr_del++; >> + } >> } >> >> - if (nr_probes - nr_del == 0) { >> + if (!probe || nr_probes - nr_del == 0) { > > We might want to do: > > if (probe) { > ... > } else { > nr_del = nr_probes; > } > > if (nr_probes - nr_del == 0) { > ... > } This code has a problem. nr_probes is initialized as zero. And, in order to get correct count of probes, we need to go through the for-loop even though probe is null. So with above code, nr_del will be zero. Anyhow, the code will fall through if-clause(nr_probes-nr_del==0). It looks odd to me. -- Kpark > > rather than: > > if (probe) { > ... > } > > if (!probe || nr_probes - nr_del == 0) { > ... > } > > Using nr_del makes the code easier to follow IMHO. > > Thanks, > > Mathieu > -- 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/
Powered by blists - more mailing lists