Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

[Russell King - ARM Linux]

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.