On Fri, Jul 17, 2020 at 12:20:56AM -0700, ira.weiny@intel.com wrote:
First I'm not sure if adding this state to idtentry_state and having that state copied is the right way to go. It seems like we should start passing this by reference instead of value. But for now this works as an RFC. Comments?
As long as you keep sizeof(struct idtentry_state_t) <= sizeof(u64) or possibly 2*sizeof(unsigned long), code gen shouldn't be too horrid IIRC. You'll have to look at what the compiler makes of it.
Second, I'm not 100% happy with having to save the reference count in the exception handler. It seems like a very ugly layering violation but I don't see a way around it at the moment.
So I've been struggling with that API, all the way from pks_update_protection() to that dev_access_{en,dis}able(). I _really_ hate it, but I see how you ended up with it.
I wanted to propose something like:
u32 current_pkey_save(int pkey, unsigned flags) { u32 *lpkr = get_cpu_ptr(&local_pkr); u32 pkr, saved = *lpkr;
pkr = update_pkey_reg(saved, pkey, flags); if (pkr != saved) wrpkr(pkr);
put_cpu_ptr(&local_pkr); return saved; }
void current_pkey_restore(u32 pkr) { u32 *lpkr = get_cpu_ptr(&local_pkr); if (*lpkr != pkr) wrpkr(pkr); put_cpu_ptr(&local_pkr); }
Together with:
void pkey_switch(struct task_struct *prev, struct task_struct *next) { prev->pkr = this_cpu_read(local_pkr); if (prev->pkr != next->pkr) wrpkr(next->pkr); }
But that's actually hard to frob into the kmap() model :-( The upside is that you only have 1 word of state, instead of the 2 you have now.
Third, this patch has gone through a couple of revisions as I've had crashes which just don't make sense to me. One particular issue I've had is taking a MCE during memcpy_mcsafe causing my WARN_ON() to fire. The code path was a pmem copy and the ref count should have been elevated due to dev_access_enable() but why was idtentry_enter()->idt_save_pkrs() not called I don't know.
Because MCEs are NMI-like and don't go through the normal interrupt path. MCEs are an abomination, please wear all the protective devices you can lay hands on when delving into that.