Message ID | alpine.LRH.2.02.1402201758190.28312@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Mike Snitzer |
Headers | show |
On Thu, Feb 20 2014 at 6:01pm -0500, Mikulas Patocka <mpatocka@redhat.com> wrote: > Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request. > The code assumed that the work item keeps executing on a single CPU, so it > used no synchronization when accessing this structure. > > When we disable a CPU by writing zero to > /sys/devices/system/cpu/cpu*/online, the work item could be moved to > another CPU. This causes crashes in dm-crypt because the code starts using > a wrong ablkcipher_request. > > This patch fixes this bug by removing the percpu definition. The structure > ablkcipher_request is accessed via a pointer from convert_context. > Consequently, if the work item is rescheduled to a different CPU, the > thread still uses the same ablkcipher_request. Hi Mikulas, Obviously avoiding crashes is more important than performance. But are we losing performance by switching away from using percpu? Do we care? I'd like to see the header to speak to the potential for slowdown (if there is any). -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, 20 Feb 2014, Mike Snitzer wrote: > On Thu, Feb 20 2014 at 6:01pm -0500, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request. > > The code assumed that the work item keeps executing on a single CPU, so it > > used no synchronization when accessing this structure. > > > > When we disable a CPU by writing zero to > > /sys/devices/system/cpu/cpu*/online, the work item could be moved to > > another CPU. This causes crashes in dm-crypt because the code starts using > > a wrong ablkcipher_request. > > > > This patch fixes this bug by removing the percpu definition. The structure > > ablkcipher_request is accessed via a pointer from convert_context. > > Consequently, if the work item is rescheduled to a different CPU, the > > thread still uses the same ablkcipher_request. > > Hi Mikulas, > > Obviously avoiding crashes is more important than performance. > > But are we losing performance by switching away from using percpu? Do > we care? I'd like to see the header to speak to the potential for > slowdown (if there is any). There is one more allocation per request than before. I don't know how much does it cost. We could also modify the code to use per_bio_data to save one allocation. Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Feb 20 2014 at 7:10pm -0500, Mikulas Patocka <mpatocka@redhat.com> wrote: > > > On Thu, 20 Feb 2014, Mike Snitzer wrote: > > > On Thu, Feb 20 2014 at 6:01pm -0500, > > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request. > > > The code assumed that the work item keeps executing on a single CPU, so it > > > used no synchronization when accessing this structure. > > > > > > When we disable a CPU by writing zero to > > > /sys/devices/system/cpu/cpu*/online, the work item could be moved to > > > another CPU. This causes crashes in dm-crypt because the code starts using > > > a wrong ablkcipher_request. > > > > > > This patch fixes this bug by removing the percpu definition. The structure > > > ablkcipher_request is accessed via a pointer from convert_context. > > > Consequently, if the work item is rescheduled to a different CPU, the > > > thread still uses the same ablkcipher_request. > > > > Hi Mikulas, > > > > Obviously avoiding crashes is more important than performance. > > > > But are we losing performance by switching away from using percpu? Do > > we care? I'd like to see the header to speak to the potential for > > slowdown (if there is any). > > There is one more allocation per request than before. I don't know how > much does it cost. OK, any reason you didn't fix this up by using cpu hotplug hooks like Tejun suggested? Too complicated? > We could also modify the code to use per_bio_data to save one allocation. OK, sounds like a good win. Can you write a separate followup patch that makes use of per_bio_data? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, 20 Feb 2014, Mike Snitzer wrote: > On Thu, Feb 20 2014 at 7:10pm -0500, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > On Thu, 20 Feb 2014, Mike Snitzer wrote: > > > > > On Thu, Feb 20 2014 at 6:01pm -0500, > > > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > > Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request. > > > > The code assumed that the work item keeps executing on a single CPU, so it > > > > used no synchronization when accessing this structure. > > > > > > > > When we disable a CPU by writing zero to > > > > /sys/devices/system/cpu/cpu*/online, the work item could be moved to > > > > another CPU. This causes crashes in dm-crypt because the code starts using > > > > a wrong ablkcipher_request. > > > > > > > > This patch fixes this bug by removing the percpu definition. The structure > > > > ablkcipher_request is accessed via a pointer from convert_context. > > > > Consequently, if the work item is rescheduled to a different CPU, the > > > > thread still uses the same ablkcipher_request. > > > > > > Hi Mikulas, > > > > > > Obviously avoiding crashes is more important than performance. > > > > > > But are we losing performance by switching away from using percpu? Do > > > we care? I'd like to see the header to speak to the potential for > > > slowdown (if there is any). > > > > There is one more allocation per request than before. I don't know how > > much does it cost. > > OK, any reason you didn't fix this up by using cpu hotplug hooks like > Tejun suggested? Too complicated? Yes, it would complicate the code. The patch that removes percpu pointers shortens the file, using cpu hotplug hooks would make it bigger. > > We could also modify the code to use per_bio_data to save one allocation. > > OK, sounds like a good win. Can you write a separate followup patch > that makes use of per_bio_data? I will try it. Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 02/21/2014 08:41 AM, Mikulas Patocka wrote: >>>> Hi Mikulas, >>>> >>>> Obviously avoiding crashes is more important than performance. Yes, but please keep these changes in linux-next for a while before submitting this for stable or you will fix one rare bug and slow down all existing users (with possible another fix to fix later)... I know that taking cpu offline becomes more used these days but still the performance is critical attribute for dmcrypt. (And IIRC this cpu work relocation problem is there for years.) >>>> But are we losing performance by switching away from using percpu? Do >>>> we care? I'd like to see the header to speak to the potential for >>>> slowdown (if there is any). >>> >>> There is one more allocation per request than before. I don't know how >>> much does it cost. Could you please try it on some test machine in lab? I think Red Hat has enough usable machines for such tests. >> OK, any reason you didn't fix this up by using cpu hotplug hooks like >> Tejun suggested? Too complicated? > > Yes, it would complicate the code. The patch that removes percpu pointers > shortens the file, using cpu hotplug hooks would make it bigger. Since we prefer source file size to correct solution? That said, dmcrypt becomes already too big. I already thought about separating some things (keeping the core functionality in one place), e.g. IV generators implementation is something what could be logically separated. (Is it worth to do it?) >>> We could also modify the code to use per_bio_data to save one allocation. >> >> OK, sounds like a good win. Can you write a separate followup patch >> that makes use of per_bio_data? > > I will try it. If you have some performance numbers, post it as well please. (I would not be surprised that removing percpu struct is sometimes even better for performance but this is just wild guess.) And if you have better solution to dmcrypt parallel performance, post it too but with real hw (with and without crypto acceleration) speed up numbers please. ;-) Thanks, Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Sat, Feb 22 2014 at 5:28am -0500, Milan Broz <gmazyland@gmail.com> wrote: > On 02/21/2014 08:41 AM, Mikulas Patocka wrote: > >>>> Hi Mikulas, > >>>> > >>>> Obviously avoiding crashes is more important than performance. > > Yes, but please keep these changes in linux-next for a while > before submitting this for stable or you will fix one rare bug and > slow down all existing users (with possible another fix to fix later)... > > I know that taking cpu offline becomes more used these days > but still the performance is critical attribute for dmcrypt. > > (And IIRC this cpu work relocation problem is there for years.) Unfortunately, we cannot leave the code in its current state. Sure it has been this way for years but taking cpus offline is _much_ more common. As I said here: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac This change may undermine performance improvements intended by commit c0297721 ("dm crypt: scale to multiple cpus"). But correctness is more important than performance. The use of per-cpu structures may be revisited later (by using cpu hot-plug notifiers to make them safe). > >>>> But are we losing performance by switching away from using percpu? Do > >>>> we care? I'd like to see the header to speak to the potential for > >>>> slowdown (if there is any). > >>> > >>> There is one more allocation per request than before. I don't know how > >>> much does it cost. > > Could you please try it on some test machine in lab? I think Red Hat has > enough usable machines for such tests. > > >> OK, any reason you didn't fix this up by using cpu hotplug hooks like > >> Tejun suggested? Too complicated? > > > > Yes, it would complicate the code. The patch that removes percpu pointers > > shortens the file, using cpu hot-plug hooks would make it bigger. > > Since we prefer source file size to correct solution? Right, that was my thought too. Definitely not a good enough justification. But a proper fix with cpu hot-plug notifier is more involved for sure. It will take time. Do you have time to look at fixing this issue? > That said, dmcrypt becomes already too big. I already thought about separating > some things (keeping the core functionality in one place), e.g. IV generators > implementation is something what could be logically separated. > (Is it worth to do it?) > > >>> We could also modify the code to use per_bio_data to save one allocation. > >> > >> OK, sounds like a good win. Can you write a separate followup patch > >> that makes use of per_bio_data? > > > > I will try it. > > If you have some performance numbers, post it as well please. > (I would not be surprised that removing percpu struct is sometimes even better > for performance but this is just wild guess.) > > And if you have better solution to dmcrypt parallel performance, post it too > but with real hw (with and without crypto acceleration) speed up numbers please. ;-) Mikulas does have an alternative approach that should be revisited ASAP given that it also fixes this cpu hotplug issue: http://people.redhat.com/mpatocka/patches/kernel/dm-crypt-paralelizace/current/series.html context/timeline: http://www.redhat.com/archives/dm-devel/2011-October/msg00127.html http://www.redhat.com/archives/dm-devel/2012-March/msg00181.html http://www.redhat.com/archives/dm-devel/2013-March/msg00103.html Christoph pointed out there is precendence for sorting: http://www.redhat.com/archives/dm-devel/2013-March/msg00104.html Even though you actively resisted it: http://www.redhat.com/archives/dm-devel/2013-March/msg00107.html Progress on improving dm-crypt is long overdue, I'd like to see Mikulas rebase/repost/retest his dm-crypt parallelization patchset for 3.15 or 3.16. Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 02/22/2014 03:25 PM, Mike Snitzer wrote: > On Sat, Feb 22 2014 at 5:28am -0500, > Milan Broz <gmazyland@gmail.com> wrote: > >> On 02/21/2014 08:41 AM, Mikulas Patocka wrote: >>>>>> Hi Mikulas, >>>>>> >>>>>> Obviously avoiding crashes is more important than performance. >> >> Yes, but please keep these changes in linux-next for a while >> before submitting this for stable or you will fix one rare bug and >> slow down all existing users (with possible another fix to fix later)... >> >> I know that taking cpu offline becomes more used these days >> but still the performance is critical attribute for dmcrypt. >> >> (And IIRC this cpu work relocation problem is there for years.) > > Unfortunately, we cannot leave the code in its current state. Sure it > has been this way for years but taking cpus offline is _much_ more > common. > > As I said here: > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac > > This change may undermine performance improvements intended by commit > c0297721 ("dm crypt: scale to multiple cpus"). But correctness is more > important than performance. The use of per-cpu structures may be > revisited later (by using cpu hot-plug notifiers to make them safe). Correctness? Yes. I think Tejun said how to fix it correctly. I have nothing against removal of percpu structure - but please assess what it breaks, how it will slow down dmcrypt and mention it in patch header if you want quick fix. I couldn't resist but "may undermine" in patch header sounds like "we have no idea what it breaks but it no longer crashes". Removal of headache by removing head is always 100% solution to the particular problem but it has usually also some side effects :-) ... > Mikulas does have an alternative approach that should be revisited ASAP > given that it also fixes this cpu hotplug issue: > http://people.redhat.com/mpatocka/patches/kernel/dm-crypt-paralelizace/current/series.html Yes, and I spent many hours testing it and while it helped somewhere it was not perfect solution in every situation. And I wish it works! Unfortunately, the tests output was not convincing. But it was described in thread you referenced. And if he just repost the same patchset few month later without any more data and no changes, why should I change my opinion? I can just shut up of course and just maintain userspace and watch people complaining. > context/timeline: > http://www.redhat.com/archives/dm-devel/2011-October/msg00127.html > http://www.redhat.com/archives/dm-devel/2012-March/msg00181.html > http://www.redhat.com/archives/dm-devel/2013-March/msg00103.html > > Christoph pointed out there is precendence for sorting: > http://www.redhat.com/archives/dm-devel/2013-March/msg00104.html My opinion is that fixing io scheduler work (sorting) in dmcrypt is just ugly hack and could lead to more problems in future because it can increase latency and it anticipates some IO access patterns. Dm-crypt should be (ideally) completely transparent and "invisible" to IO processing... I think there is a report of very slow operation of database (IIRC it was MySQL) running over dmcrypt device. This would be probably nice test case for these changes. But I am not nacking it, if you want it, do it, you are maintainer. > Even though you actively resisted it: > http://www.redhat.com/archives/dm-devel/2013-March/msg00107.html I am repeating still repeating the same: Do we have performance numbers proving it is better in real world scenarios? Nobody will use ramdisk for dmcrypt. These are the most used scenarios for dmcrypt I know : - Notebook with single SSD / rotational disk and multicore (2 or 4) CPU with AES-NI acceleration, dmcrypt over plain partition or LVM (typical encrypted distro installation) - dmcrypt over MD RAID1/5/6 with several multicore CPUs (think encrypted array for backups on server) [There is also variant with several dmcrypt devices with MD RAID over it - which is currently performance disaster - but I am not sure this is good idea. It was suggested as workaround before percpu changes were implemented.] - several dmcrypt devices in system with many cores over LVM (encrypted storage for VMs. It would be nice to have fixed CPU limit per dmcrypt device - you do not want to one VM eat all processing power. I think we discussed with Mikulas that dmcrypt should have some tweaking parameters to limit used CPUs per device. But it was long time ago...) It would be also interesting how it behaves when stacking dmcrypt devices on top of each other (this is used in TrueCrypt compatibility mode in crypsetup, but also in TrueCrypt itself). > Progress on improving dm-crypt is long overdue, I'd like to see Mikulas > rebase/repost/retest his dm-crypt parallelization patchset for 3.15 or > 3.16. I agree and I would put accent on _improving_ in this sentence ;-) Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Sat, Feb 22 2014 at 12:34pm -0500, Milan Broz <gmazyland@gmail.com> wrote: > On 02/22/2014 03:25 PM, Mike Snitzer wrote: > > On Sat, Feb 22 2014 at 5:28am -0500, > > Milan Broz <gmazyland@gmail.com> wrote: > > > >> On 02/21/2014 08:41 AM, Mikulas Patocka wrote: > >>>>>> Hi Mikulas, > >>>>>> > >>>>>> Obviously avoiding crashes is more important than performance. > >> > >> Yes, but please keep these changes in linux-next for a while > >> before submitting this for stable or you will fix one rare bug and > >> slow down all existing users (with possible another fix to fix later)... > >> > >> I know that taking cpu offline becomes more used these days > >> but still the performance is critical attribute for dmcrypt. > >> > >> (And IIRC this cpu work relocation problem is there for years.) > > > > Unfortunately, we cannot leave the code in its current state. Sure it > > has been this way for years but taking cpus offline is _much_ more > > common. > > > > As I said here: > > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac > > > > This change may undermine performance improvements intended by commit > > c0297721 ("dm crypt: scale to multiple cpus"). But correctness is more > > important than performance. The use of per-cpu structures may be > > revisited later (by using cpu hot-plug notifiers to make them safe). > > Correctness? Yes. I think Tejun said how to fix it correctly. OK, patches welcome. > I have nothing against removal of percpu structure - but please assess > what it breaks, how it will slow down dmcrypt and mention it in patch > header if you want quick fix. > > I couldn't resist but "may undermine" in patch header sounds like > "we have no idea what it breaks but it no longer crashes". > > Removal of headache by removing head is always 100% solution to > the particular problem but it has usually also some side effects :-) Show me that the percpu performance is worth the risk of crashing the kernel. I'd love to see you prove that. If Mikulas' changes were embraced 2.5 years ago we'd be in a much better place than we are now. I'm making the decision to affect change. You may not agree with it but I'm confident dm-crypt will be better for it. > > Even though you actively resisted it: > > http://www.redhat.com/archives/dm-devel/2013-March/msg00107.html > > I am repeating still repeating the same: Do we have performance > numbers proving it is better in real world scenarios? > Nobody will use ramdisk for dmcrypt. > > These are the most used scenarios for dmcrypt I know : > > - Notebook with single SSD / rotational disk and multicore (2 or 4) CPU > with AES-NI acceleration, dmcrypt over plain partition or LVM > (typical encrypted distro installation) > > - dmcrypt over MD RAID1/5/6 with several multicore CPUs > (think encrypted array for backups on server) > [There is also variant with several dmcrypt devices with MD RAID > over it - which is currently performance disaster - but I am > not sure this is good idea. It was suggested as workaround > before percpu changes were implemented.] > > - several dmcrypt devices in system with many cores over LVM > (encrypted storage for VMs. It would be nice to have > fixed CPU limit per dmcrypt device - you do not want to one VM > eat all processing power. I think we discussed with Mikulas > that dmcrypt should have some tweaking parameters to limit used > CPUs per device. But it was long time ago...) > > It would be also interesting how it behaves when stacking dmcrypt > devices on top of each other (this is used in TrueCrypt compatibility > mode in crypsetup, but also in TrueCrypt itself). Thanks for the configuration scenarios. I think I've asked this of you before but: Do you have any automated testing? Something that we hand a blockdevice to and it produces a result that you find meaningful to use as a point of comparison. > > Progress on improving dm-crypt is long overdue, I'd like to see Mikulas > > rebase/repost/retest his dm-crypt parallelization patchset for 3.15 or > > 3.16. > > I agree and I would put accent on _improving_ in this sentence ;-) Uh huh... -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 02/22/2014 07:29 PM, Mike Snitzer wrote: > Thanks for the configuration scenarios. I think I've asked this of you > before but: Do you have any automated testing? Something that we hand a > blockdevice to and it produces a result that you find meaningful to use > as a point of comparison. Ask Ondra, he should have all code what I used (I think he modified it later). (It was some fio scripts, stacked dmcrypt test, some basic dt and fsx tests and some simple seek test.) I run it against several versions (no patch, without sort part, etc) and tried to compare times and throughput. It is of course possible I did some stupid mistake. On top of it, I do not have anything better. And I do not have any suitable hw for testing now, so I can just say my opinion. I just hope you have more facts than you mentioned here to support your decision to merge these changes. Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Sat, 22 Feb 2014, Milan Broz wrote: > On 02/22/2014 03:25 PM, Mike Snitzer wrote: > > On Sat, Feb 22 2014 at 5:28am -0500, > > Milan Broz <gmazyland@gmail.com> wrote: > > > >> On 02/21/2014 08:41 AM, Mikulas Patocka wrote: > >>>>>> Hi Mikulas, > >>>>>> > >>>>>> Obviously avoiding crashes is more important than performance. > >> > >> Yes, but please keep these changes in linux-next for a while > >> before submitting this for stable or you will fix one rare bug and > >> slow down all existing users (with possible another fix to fix later)... > >> > >> I know that taking cpu offline becomes more used these days > >> but still the performance is critical attribute for dmcrypt. > >> > >> (And IIRC this cpu work relocation problem is there for years.) > > > > Unfortunately, we cannot leave the code in its current state. Sure it > > has been this way for years but taking cpus offline is _much_ more > > common. > > > > As I said here: > > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac > > > > This change may undermine performance improvements intended by commit > > c0297721 ("dm crypt: scale to multiple cpus"). But correctness is more > > important than performance. The use of per-cpu structures may be > > revisited later (by using cpu hot-plug notifiers to make them safe). > > Correctness? Yes. I think Tejun said how to fix it correctly. > > I have nothing against removal of percpu structure - but please assess > what it breaks, how it will slow down dmcrypt and mention it in patch > header if you want quick fix. With the patch applied, dm-crypt uses one more mempool_alloc and mempool_free call per requests. With 512-byte requests and ramdisk as a backing device, dm-crypt can process up to 25000 requests per second. On the same machine, you can do 19000000 mempool_alloc+mempool_free calls per second. So, the slowdown caused by mempool_alloc+mempool_free is negligable - in theory, every request is slowed down by 1/760. I and Mike measured it and we didn't see any slowdown caused by the patch. > I couldn't resist but "may undermine" in patch header sounds like > "we have no idea what it breaks but it no longer crashes". > > Removal of headache by removing head is always 100% solution to > the particular problem but it has usually also some side effects :-) > > ... > > > Mikulas does have an alternative approach that should be revisited ASAP > > given that it also fixes this cpu hotplug issue: > > http://people.redhat.com/mpatocka/patches/kernel/dm-crypt-paralelizace/current/series.html > > Yes, and I spent many hours testing it and while it helped somewhere > it was not perfect solution in every situation. And I wish it works! > Unfortunately, the tests output was not convincing. > But it was described in thread you referenced. The parallelization patches help in sequential reading. In other workloads - such as concurrent access by multiple threads or bulk data writing - encryption is already parallelized by the current dm-crypt implementation, so there's nothing to gain. The new implementation can't improve parallelization, if it is already parallelized with the current imeplemtation. > And if he just repost the same patchset few month later without any > more data and no changes, why should I change my opinion? > I can just shut up of course and just maintain userspace and watch > people complaining. > > > context/timeline: > > http://www.redhat.com/archives/dm-devel/2011-October/msg00127.html > > http://www.redhat.com/archives/dm-devel/2012-March/msg00181.html > > http://www.redhat.com/archives/dm-devel/2013-March/msg00103.html > > > > Christoph pointed out there is precendence for sorting: > > http://www.redhat.com/archives/dm-devel/2013-March/msg00104.html > > My opinion is that fixing io scheduler work (sorting) in dmcrypt is > just ugly hack and could lead to more problems in future because > it can increase latency and it anticipates some IO access patterns. > Dm-crypt should be (ideally) completely transparent and "invisible" > to IO processing... The problem is that the cfq scheduler doesn't merge adjacent requests submitted by different thread. You don't have to sort it in dm-crypt, but you must submit all the requests from a single thread. Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Index: linux-3.14-rc1/drivers/md/dm-crypt.c =================================================================== --- linux-3.14-rc1.orig/drivers/md/dm-crypt.c 2014-02-03 19:18:23.000000000 +0100 +++ linux-3.14-rc1/drivers/md/dm-crypt.c 2014-02-03 19:21:35.000000000 +0100 @@ -19,7 +19,6 @@ #include <linux/crypto.h> #include <linux/workqueue.h> #include <linux/backing-dev.h> -#include <linux/percpu.h> #include <linux/atomic.h> #include <linux/scatterlist.h> #include <asm/page.h> @@ -43,6 +42,7 @@ struct convert_context { struct bvec_iter iter_out; sector_t cc_sector; atomic_t cc_pending; + struct ablkcipher_request *req; }; /* @@ -111,15 +111,7 @@ struct iv_tcw_private { enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID }; /* - * Duplicated per-CPU state for cipher. - */ -struct crypt_cpu { - struct ablkcipher_request *req; -}; - -/* - * The fields in here must be read only after initialization, - * changing state should be in crypt_cpu. + * The fields in here must be read only after initialization. */ struct crypt_config { struct dm_dev *dev; @@ -150,12 +142,6 @@ struct crypt_config { sector_t iv_offset; unsigned int iv_size; - /* - * Duplicated per cpu state. Access through - * per_cpu_ptr() only. - */ - struct crypt_cpu __percpu *cpu; - /* ESSIV: struct crypto_cipher *essiv_tfm */ void *iv_private; struct crypto_ablkcipher **tfms; @@ -192,11 +178,6 @@ static void clone_init(struct dm_crypt_i static void kcryptd_queue_crypt(struct dm_crypt_io *io); static u8 *iv_of_dmreq(struct crypt_config *cc, struct dm_crypt_request *dmreq); -static struct crypt_cpu *this_crypt_config(struct crypt_config *cc) -{ - return this_cpu_ptr(cc->cpu); -} - /* * Use this to access cipher attributes that are the same for each CPU. */ @@ -903,16 +884,15 @@ static void kcryptd_async_done(struct cr static void crypt_alloc_req(struct crypt_config *cc, struct convert_context *ctx) { - struct crypt_cpu *this_cc = this_crypt_config(cc); unsigned key_index = ctx->cc_sector & (cc->tfms_count - 1); - if (!this_cc->req) - this_cc->req = mempool_alloc(cc->req_pool, GFP_NOIO); + if (!ctx->req) + ctx->req = mempool_alloc(cc->req_pool, GFP_NOIO); - ablkcipher_request_set_tfm(this_cc->req, cc->tfms[key_index]); - ablkcipher_request_set_callback(this_cc->req, + ablkcipher_request_set_tfm(ctx->req, cc->tfms[key_index]); + ablkcipher_request_set_callback(ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP, - kcryptd_async_done, dmreq_of_req(cc, this_cc->req)); + kcryptd_async_done, dmreq_of_req(cc, ctx->req)); } /* @@ -921,7 +901,6 @@ static void crypt_alloc_req(struct crypt static int crypt_convert(struct crypt_config *cc, struct convert_context *ctx) { - struct crypt_cpu *this_cc = this_crypt_config(cc); int r; atomic_set(&ctx->cc_pending, 1); @@ -932,7 +911,7 @@ static int crypt_convert(struct crypt_co atomic_inc(&ctx->cc_pending); - r = crypt_convert_block(cc, ctx, this_cc->req); + r = crypt_convert_block(cc, ctx, ctx->req); switch (r) { /* async */ @@ -941,7 +920,7 @@ static int crypt_convert(struct crypt_co reinit_completion(&ctx->restart); /* fall through*/ case -EINPROGRESS: - this_cc->req = NULL; + ctx->req = NULL; ctx->cc_sector++; continue; @@ -1040,6 +1019,7 @@ static struct dm_crypt_io *crypt_io_allo io->sector = sector; io->error = 0; io->base_io = NULL; + io->ctx.req = NULL; atomic_set(&io->io_pending, 0); return io; @@ -1065,6 +1045,8 @@ static void crypt_dec_pending(struct dm_ if (!atomic_dec_and_test(&io->io_pending)) return; + if (io->ctx.req) + mempool_free(io->ctx.req, cc->req_pool); mempool_free(io, cc->io_pool); if (likely(!base_io)) @@ -1492,8 +1474,6 @@ static int crypt_wipe_key(struct crypt_c static void crypt_dtr(struct dm_target *ti) { struct crypt_config *cc = ti->private; - struct crypt_cpu *cpu_cc; - int cpu; ti->private = NULL; @@ -1505,13 +1485,6 @@ static void crypt_dtr(struct dm_target * if (cc->crypt_queue) destroy_workqueue(cc->crypt_queue); - if (cc->cpu) - for_each_possible_cpu(cpu) { - cpu_cc = per_cpu_ptr(cc->cpu, cpu); - if (cpu_cc->req) - mempool_free(cpu_cc->req, cc->req_pool); - } - crypt_free_tfms(cc); if (cc->bs) @@ -1530,9 +1503,6 @@ static void crypt_dtr(struct dm_target * if (cc->dev) dm_put_device(ti, cc->dev); - if (cc->cpu) - free_percpu(cc->cpu); - kzfree(cc->cipher); kzfree(cc->cipher_string); @@ -1588,13 +1558,6 @@ static int crypt_ctr_cipher(struct dm_ta if (tmp) DMWARN("Ignoring unexpected additional cipher options"); - cc->cpu = __alloc_percpu(sizeof(*(cc->cpu)), - __alignof__(struct crypt_cpu)); - if (!cc->cpu) { - ti->error = "Cannot allocate per cpu state"; - goto bad_mem; - } - /* * For compatibility with the original dm-crypt mapping format, if * only the cipher name is supplied, use cbc-plain.
Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request. The code assumed that the work item keeps executing on a single CPU, so it used no synchronization when accessing this structure. When we disable a CPU by writing zero to /sys/devices/system/cpu/cpu*/online, the work item could be moved to another CPU. This causes crashes in dm-crypt because the code starts using a wrong ablkcipher_request. This patch fixes this bug by removing the percpu definition. The structure ablkcipher_request is accessed via a pointer from convert_context. Consequently, if the work item is rescheduled to a different CPU, the thread still uses the same ablkcipher_request. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@vger.kernel.org --- drivers/md/dm-crypt.c | 61 +++++++++----------------------------------------- 1 file changed, 12 insertions(+), 49 deletions(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel