| Message ID | 20220712133946.307181-17-42.hyeyoo@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | common kmalloc v3 | expand |
On Tue, 12 Jul 2022, Hyeonggon Yoo wrote: > __ksize() returns size of objects allocated from slab allocator. > When invalid object is passed to __ksize(), returning zero > prevents further memory corruption and makes caller be able to > check if there is an error. > > If address of large object is not beginning of folio or size of > the folio is too small, it must be invalid. Return zero in such cases. Why return 0 if there is an error and why bother the callers with these checks. BUG()? > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 1f8db7959366..0d6cbe9d7ad0 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1013,8 +1013,12 @@ size_t __ksize(const void *object) > > folio = virt_to_folio(object); > > - if (unlikely(!folio_test_slab(folio))) > + if (unlikely(!folio_test_slab(folio))) { > + if (WARN_ON(object != folio_address(folio) || > + folio_size(folio) <= KMALLOC_MAX_CACHE_SIZE)) Hmmm... This may change things a bit. Before this patch it was possible to determine the storage size of order-0 pages using ksize(). Now this returns 0? I guess this is an error since the order-0 page cannot come from slab allocations.
On Tue, Jul 12, 2022 at 05:13:44PM +0200, Christoph Lameter wrote: > On Tue, 12 Jul 2022, Hyeonggon Yoo wrote: > > > __ksize() returns size of objects allocated from slab allocator. > > When invalid object is passed to __ksize(), returning zero > > prevents further memory corruption and makes caller be able to > > check if there is an error. > > > > If address of large object is not beginning of folio or size of > > the folio is too small, it must be invalid. Return zero in such cases. > > Why return 0 if there is an error and why bother the callers with these > checks. BUG()? I thought BUG should be used when there is no other solution. > > diff --git a/mm/slab_common.c b/mm/slab_common.c > > index 1f8db7959366..0d6cbe9d7ad0 100644 > > --- a/mm/slab_common.c > > +++ b/mm/slab_common.c > > @@ -1013,8 +1013,12 @@ size_t __ksize(const void *object) > > > > folio = virt_to_folio(object); > > > > - if (unlikely(!folio_test_slab(folio))) > > + if (unlikely(!folio_test_slab(folio))) { > > + if (WARN_ON(object != folio_address(folio) || > > + folio_size(folio) <= KMALLOC_MAX_CACHE_SIZE)) > > Hmmm... This may change things a bit. Before this patch it was possible to > determine the storage size of order-0 pages using ksize(). Now this > returns 0? > > I guess this is an error since the order-0 page cannot come from slab > allocations. comment in ksize() says: "The caller must guarantee that objp points to a valid object previously allocated with either kmalloc() or kmem_cache_alloc()." It should not be used on order-0 page that is not allocated from slab. No?
On Wed, 13 Jul 2022, Hyeonggon Yoo wrote: > > Why return 0 if there is an error and why bother the callers with these > > checks. BUG()? > > I thought BUG should be used when there is no other solution. Spurios returns of 0 that the caller has to check for is a solution? > > I guess this is an error since the order-0 page cannot come from slab > > allocations. > > comment in ksize() says: > "The caller must guarantee that objp points to a valid object > previously allocated with either kmalloc() or kmem_cache_alloc()." > > It should not be used on order-0 page that is not allocated from slab. No? I guess we would need to check. Code could exist that does this. Getting a 0 size would be surprising too here. BUG()? Or WARN() and return PAGE_SIZE.
On Wed, 13 Jul 2022 at 12:07, Christoph Lameter <cl@gentwo.de> wrote: > > On Wed, 13 Jul 2022, Hyeonggon Yoo wrote: > > > > Why return 0 if there is an error and why bother the callers with these > > > checks. BUG()? > > > > I thought BUG should be used when there is no other solution. > > Spurios returns of 0 that the caller has to check for is a solution? It's never really been about the caller checking for 0, see below. > > > I guess this is an error since the order-0 page cannot come from slab > > > allocations. > > > > comment in ksize() says: > > "The caller must guarantee that objp points to a valid object > > previously allocated with either kmalloc() or kmem_cache_alloc()." > > > > It should not be used on order-0 page that is not allocated from slab. No? > > I guess we would need to check. Code could exist that does this. > > Getting a 0 size would be surprising too here. BUG()? Or WARN() and return > PAGE_SIZE. We shouldn't crash, so it should be WARN(), but also returning PAGE_SIZE is bad. The intuition behind returning 0 is to try and make the buggy code cause less harm to the rest of the kernel. From [1]: > Similarly, if you are able to tell if the passed pointer is not a > valid object some other way, you can do something better - namely, > return 0. The intuition here is that the caller has a pointer to an > invalid object, and wants to use ksize() to determine its size, and > most likely access all those bytes. Arguably, at that point the kernel > is already in a degrading state. But we can try to not let things get > worse by having ksize() return 0, in the hopes that it will stop > corrupting more memory. It won't work in all cases, but should avoid > things like "s = ksize(obj); touch_all_bytes(obj, s)" where the size > bounds the memory accessed corrupting random memory. [1] https://lore.kernel.org/all/CANpmjNNYt9AG8RrGF0pq2dPbFc=vw2kaOnL2k5+8kfJeEMGuwg@mail.gmail.com/ Thanks, -- Marco
On Wed, 13 Jul 2022, Marco Elver wrote: > We shouldn't crash, so it should be WARN(), but also returning > PAGE_SIZE is bad. The intuition behind returning 0 is to try and make > the buggy code cause less harm to the rest of the kernel. > > >From [1]: > > > Similarly, if you are able to tell if the passed pointer is not a > > valid object some other way, you can do something better - namely, > > return 0. The intuition here is that the caller has a pointer to an > > invalid object, and wants to use ksize() to determine its size, and > > most likely access all those bytes. Arguably, at that point the kernel > > is already in a degrading state. But we can try to not let things get > > worse by having ksize() return 0, in the hopes that it will stop > > corrupting more memory. It won't work in all cases, but should avoid > > things like "s = ksize(obj); touch_all_bytes(obj, s)" where the size > > bounds the memory accessed corrupting random memory. "in the hopes that it will stop corrupting memory"!!!??? Do a BUG() then and definitely stop all chances of memory corruption.
On Thu, 14 Jul 2022 at 11:16, Christoph Lameter <cl@gentwo.de> wrote: > > On Wed, 13 Jul 2022, Marco Elver wrote: > > > We shouldn't crash, so it should be WARN(), but also returning > > PAGE_SIZE is bad. The intuition behind returning 0 is to try and make > > the buggy code cause less harm to the rest of the kernel. > > > > >From [1]: > > > > > Similarly, if you are able to tell if the passed pointer is not a > > > valid object some other way, you can do something better - namely, > > > return 0. The intuition here is that the caller has a pointer to an > > > invalid object, and wants to use ksize() to determine its size, and > > > most likely access all those bytes. Arguably, at that point the kernel > > > is already in a degrading state. But we can try to not let things get > > > worse by having ksize() return 0, in the hopes that it will stop > > > corrupting more memory. It won't work in all cases, but should avoid > > > things like "s = ksize(obj); touch_all_bytes(obj, s)" where the size > > > bounds the memory accessed corrupting random memory. > > "in the hopes that it will stop corrupting memory"!!!??? > > Do a BUG() then and definitely stop all chances of memory corruption. Fair enough. Well, I'd also prefer to just kill the kernel. But some people don't like that and want the option to continue running. So a WARN() gives that option, and just have to boot the kernel with panic_on_warn to kill it. There are other warnings in the kernel where we'd better kill the kernel as the chances of corrupting memory are pretty damn high if we hit them. And I still don't quite see why the case here is any more or less special. If the situation here is exceedingly rare, let's try BUG() and see what breaks?
On Thu, Jul 14, 2022 at 12:30:09PM +0200, Marco Elver wrote: > On Thu, 14 Jul 2022 at 11:16, Christoph Lameter <cl@gentwo.de> wrote: > > > > On Wed, 13 Jul 2022, Marco Elver wrote: > > > > > We shouldn't crash, so it should be WARN(), but also returning > > > PAGE_SIZE is bad. The intuition behind returning 0 is to try and make > > > the buggy code cause less harm to the rest of the kernel. > > > > > > >From [1]: > > > > > > > Similarly, if you are able to tell if the passed pointer is not a > > > > valid object some other way, you can do something better - namely, > > > > return 0. The intuition here is that the caller has a pointer to an > > > > invalid object, and wants to use ksize() to determine its size, and > > > > most likely access all those bytes. Arguably, at that point the kernel > > > > is already in a degrading state. But we can try to not let things get > > > > worse by having ksize() return 0, in the hopes that it will stop > > > > corrupting more memory. It won't work in all cases, but should avoid > > > > things like "s = ksize(obj); touch_all_bytes(obj, s)" where the size > > > > bounds the memory accessed corrupting random memory. > > > > "in the hopes that it will stop corrupting memory"!!!??? > > > > Do a BUG() then and definitely stop all chances of memory corruption. > > Fair enough. > > Well, I'd also prefer to just kill the kernel. But some people don't > like that and want the option to continue running. So a WARN() gives > that option, and just have to boot the kernel with panic_on_warn to > kill it. There are other warnings in the kernel where we'd better kill > the kernel as the chances of corrupting memory are pretty damn high if > we hit them. And I still don't quite see why the case here is any more > or less special. > > If the situation here is exceedingly rare, let's try BUG() and see what breaks? Let's try BUG() for both conditions and replace it with WARN() later if kernel hit those often. I'll update this patch in next version. And I have no strong opinion on returning 0, but if kernel hits it a lot, I think returning 0 would be more safe as you said. Thanks, Hyeonggon
On 7/12/22 15:39, Hyeonggon Yoo wrote: > __ksize() returns size of objects allocated from slab allocator. > When invalid object is passed to __ksize(), returning zero > prevents further memory corruption and makes caller be able to > check if there is an error. > > If address of large object is not beginning of folio or size of > the folio is too small, it must be invalid. Return zero in such cases. > > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> If you want to change it to BUG() I won't object, no strong opinion. > --- > mm/slab_common.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 1f8db7959366..0d6cbe9d7ad0 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1013,8 +1013,12 @@ size_t __ksize(const void *object) > > folio = virt_to_folio(object); > > - if (unlikely(!folio_test_slab(folio))) > + if (unlikely(!folio_test_slab(folio))) { > + if (WARN_ON(object != folio_address(folio) || > + folio_size(folio) <= KMALLOC_MAX_CACHE_SIZE)) > + return 0; > return folio_size(folio); > + } > > return slab_ksize(folio_slab(folio)->slab_cache); > }
diff --git a/mm/slab_common.c b/mm/slab_common.c index 1f8db7959366..0d6cbe9d7ad0 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1013,8 +1013,12 @@ size_t __ksize(const void *object) folio = virt_to_folio(object); - if (unlikely(!folio_test_slab(folio))) + if (unlikely(!folio_test_slab(folio))) { + if (WARN_ON(object != folio_address(folio) || + folio_size(folio) <= KMALLOC_MAX_CACHE_SIZE)) + return 0; return folio_size(folio); + } return slab_ksize(folio_slab(folio)->slab_cache); }
__ksize() returns size of objects allocated from slab allocator. When invalid object is passed to __ksize(), returning zero prevents further memory corruption and makes caller be able to check if there is an error. If address of large object is not beginning of folio or size of the folio is too small, it must be invalid. Return zero in such cases. Suggested-by: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- mm/slab_common.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)