linux-kernel - Re: [PATCH V8 09/13] posix clocks: introduce dynamic clocks

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:	Thu, 6 Jan 2011 20:56:22 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Richard Cochran <richardcochran@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
	netdev@...r.kernel.org, Alan Cox <alan@...rguk.ukuu.org.uk>,
	Christoph Lameter <cl@...ux.com>,
	David Miller <davem@...emloft.net>,
	John Stultz <johnstul@...ibm.com>,
	Krzysztof Halasa <khc@...waw.pl>,
	Peter Zijlstra <peterz@...radead.org>,
	Rodolfo Giometti <giometti@...ux.it>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH V8 09/13] posix clocks: introduce dynamic clocks

On Friday 31 December 2010, Richard Cochran wrote:
> This patch adds support for adding and removing posix clocks. The
> clock lifetime cycle is patterned after usb devices. Each clock is
> represented by a standard character device. In addition, the driver
> may optionally implemented custom character device operations.
> 
> The posix clock and timer system calls listed below now work with
> dynamic posix clocks, as well as the traditional static clocks.
> For the performance critical calls (eg clock_gettime) the code path
> from the traditional static clocks is preserved.

Combining the operations structures and using container_of as you did
looks much better than before, but I had something slightly different
in mind:

The way that other subsystems do this is to pass a pointer to the
actual low-level object (struct posix_clock in your case) to the
abstracted functions, while you pass a pointer to the operations
structure. This has the advantage of keeping the definition of
posix_clock private to posix-clock.c, but it is something we do
very rarely.

I can see disadvantages with your approach: You still need to dynamically
allocate the posix_clock in posix_clock_create(), and the operations
structure cannot be const, which is a theoretical security problem
if there is a hole that can replace one of the pointers with a
reference to user memory.

I would recommend changing this to the more common model, where you
passs the (publically defined) struct posix_clock to all operations
and can do something like:

struct my_posix_clock {
	...
	struct posix_clock pclk;
} this_clock = {
	...
	.pclk = {
		.ops = &my_posix_clock_operations,
	}
};

int my_init(void)
{
	return posix_clock_operations_register(&my_posix_clock.pclk);
}

void my_exit(void)
{
	posix_clock_operations_unregister(&my_posix_clock.pclk);
}

It should be just a trivial change and just affect how easy it is for
other people to understand the code if they are already familiar
with other kernel code.

Overall, your series looks really good now, it would be nice if this
could still make it into 2.6.38.

	Arnd

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