On Fri, Dec 21, 2018 at 09:07:54PM +0200, Igor Stoppa wrote:
> On 21/12/2018 20:41, Matthew Wilcox wrote:
> > On Fri, Dec 21, 2018 at 08:14:14PM +0200, Igor Stoppa wrote:
> > > +static inline int memtst(void *p, int c, __kernel_size_t len)
> >
> > I don't understand why you're verifying that writes actually happen
> > in production code. Sure, write lib/test_wrmem.c or something, but
> > verifying every single rare write seems like a mistake to me.
>
> This is actually something I wrote more as a stop-gap.
> I have the feeling there should be already something similar available.
> And probably I could not find it. Unless it's so trivial that it doesn't
> deserve to become a function?
>
> But if there is really no existing alternative, I can put it in a separate
> file.
I'm not questioning the implementation, I'm questioning why it's ever
called. If I type 'p = q', I don't then verify that p actually is equal
to q. I just assume that the compiler did its job.
> > > +#ifndef CONFIG_PRMEM
> >
> > So is this PRMEM or wr_mem? It's not obvious that CONFIG_PRMEM controls
> > wrmem.
>
> In my mind (maybe still clinging to the old implementation), PRMEM is the
> master toggle, for protected memory.
>
> Then there are various types and the first one being now implemented is
> write rare after init (because ro after init already exists).
>
> However, the same levels of protection should then follow for dynamically
> allocated memory (ye old pmalloc).
>
> PRMEM would then become the moniker for the whole shebang.
To my mind, what we have in this patchset is support for statically
allocated protected (or write-rare) memory. Later, we'll add dynamically
allocated protected memory. So it's all protected memory, and we'll
use the same accessors for both ... right?
> > > +#define wr_rcu_assign_pointer(p, v) rcu_assign_pointer(p, v)
> > > +#define wr_assign(var, val) ({ \
> > > + typeof(var) tmp = (typeof(var))val; \
> > > + \
> > > + wr_memcpy(&var, &tmp, sizeof(var)); \
> > > + var; \
> > > +})
> >
> > Doesn't wr_memcpy return 'var' anyway?
>
> It should return the destination, which is &var.
>
> But I wanted to return the actual value of the assignment, val
>
> Like if I do (a = 7) it evaluates to 7,
>
> similarly wr_assign(a, 7) would also evaluate to 7
>
> The reason why i returned var instead of val is that it would allow to
> detect any error.
Ah, good point; I missed the var vs &var distinction.
> > > +void *wr_memcpy(void *p, const void *q, __kernel_size_t size)
> > > +{
> > > + wr_state_t wr_state;
> > > + void *wr_poking_addr = __wr_addr(p);
> > > +
> > > + if (WARN_ONCE(!wr_ready, "No writable mapping available") ||
> >
> > Surely not. If somebody's called wr_memcpy() before wr_ready is set,
> > that means we can just call memcpy().
>
> What I was trying to catch is the case where, after a failed init, the
> writable mapping doesn't exist. In that case wr_ready is also not set.
>
> The problem is that I just don't know what to do in a case where there has
> been such a major error which prevents he creation of hte alternate mapping.
>
> I understand that we still want to continue, to provide as much debug info
> as possible, but I am at a loss about finding the saner course of actions.
I don't think there's anything to be done in that case. Indeed,
I think the only thing to do is panic and stop the whole machine if
initialisation fails. We'd be in a situation where nothing can update
protected memory, and the machine just won't work.
I suppose we could "fail insecure" and never protect the memory, but I
think that's asking for trouble.
Anyway, my concern was for a driver which can be built either as a
module or built-in. Its init code will be called before write-protection
happens when it's built in, and after write-protection happens when it's
a module. It should be able to use wr_assign() in either circumstance.
One might also have a utility function which is called from both init
and non-init code and want to use wr_assign() whether initialisation
has completed or not.
