Message-ID: <20170104121425.GP14217@n2100.armlinux.org.uk>
Date:   Wed, 4 Jan 2017 12:14:25 +0000
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Julia Lawall <julia.lawall@...6.fr>
Cc:     Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        Kees Cook <keescook@...omium.org>, andrew@...n.ch,
        Jason Cooper <jason@...edaemon.net>,
        rtc-linux@...glegroups.com, a.zummo@...ertech.it,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        gregory.clement@...e-electrons.com,
        Bhumika Goyal <bhumirks@...il.com>,
        sebastian.hesselbarth@...il.com
Subject: Re: [PATCH] rtc: armada38x: add __ro_after_init to armada38x_rtc_ops

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.

Powered by blists - more mailing lists