linux-kernel - Re: [patch 1/4] x86, bts: add selftest for BTS

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]

Message-ID: <20090306143237.GC21907@elte.hu>
Date:	Fri, 6 Mar 2009 15:32:37 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Markus Metzger <markus.t.metzger@...el.com>
Cc:	linux-kernel@...r.kernel.org, tglx@...utronix.de, hpa@...or.com,
	markus.t.metzger@...il.com, roland@...hat.com,
	eranian@...glemail.com, oleg@...hat.com, juan.villacis@...el.com,
	ak@...ux.jf.intel.com
Subject: Re: [patch 1/4] x86, bts: add selftest for BTS


* Markus Metzger <markus.t.metzger@...el.com> wrote:

> +#ifdef CONFIG_X86_DS_SELFTEST
> +static int ds_selftest_bts_consistency(const struct bts_trace *trace)

Consider putting this into ds_selftest.c instead of adding a 
large #ifdef block to ds.c.

> +	if (trace->ds.end !=
> +	    (char *) trace->ds.begin + (trace->ds.n * trace->ds.size)) {

That extra space character after the type cast is not 
needed.

> +	/* Check a few things which do not belong to this test.
> +	 * They should be covered by other tests. */

Please use the customary comment style of:

  /*
   * Comment .....
   * ...... goes here:
   */

> +		index = ((void *) at - trace->ds.begin) / trace->ds.size;

cast.

> +#define DS_SELFTEST_SIZEOF_BUFFER 1021 /* intentionally chose an odd size */
> +static int ds_selftest_bts(void)

We put newlines after definitions.

Also, DS_SELFTEST_SIZEOF_BUFFER is an odd name - why not 
DS_SELFTEST_BUFFER_SIZE ?

> +	tracer =
> +		ds_request_bts(/* task = */ NULL, buffer,
> +			       DS_SELFTEST_SIZEOF_BUFFER,
> +			       /* ovfl = */ NULL, /* th = */ (size_t)-1,
> +			       BTS_KERNEL);

that looks weird. Why not:

	tracer = ds_request_bts(NULL, buffer, DS_SELFTEST_BUFFER_SIZE,
				NULL, (size_t)-1, BTS_KERNEL);

> +	/* The return from the initialization call should already give
> +	 * us enough trace. */

Comment style.

> +		/* It is possible but highly unlikely that we got a
> +		 * buffer overflow and end up at exactly the same
> +		 * position we started from.
> +		 * Let's issue a warning, but continue. */

ditto.

> +	/* Let's read the trace again. Since we suspended tracing, we should
> +	 * get the same result. */

ditto.

> +		/* It is possible but highly unlikely that we got a
> +		 * buffer overflow and end up at exactly the same
> +		 * position we started from.
> +		 * Let's issue a warning and check the full trace. */

ditto.

> +		/* We had a buffer overflow - the entire buffer should
> +		 * contain trace records. */

ditto.

> +		/* It is quite likely that the buffer did not
> +		 * overflow. Let's just check the delta trace. */

ditto.

> +#ifdef CONFIG_X86_DS_SELFTEST
> +	if (ds_cfg.ctl[dsf_bts]) {
> +		int error;
> +
> +		printk(KERN_INFO "[ds] bts selftest...");
> +		error = ds_selftest_bts();
> +		printk(KERN_CONT "%s.\n", (error ? "failed" : "passed"));
> +
> +		if (error) {
> +			WARN(1, "[ds] selftest failed. disabling bts.\n");
> +			ds_cfg.ctl[dsf_bts] = 0;
> +		}
> +	}
> +#endif /* CONFIG_X86_DS_SELFTEST */

This bit should be in a helper function i guess - so the ugly 
#ifdef goes out of a function.

Thanks,

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