| Message ID | 20211005063220.1513915-2-frank.heimes@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Problem leading IUCV service down (on s390x) (LP: 1913442) | expand |
On 05.10.21 08:32, frank.heimes@canonical.com wrote: > From: Vlastimil Babka <vbabka@suse.cz> > > BugLink: https://bugs.launchpad.net/bugs/1913442 > > Upstream commit : 49f2d2419d60a103752e5fbaf158cf8d07c0d884 > > We have seen a "usercopy: Kernel memory overwrite attempt detected to > SLUB object 'dma-kmalloc-1 k' (offset 0, size 11)!" error on s390x, as > IUCV uses kmalloc() with __GFP_DMA because of memory address > restrictions. The issue has been discussed [2] and it has been noted > that if all the kmalloc caches are marked as usercopy, there's little > reason not to mark dma-kmalloc caches too. The 'dma' part merely means > that __GFP_DMA is used to restrict memory address range. > > As Jann Horn put it [3]: > "I think dma-kmalloc slabs should be handled the same way as normal > kmalloc slabs. When a dma-kmalloc allocation is freshly created, it is > just normal kernel memory - even if it might later be used for DMA -, > and it should be perfectly fine to copy_from_user() into such > allocations at that point, and to copy_to_user() out of them at the > end. If you look at the places where such allocations are created, you > can see things like kmemdup(), memcpy() and so on - all normal > operations that shouldn't conceptually be different from usercopy in > any relevant way." > > Thus this patch marks the dma-kmalloc-* caches as usercopy. > > [1] https://bugzilla.suse.com/show_bug.cgi?id=1156053 > [2] https://lore.kernel.org/kernel-hardening/bfca96db-bbd0-d958-7732-76e36c667c68@suse.cz/ > [3] https://lore.kernel.org/kernel-hardening/CAG48ez1a4waGk9kB0WLaSbs4muSoK0AYAVk8=XYaKj4_+6e6Hg@mail.gmail.com/ > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > Acked-by: Jiri Slaby <jslaby@suse.cz> > Cc: Jann Horn <jannh@google.com> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Christopher Lameter <cl@linux.com> > Cc: Julian Wiedmann <jwi@linux.ibm.com> > Cc: Ursula Braun <ubraun@linux.ibm.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: David Windsor <dave@nullcore.net> > Cc: Pekka Enberg <penberg@kernel.org> > Cc: David Rientjes <rientjes@google.com> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Laura Abbott <labbott@redhat.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Dave Kleikamp <dave.kleikamp@oracle.com> > Cc: Jan Kara <jack@suse.cz> > Cc: Luis de Bethencourt <luisbg@kernel.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Rik van Riel <riel@surriel.com> > Cc: Matthew Garrett <mjg59@google.com> > Cc: Michal Kubecek <mkubecek@suse.cz> > Link: http://lkml.kernel.org/r/7d810f6d-8085-ea2f-7805-47ba3842dc50@suse.cz > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (backported from commit 49f2d2419d60a103752e5fbaf158cf8d07c0d884) > Signed-off-by: Frank Heimes <frank.heimes@canonical.com> > --- > mm/slab_common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 8c1ffbf7de45..76433e2187d0 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1296,7 +1296,7 @@ void __init create_kmalloc_caches(slab_flags_t flags) > > BUG_ON(!n); > kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache( > - n, size, SLAB_CACHE_DMA | flags, 0, 0); > + n, size, SLAB_CACHE_DMA | flags, 0, kmalloc_info[i].size); Although this is probably functionally the same, I'm wondering if we should keep both "size" and "usersize" parameters passed to create_kmalloc_cache() consistent. In the original patch (49f2d2419d60), the "size" is not calculated anymore using "kmalloc_size(i)", which was removed by dc0a7f7558dd (mm, slab: remove unused kmalloc_size()), but passing "kmalloc_info[i].size" instead. The same is passed as the "usersize" then. Both ways of calculating "usersize" seem equivalent (using either "kmalloc_size(i)" or "kmalloc_info[i].size"), but I didn't look very deeply to check if any patch changed any of them in the meantime. So in my opinion we should also use "size" for the backport to be on the safe side and do something like: kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache( - n, size, SLAB_CACHE_DMA | flags, 0, 0); + n, size, SLAB_CACHE_DMA | flags, 0, size); I'll not ACK nor NACK for now and wait for someone else to weight in. Kleber
Well, I'm sure this is a question for experienced kernel engineers - so I just stay tuned. But as a side note: My approach was only to get the single commit 49f2d2419d60 in as smooth as possible, means with as few changes as possible (incl. context), so that potential further cherry-picks or backports in that area are not harmed (or at least as little as possible). On Tue, Oct 5, 2021 at 4:10 PM Kleber Souza <kleber.souza@canonical.com> wrote: > On 05.10.21 08:32, frank.heimes@canonical.com wrote: > > From: Vlastimil Babka <vbabka@suse.cz> > > > > BugLink: https://bugs.launchpad.net/bugs/1913442 > > > > Upstream commit : 49f2d2419d60a103752e5fbaf158cf8d07c0d884 > > > > We have seen a "usercopy: Kernel memory overwrite attempt detected to > > SLUB object 'dma-kmalloc-1 k' (offset 0, size 11)!" error on s390x, as > > IUCV uses kmalloc() with __GFP_DMA because of memory address > > restrictions. The issue has been discussed [2] and it has been noted > > that if all the kmalloc caches are marked as usercopy, there's little > > reason not to mark dma-kmalloc caches too. The 'dma' part merely means > > that __GFP_DMA is used to restrict memory address range. > > > > As Jann Horn put it [3]: > > "I think dma-kmalloc slabs should be handled the same way as normal > > kmalloc slabs. When a dma-kmalloc allocation is freshly created, it is > > just normal kernel memory - even if it might later be used for DMA -, > > and it should be perfectly fine to copy_from_user() into such > > allocations at that point, and to copy_to_user() out of them at the > > end. If you look at the places where such allocations are created, you > > can see things like kmemdup(), memcpy() and so on - all normal > > operations that shouldn't conceptually be different from usercopy in > > any relevant way." > > > > Thus this patch marks the dma-kmalloc-* caches as usercopy. > > > > [1] https://bugzilla.suse.com/show_bug.cgi?id=1156053 > > [2] > https://lore.kernel.org/kernel-hardening/bfca96db-bbd0-d958-7732-76e36c667c68@suse.cz/ > > [3] > https://lore.kernel.org/kernel-hardening/CAG48ez1a4waGk9kB0WLaSbs4muSoK0AYAVk8=XYaKj4_+6e6Hg@mail.gmail.com/ > > > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > > Acked-by: Jiri Slaby <jslaby@suse.cz> > > Cc: Jann Horn <jannh@google.com> > > Cc: Christoph Hellwig <hch@infradead.org> > > Cc: Christopher Lameter <cl@linux.com> > > Cc: Julian Wiedmann <jwi@linux.ibm.com> > > Cc: Ursula Braun <ubraun@linux.ibm.com> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > Cc: David Windsor <dave@nullcore.net> > > Cc: Pekka Enberg <penberg@kernel.org> > > Cc: David Rientjes <rientjes@google.com> > > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > Cc: Andy Lutomirski <luto@kernel.org> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Laura Abbott <labbott@redhat.com> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Christoffer Dall <christoffer.dall@linaro.org> > > Cc: Dave Kleikamp <dave.kleikamp@oracle.com> > > Cc: Jan Kara <jack@suse.cz> > > Cc: Luis de Bethencourt <luisbg@kernel.org> > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > Cc: Rik van Riel <riel@surriel.com> > > Cc: Matthew Garrett <mjg59@google.com> > > Cc: Michal Kubecek <mkubecek@suse.cz> > > Link: > http://lkml.kernel.org/r/7d810f6d-8085-ea2f-7805-47ba3842dc50@suse.cz > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > (backported from commit 49f2d2419d60a103752e5fbaf158cf8d07c0d884) > > Signed-off-by: Frank Heimes <frank.heimes@canonical.com> > > --- > > mm/slab_common.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/mm/slab_common.c b/mm/slab_common.c > > index 8c1ffbf7de45..76433e2187d0 100644 > > --- a/mm/slab_common.c > > +++ b/mm/slab_common.c > > @@ -1296,7 +1296,7 @@ void __init create_kmalloc_caches(slab_flags_t > flags) > > > > BUG_ON(!n); > > kmalloc_caches[KMALLOC_DMA][i] = > create_kmalloc_cache( > > - n, size, SLAB_CACHE_DMA | flags, 0, 0); > > + n, size, SLAB_CACHE_DMA | flags, 0, > kmalloc_info[i].size); > > Although this is probably functionally the same, I'm wondering if we should > keep both "size" and "usersize" parameters passed to create_kmalloc_cache() > consistent. > > In the original patch (49f2d2419d60), the "size" is not calculated anymore > using > "kmalloc_size(i)", which was removed by dc0a7f7558dd (mm, slab: remove > unused > kmalloc_size()), but passing "kmalloc_info[i].size" instead. The same is > passed > as the "usersize" then. > > Both ways of calculating "usersize" seem equivalent (using either > "kmalloc_size(i)" > or "kmalloc_info[i].size"), but I didn't look very deeply to check if any > patch > changed any of them in the meantime. So in my opinion we should also use > "size" for > the backport to be on the safe side and do something like: > > kmalloc_caches[KMALLOC_DMA][i] = > create_kmalloc_cache( > - n, size, SLAB_CACHE_DMA | flags, 0, 0); > + n, size, SLAB_CACHE_DMA | flags, 0, size); > > > I'll not ACK nor NACK for now and wait for someone else to weight in. > > > Kleber >
On 05/10/2021 16:10, Kleber Souza wrote: > On 05.10.21 08:32, frank.heimes@canonical.com wrote: >> From: Vlastimil Babka <vbabka@suse.cz> >> >> BugLink: https://bugs.launchpad.net/bugs/1913442 >> >> Upstream commit : 49f2d2419d60a103752e5fbaf158cf8d07c0d884 Thanks Frank for the patch. This should be removed. It suggest it came from upstream stable but it did not. >> >> We have seen a "usercopy: Kernel memory overwrite attempt detected to >> SLUB object 'dma-kmalloc-1 k' (offset 0, size 11)!" error on s390x, as >> IUCV uses kmalloc() with __GFP_DMA because of memory address >> restrictions. The issue has been discussed [2] and it has been noted >> that if all the kmalloc caches are marked as usercopy, there's little >> reason not to mark dma-kmalloc caches too. The 'dma' part merely means >> that __GFP_DMA is used to restrict memory address range. >> >> As Jann Horn put it [3]: >> "I think dma-kmalloc slabs should be handled the same way as normal >> kmalloc slabs. When a dma-kmalloc allocation is freshly created, it is >> just normal kernel memory - even if it might later be used for DMA -, >> and it should be perfectly fine to copy_from_user() into such >> allocations at that point, and to copy_to_user() out of them at the >> end. If you look at the places where such allocations are created, you >> can see things like kmemdup(), memcpy() and so on - all normal >> operations that shouldn't conceptually be different from usercopy in >> any relevant way." >> >> Thus this patch marks the dma-kmalloc-* caches as usercopy. >> >> [1] https://bugzilla.suse.com/show_bug.cgi?id=1156053 >> [2] https://lore.kernel.org/kernel-hardening/bfca96db-bbd0-d958-7732-76e36c667c68@suse.cz/ >> [3] https://lore.kernel.org/kernel-hardening/CAG48ez1a4waGk9kB0WLaSbs4muSoK0AYAVk8=XYaKj4_+6e6Hg@mail.gmail.com/ >> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Acked-by: Jiri Slaby <jslaby@suse.cz> >> Cc: Jann Horn <jannh@google.com> >> Cc: Christoph Hellwig <hch@infradead.org> >> Cc: Christopher Lameter <cl@linux.com> >> Cc: Julian Wiedmann <jwi@linux.ibm.com> >> Cc: Ursula Braun <ubraun@linux.ibm.com> >> Cc: Alexander Viro <viro@zeniv.linux.org.uk> >> Cc: David Windsor <dave@nullcore.net> >> Cc: Pekka Enberg <penberg@kernel.org> >> Cc: David Rientjes <rientjes@google.com> >> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> >> Cc: Andy Lutomirski <luto@kernel.org> >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: Laura Abbott <labbott@redhat.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Christoffer Dall <christoffer.dall@linaro.org> >> Cc: Dave Kleikamp <dave.kleikamp@oracle.com> >> Cc: Jan Kara <jack@suse.cz> >> Cc: Luis de Bethencourt <luisbg@kernel.org> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: Rik van Riel <riel@surriel.com> >> Cc: Matthew Garrett <mjg59@google.com> >> Cc: Michal Kubecek <mkubecek@suse.cz> >> Link: http://lkml.kernel.org/r/7d810f6d-8085-ea2f-7805-47ba3842dc50@suse.cz >> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> >> (backported from commit 49f2d2419d60a103752e5fbaf158cf8d07c0d884) >> Signed-off-by: Frank Heimes <frank.heimes@canonical.com> >> --- >> mm/slab_common.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/slab_common.c b/mm/slab_common.c >> index 8c1ffbf7de45..76433e2187d0 100644 >> --- a/mm/slab_common.c >> +++ b/mm/slab_common.c >> @@ -1296,7 +1296,7 @@ void __init create_kmalloc_caches(slab_flags_t flags) >> >> BUG_ON(!n); >> kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache( >> - n, size, SLAB_CACHE_DMA | flags, 0, 0); >> + n, size, SLAB_CACHE_DMA | flags, 0, kmalloc_info[i].size); > > Although this is probably functionally the same, I'm wondering if we should > keep both "size" and "usersize" parameters passed to create_kmalloc_cache() > consistent. > > In the original patch (49f2d2419d60), the "size" is not calculated anymore using > "kmalloc_size(i)", which was removed by dc0a7f7558dd (mm, slab: remove unused > kmalloc_size()), but passing "kmalloc_info[i].size" instead. The same is passed > as the "usersize" then. > > Both ways of calculating "usersize" seem equivalent (using either "kmalloc_size(i)" > or "kmalloc_info[i].size"), but I didn't look very deeply to check if any patch > changed any of them in the meantime. So in my opinion we should also use "size" for > the backport to be on the safe side and do something like: > > kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache( > - n, size, SLAB_CACHE_DMA | flags, 0, 0); > + n, size, SLAB_CACHE_DMA | flags, 0, size); > > > I'll not ACK nor NACK for now and wait for someone else to weight in. It would be easier for future backports if we have here direct pick, so Frank's version. It would be more logical to have a backport matching v5.4 context, so what Kleber proposes. IOW, I don't see perfect solution and since this was tested: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > > > Kleber > Best regards, Krzysztof
diff --git a/mm/slab_common.c b/mm/slab_common.c index 8c1ffbf7de45..76433e2187d0 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1296,7 +1296,7 @@ void __init create_kmalloc_caches(slab_flags_t flags) BUG_ON(!n); kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache( - n, size, SLAB_CACHE_DMA | flags, 0, 0); + n, size, SLAB_CACHE_DMA | flags, 0, kmalloc_info[i].size); } } #endif