linux-kernel - Re: [RFC] mmiotrace full patch, preview 1

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 26 Feb 2008 10:20:08 -0700
From:	corbet@....net (Jonathan Corbet)
To:	Pekka Paalanen <pq@....fi>
Cc:	Ingo Molnar <mingo@...e.hu>, Christoph Hellwig <hch@...radead.org>,
	Arjan van de Ven <arjan@...radead.org>,
	Pavel Roskin <proski@....org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC] mmiotrace full patch, preview 1 

Hey, Pekka,

A couple of little things I noticed...

> +static int post_kmmio_handler(unsigned long condition, struct pt_regs *regs)
> +{
> +	int ret = 0;
> +	struct kmmio_probe *probe;
> +	struct kmmio_fault_page *faultpage;
> +	struct kmmio_context *ctx = &get_cpu_var(kmmio_ctx);
> +
> +	if (!ctx->active)
> +		goto out;

Should that text read something like:

	if (condition != DIE_TRAP || !ctx->active)

Presumably you won't be active if something else is going wrong, but one
never knows.

> +int register_kmmio_probe(struct kmmio_probe *p)
> +{
> +	int ret = 0;
> +	unsigned long size = 0;
> +
> +	spin_lock_irq(&kmmio_lock);
> +	kmmio_count++;
> +	if (get_kmmio_probe(p->addr)) {
> +		ret = -EEXIST;
> +		goto out;
> +	}

That only checks the first page; if the probed region partially overlaps
another one found later in memory, the registration will succeed.

Maybe you want to decrement kmmio_count if you decide to return -EEXIST
(or hold off on the increment until after the test)?

In general, I worry about what happens if an interrupt handler generates
traced MMIO traffic while a fault handler is active.  It looks a lot
like the "all hell breaks loose" scenario mentioned in the comments.  Is
there some way of preventing that which I missed?  Otherwise, maybe,
should the ioremap() wrappers take an additional argument, being an IRQ
to disable while trace handlers are active?

jon
--
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/