Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734795 > unrolled thread

Re: [PATCH] zram: fix null dereference of handle

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH] zram: fix null dereference of handle Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-19 12:30 +0200
    BUG_ON when zsmalloc from IRQ (was zram: fix null dereference of  handle) Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-19 12:40 +0200
    Re: [PATCH] zram: fix null dereference of handle Minchan Kim <minchan@kernel.org> - 2017-09-20 08:00 +0200
      Re: [PATCH] zram: fix null dereference of handle Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-20 08:30 +0200

#1734795 — Re: [PATCH] zram: fix null dereference of handle

Minchan,

I just ran across it [because I had a bug to analize where this
part was involved]. I'd really prefer the kernel to BUG_ON immediately
instead of dying in agony.

can we, please, return BUG_ON() back?

there is no point in trying to save the kernel once it did that type
of violation.

---

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 7c38e850a8fc..685049a9048d 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1349,7 +1349,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
         * pools/users, we can't allow mapping in interrupt context
         * because it can corrupt another users mappings.
         */
-       WARN_ON_ONCE(in_interrupt());
+       BUG_ON(in_interrupt());
 
        /* From now on, migration cannot move the object */
        pin_tag(handle);

[toc] | [next] | [standalone]

#1734806 — BUG_ON when zsmalloc from IRQ (was zram: fix null dereference of handle)

ah... this should have had another Subject line....
sorry.

	-ss

On (09/19/17 19:21), Sergey Senozhatsky wrote:
> Minchan,
> 
> I just ran across it [because I had a bug to analize where this
> part was involved]. I'd really prefer the kernel to BUG_ON immediately
> instead of dying in agony.
> 
> can we, please, return BUG_ON() back?
> 
> there is no point in trying to save the kernel once it did that type
> of violation.
> 
> ---
> 
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 7c38e850a8fc..685049a9048d 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1349,7 +1349,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
>          * pools/users, we can't allow mapping in interrupt context
>          * because it can corrupt another users mappings.
>          */
> -       WARN_ON_ONCE(in_interrupt());
> +       BUG_ON(in_interrupt());
>  
>         /* From now on, migration cannot move the object */
>         pin_tag(handle);
> 

[toc] | [prev] | [next] | [standalone]

#1735537

On Tue, Sep 19, 2017 at 07:21:25PM +0900, Sergey Senozhatsky wrote:
> Minchan,
> 
> I just ran across it [because I had a bug to analize where this
> part was involved]. I'd really prefer the kernel to BUG_ON immediately
> instead of dying in agony.
> 
> can we, please, return BUG_ON() back?
> 
> there is no point in trying to save the kernel once it did that type
> of violation.

I agree. If it happens, it would corrupt other user's buffer which ends
up leaking some private data from others so there is pointless to keep
system alive to debug it.

Do you mind sending a formal patch?
Thanks!

> 
> ---
> 
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 7c38e850a8fc..685049a9048d 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1349,7 +1349,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
>          * pools/users, we can't allow mapping in interrupt context
>          * because it can corrupt another users mappings.
>          */
> -       WARN_ON_ONCE(in_interrupt());
> +       BUG_ON(in_interrupt());
>  
>         /* From now on, migration cannot move the object */
>         pin_tag(handle);
> 

[toc] | [prev] | [next] | [standalone]

#1735552

Hello,

On (09/20/17 14:51), Minchan Kim wrote:
> On Tue, Sep 19, 2017 at 07:21:25PM +0900, Sergey Senozhatsky wrote:
> > Minchan,
> > 
> > I just ran across it [because I had a bug to analize where this
> > part was involved]. I'd really prefer the kernel to BUG_ON immediately
> > instead of dying in agony.
> > 
> > can we, please, return BUG_ON() back?
> > 
> > there is no point in trying to save the kernel once it did that type
> > of violation.
> 
> I agree. If it happens, it would corrupt other user's buffer which ends
> up leaking some private data from others so there is pointless to keep
> system alive to debug it.

thanks!

yep, I guess we are also potentially looking at __zs_map_object()
preempting zsmalloc in normal context, which would overwrite
area->vm_addr from IRQ, map it, use it, unmap it in __zs_unmap_object().
and then resumed normal context will attempt to use unmapped area->vm_addr,
I suppose.


> Do you mind sending a formal patch?

sure, will do.

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web