From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758074AbdADMP4 (ORCPT ); Wed, 4 Jan 2017 07:15:56 -0500 Received: from pandora.armlinux.org.uk ([78.32.30.218]:39382 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757449AbdADMOt (ORCPT ); Wed, 4 Jan 2017 07:14:49 -0500 Date: Wed, 4 Jan 2017 12:14:25 +0000 From: Russell King - ARM Linux To: Julia Lawall Cc: Alexandre Belloni , Kees Cook , andrew@lunn.ch, Jason Cooper , rtc-linux@googlegroups.com, a.zummo@towertech.it, LKML , "linux-arm-kernel@lists.infradead.org" , gregory.clement@free-electrons.com, Bhumika Goyal , sebastian.hesselbarth@gmail.com Subject: Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops Message-ID: <20170104121425.GP14217@n2100.armlinux.org.uk> References: <1482751862-18699-1-git-send-email-bhumirks@gmail.com> <20170102140654.GF14217@n2100.armlinux.org.uk> <20170103213118.GM14217@n2100.armlinux.org.uk> <20170103215421.GN14217@n2100.armlinux.org.uk> <20170104110750.dtu54t74qkuuvkvq@piout.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 04, 2017 at 12:43:32PM +0100, Julia Lawall wrote: > > The question was whether the point to the rtc_class_ops could be made > > __ro_after_init. And Russell is right, it is pointed to by the ops > > pointer in a struct rtc_device and that struct is dynamically allocated > > in rtc_device_register(). > > OK, I think it's a terminology issue. You mean the structure that > contains the pointer, and not the pointer itself, which is already const. That statement is really ambiguous, and really doesn't help the cause - we have several structures here which contain pointers and it's far from clear which you're talking about: - The armada38x_rtc_ops and armada38x_rtc_ops_noirq structures contain pointers to functions. - The dynamically allocated struct rtc_device contains an ops pointer, which will point at one or other of these two structures. Now, as soon as we make armada38x_rtc_ops and armada38x_rtc_ops_noirq const, if the pointer is passed through any function call where the argument is not also marked const, or is assigned to a pointer that is not marked const (without an explicit cast), the compiler will complain. Remember that a const pointer (iow, const void *ptr) is just a hint to the compiler that "ptr" _may_ point at read-only data, and dereferences of the pointer are not allowed to write - it's just syntactic checking. Given that this is stuff we should all know, I'm not quite sure what people are getting in a tiz over... I'm finding it worrying that I'm even writing this mail, reviewing this stuff! _Really_ worried that Kees even brought it up in the first place - I suspect that came from a misunderstanding of my suggestion which is why I later provided the suggestion in patch form. What I suggested, and what my patch does is: 1. It places both the armada38x_rtc_ops and armada38x_rtc_ops_noirq structures into the .rodata section, which will be protected from writes by hardware when appropriate kernel options are enabled. 2. The driver does _not_ store a local pointer to this memory at a static address which could be subsequently modified (*). 3. The only pointer to this memory is during driver initialisation (which may well reside in a CPU register only) before being passed to the RTC subsystem. 4. The RTC subsystem dynamically allocates a struct rtc_device structure (in rtc_device_register()) where it eventually stores this pointer. This pointer is already marked const. This structure contains read/write data, and can't be marked read-only, just in the same way as "struct file" can't be. The whole __ro_after_init thing is completely irrelevant and a total distraction at this point - there is nothing that you could add a __ro_after_init annotation to after my patch in regard of these ops structures. * - however, a compiler may decide to store the addresses of these structures as a literal constant near the function, but with RONX protection for the .text section, this memory is also read-only, and so can't be modified. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.