| Message ID | 72dd5f38c1fdbc4c532f8caf2d2010f1ddfa8439.1543903910.git.alison.schofield@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multi-Key Total Memory Encryption API (MKTME) | expand |
On Mon, Dec 03, 2018 at 11:39:58PM -0800, Alison Schofield wrote: > +struct mktme_hw_program_info { > + struct mktme_key_program *key_program; > + unsigned long status; > +}; > + > +/* Program a KeyID on a single package. */ > +static void mktme_program_package(void *hw_program_info) > +{ > + struct mktme_hw_program_info *info = hw_program_info; > + int ret; > + > + ret = mktme_key_program(info->key_program); > + if (ret != MKTME_PROG_SUCCESS) > + WRITE_ONCE(info->status, ret); What's the purpose of that WRITE_ONCE()? > +} > + > +/* Program a KeyID across the entire system. */ > +static int mktme_program_system(struct mktme_key_program *key_program, > + cpumask_var_t mktme_cpumask) > +{ > + struct mktme_hw_program_info info = { > + .key_program = key_program, > + .status = MKTME_PROG_SUCCESS, > + }; > + get_online_cpus(); > + on_each_cpu_mask(mktme_cpumask, mktme_program_package, &info, 1); > + put_online_cpus(); > + > + return info.status; > +} > + > /* Copy the payload to the HW programming structure and program this KeyID */ > static int mktme_program_keyid(int keyid, struct mktme_payload *payload) > { > @@ -84,7 +116,7 @@ static int mktme_program_keyid(int keyid, struct mktme_payload *payload) > kprog->key_field_2[i] ^= kern_entropy[i]; > } > } > - ret = mktme_key_program(kprog); > + ret = mktme_program_system(kprog, mktme_leadcpus); > kmem_cache_free(mktme_prog_cache, kprog); > return ret; > } > @@ -299,6 +331,28 @@ struct key_type key_type_mktme = { > .destroy = mktme_destroy_key, > }; > > +static int mktme_build_leadcpus_mask(void) > +{ > + int online_cpu, mktme_cpu; > + int online_pkgid, mktme_pkgid = -1; > + > + if (!zalloc_cpumask_var(&mktme_leadcpus, GFP_KERNEL)) > + return -ENOMEM; > + > + for_each_online_cpu(online_cpu) { > + online_pkgid = topology_physical_package_id(online_cpu); > + > + for_each_cpu(mktme_cpu, mktme_leadcpus) { > + mktme_pkgid = topology_physical_package_id(mktme_cpu); > + if (mktme_pkgid == online_pkgid) > + break; > + } > + if (mktme_pkgid != online_pkgid) > + cpumask_set_cpu(online_cpu, mktme_leadcpus); Do you really need LOCK prefixed bit set here? > + } > + return 0; > +} How is that serialized and kept relevant in the face of hotplug? Also, do you really need O(n^2) to find the first occurence of a value in an array?
On Tue, Dec 04, 2018 at 09:21:45AM +0000, Peter Zijlstra wrote: > On Mon, Dec 03, 2018 at 11:39:58PM -0800, Alison Schofield wrote: > > > +struct mktme_hw_program_info { > > + struct mktme_key_program *key_program; > > + unsigned long status; > > +}; > > + > > +/* Program a KeyID on a single package. */ > > +static void mktme_program_package(void *hw_program_info) > > +{ > > + struct mktme_hw_program_info *info = hw_program_info; > > + int ret; > > + > > + ret = mktme_key_program(info->key_program); > > + if (ret != MKTME_PROG_SUCCESS) > > + WRITE_ONCE(info->status, ret); > > What's the purpose of that WRITE_ONCE()? [I suggested the code to Alison.] Yes, you're right. Simple assignment will do.
On Tue, Dec 04, 2018 at 10:21:45AM +0100, Peter Zijlstra wrote: > On Mon, Dec 03, 2018 at 11:39:58PM -0800, Alison Schofield wrote: > > > +static int mktme_build_leadcpus_mask(void) > > +{ > > + int online_cpu, mktme_cpu; > > + int online_pkgid, mktme_pkgid = -1; > > + > > + if (!zalloc_cpumask_var(&mktme_leadcpus, GFP_KERNEL)) > > + return -ENOMEM; > > + > > + for_each_online_cpu(online_cpu) { > > + online_pkgid = topology_physical_package_id(online_cpu); > > + > > + for_each_cpu(mktme_cpu, mktme_leadcpus) { > > + mktme_pkgid = topology_physical_package_id(mktme_cpu); > > + if (mktme_pkgid == online_pkgid) > > + break; > > + } > > + if (mktme_pkgid != online_pkgid) > > + cpumask_set_cpu(online_cpu, mktme_leadcpus); > > Do you really need LOCK prefixed bit set here? No. Changed to __cpumask_set_cpu(). Will check for other instances where I can skip LOCK prefix. > How is that serialized and kept relevant in the face of hotplug? mktme_leadcpus is updated on hotplug startup and teardowns. > Also, do you really need O(n^2) to find the first occurence of a value > in an array? How about this O(n)? unsigned long *pkg_map; int cpu, pkgid; if (!zalloc_cpumask_var(&mktme_leadcpus, GFP_KERNEL)) return -ENOMEM; pkg_map = bitmap_zalloc(topology_max_packages(), GFP_KERNEL); if (!pkg_map) { free_cpumask_var(mktme_leadcpus); return -ENOMEM; } for_each_online_cpu(cpu) { pkgid = topology_physical_package_id(cpu); if (!test_and_set_bit(pkgid, pkg_map)) __cpumask_set_cpu(cpu, mktme_leadcpus); }
On Tue, Dec 04, 2018 at 12:50:09PM +0300, Kirill A. Shutemov wrote: > On Tue, Dec 04, 2018 at 09:21:45AM +0000, Peter Zijlstra wrote: > > On Mon, Dec 03, 2018 at 11:39:58PM -0800, Alison Schofield wrote: > > > > > +struct mktme_hw_program_info { > > > + struct mktme_key_program *key_program; > > > + unsigned long status; > > > +}; > > > + > > > +/* Program a KeyID on a single package. */ > > > +static void mktme_program_package(void *hw_program_info) > > > +{ > > > + struct mktme_hw_program_info *info = hw_program_info; > > > + int ret; > > > + > > > + ret = mktme_key_program(info->key_program); > > > + if (ret != MKTME_PROG_SUCCESS) > > > + WRITE_ONCE(info->status, ret); > > > > What's the purpose of that WRITE_ONCE()? > > [I suggested the code to Alison.] > > Yes, you're right. Simple assignment will do. Will do. Thanks! > > -- > Kirill A. Shutemov
On Tue, Dec 04, 2018 at 09:43:53PM -0800, Alison Schofield wrote: > On Tue, Dec 04, 2018 at 10:21:45AM +0100, Peter Zijlstra wrote: > > On Mon, Dec 03, 2018 at 11:39:58PM -0800, Alison Schofield wrote: > > > > > +static int mktme_build_leadcpus_mask(void) > > > +{ > > > + int online_cpu, mktme_cpu; > > > + int online_pkgid, mktme_pkgid = -1; > > > + > > > + if (!zalloc_cpumask_var(&mktme_leadcpus, GFP_KERNEL)) > > > + return -ENOMEM; > > > + > > > + for_each_online_cpu(online_cpu) { > > > + online_pkgid = topology_physical_package_id(online_cpu); > > > + > > > + for_each_cpu(mktme_cpu, mktme_leadcpus) { > > > + mktme_pkgid = topology_physical_package_id(mktme_cpu); > > > + if (mktme_pkgid == online_pkgid) > > > + break; > > > + } > > > + if (mktme_pkgid != online_pkgid) > > > + cpumask_set_cpu(online_cpu, mktme_leadcpus); > > > > Do you really need LOCK prefixed bit set here? > No. Changed to __cpumask_set_cpu(). Will check for other instances > where I can skip LOCK prefix. > > > How is that serialized and kept relevant in the face of hotplug? > mktme_leadcpus is updated on hotplug startup and teardowns. Not in this patch it is not. That is added in a subsequent patch, which means that during bisection hotplug is utterly wrecked if you happen to land between these patches, that is bad. > > Also, do you really need O(n^2) to find the first occurence of a value > > in an array? > How about this O(n)? > > unsigned long *pkg_map; > int cpu, pkgid; > > if (!zalloc_cpumask_var(&mktme_leadcpus, GFP_KERNEL)) > return -ENOMEM; > > pkg_map = bitmap_zalloc(topology_max_packages(), GFP_KERNEL); > if (!pkg_map) { > free_cpumask_var(mktme_leadcpus); > return -ENOMEM; > } > for_each_online_cpu(cpu) { > pkgid = topology_physical_package_id(cpu); > if (!test_and_set_bit(pkgid, pkg_map)) You again don't need that LOCK prefix here. __test_and_set_bit() :-) > __cpumask_set_cpu(cpu, mktme_leadcpus); > } Right.
On Wed, Dec 05, 2018 at 10:10:29AM +0100, Peter Zijlstra wrote: > On Tue, Dec 04, 2018 at 09:43:53PM -0800, Alison Schofield wrote: > > On Tue, Dec 04, 2018 at 10:21:45AM +0100, Peter Zijlstra wrote: > > > On Mon, Dec 03, 2018 at 11:39:58PM -0800, Alison Schofield wrote: > > > > > How is that serialized and kept relevant in the face of hotplug? > > mktme_leadcpus is updated on hotplug startup and teardowns. > > Not in this patch it is not. That is added in a subsequent patch, which > means that during bisection hotplug is utterly wrecked if you happen to > land between these patches, that is bad. > The Key Service support is split between 4 main patches (10-13), but the dependencies go further back in the patchset. If the bisect need outweighs any benefit from reviewing in pieces, then these patches can be squashed to a single patch: keys/mktme: Add the MKTME Key Service type for memory encryption keys/mktme: Program memory encryption keys on a system wide basis keys/mktme: Save MKTME data if kernel cmdline parameter allows keys/mktme: Support CPU Hotplug for MKTME keys Am I interpreting your point correctly? Thanks, Alison
diff --git a/security/keys/mktme_keys.c b/security/keys/mktme_keys.c index e615eb58e600..7f113146acf2 100644 --- a/security/keys/mktme_keys.c +++ b/security/keys/mktme_keys.c @@ -21,6 +21,7 @@ #include "internal.h" struct kmem_cache *mktme_prog_cache; /* Hardware programming cache */ +cpumask_var_t mktme_leadcpus; /* one cpu per pkg to program keys */ static const char * const mktme_program_err[] = { "KeyID was successfully programmed", /* 0 */ @@ -59,6 +60,37 @@ static void mktme_destroy_key(struct key *key) key_put_encrypt_ref(mktme_map_keyid_from_key(key)); } +struct mktme_hw_program_info { + struct mktme_key_program *key_program; + unsigned long status; +}; + +/* Program a KeyID on a single package. */ +static void mktme_program_package(void *hw_program_info) +{ + struct mktme_hw_program_info *info = hw_program_info; + int ret; + + ret = mktme_key_program(info->key_program); + if (ret != MKTME_PROG_SUCCESS) + WRITE_ONCE(info->status, ret); +} + +/* Program a KeyID across the entire system. */ +static int mktme_program_system(struct mktme_key_program *key_program, + cpumask_var_t mktme_cpumask) +{ + struct mktme_hw_program_info info = { + .key_program = key_program, + .status = MKTME_PROG_SUCCESS, + }; + get_online_cpus(); + on_each_cpu_mask(mktme_cpumask, mktme_program_package, &info, 1); + put_online_cpus(); + + return info.status; +} + /* Copy the payload to the HW programming structure and program this KeyID */ static int mktme_program_keyid(int keyid, struct mktme_payload *payload) { @@ -84,7 +116,7 @@ static int mktme_program_keyid(int keyid, struct mktme_payload *payload) kprog->key_field_2[i] ^= kern_entropy[i]; } } - ret = mktme_key_program(kprog); + ret = mktme_program_system(kprog, mktme_leadcpus); kmem_cache_free(mktme_prog_cache, kprog); return ret; } @@ -299,6 +331,28 @@ struct key_type key_type_mktme = { .destroy = mktme_destroy_key, }; +static int mktme_build_leadcpus_mask(void) +{ + int online_cpu, mktme_cpu; + int online_pkgid, mktme_pkgid = -1; + + if (!zalloc_cpumask_var(&mktme_leadcpus, GFP_KERNEL)) + return -ENOMEM; + + for_each_online_cpu(online_cpu) { + online_pkgid = topology_physical_package_id(online_cpu); + + for_each_cpu(mktme_cpu, mktme_leadcpus) { + mktme_pkgid = topology_physical_package_id(mktme_cpu); + if (mktme_pkgid == online_pkgid) + break; + } + if (mktme_pkgid != online_pkgid) + cpumask_set_cpu(online_cpu, mktme_leadcpus); + } + return 0; +} + /* * Allocate the global mktme_map structure based on the available keyids. * Create a cache for the hardware structure. Initialize the encrypt_count @@ -323,10 +377,15 @@ static int __init init_mktme(void) if (mktme_alloc_encrypt_array() < 0) goto free_cache; + if (mktme_build_leadcpus_mask() < 0) + goto free_array; + ret = register_key_type(&key_type_mktme); if (!ret) return ret; /* SUCCESS */ + free_cpumask_var(mktme_leadcpus); +free_array: mktme_free_encrypt_array(); free_cache: kmem_cache_destroy(mktme_prog_cache);
The kernel manages the MKTME (Multi-Key Total Memory Encryption) Keys as a system wide single pool of keys. The hardware, however, manages the keys on a per physical package basis. Each physical package maintains a Key Table that all CPU's in that package share. In order to maintain the consistent, system wide view that the kernel requires, program all physical packages during a key program request. Change-Id: I0ff46f37fde47a0305842baeb8ea600b6c568639 Signed-off-by: Alison Schofield <alison.schofield@intel.com> --- security/keys/mktme_keys.c | 61 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-)