From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751348AbdISG72 (ORCPT <rfc822;w@1wt.eu>);
        Tue, 19 Sep 2017 02:59:28 -0400
Received: from LGEAMRELO11.lge.com ([156.147.23.51]:52765 "EHLO
        lgeamrelo11.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750919AbdISG71 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 19 Sep 2017 02:59:27 -0400
X-Original-SENDERIP: 156.147.1.127
X-Original-MAILFROM: minchan@kernel.org
X-Original-SENDERIP: 10.177.220.163
X-Original-MAILFROM: minchan@kernel.org
Date: Tue, 19 Sep 2017 15:59:24 +0900
From: Minchan Kim <minchan@kernel.org>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, kernel-team <kernel-team@lge.com>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [PATCH] zram: fix null dereference of handle
Message-ID: <20170919065924.GA29971@bbox>
References: <1505788488-26723-1-git-send-email-minchan@kernel.org>
 <20170919053934.GC16991@jagdpanzerIV.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170919053934.GC16991@jagdpanzerIV.localdomain>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Sergey,

On Tue, Sep 19, 2017 at 02:39:35PM +0900, Sergey Senozhatsky wrote:
> On (09/19/17 11:34), Minchan Kim wrote:
> [..]
> >  static void zram_meta_free(struct zram *zram, u64 disksize)
> >  {
> >  	size_t num_pages = disksize >> PAGE_SHIFT;
> > @@ -876,11 +855,18 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
> >  		zram_slot_unlock(zram, index);
> >  	}
> >  
> > -	if (zram_same_page_read(zram, index, page, 0, PAGE_SIZE))
> > -		return 0;
> > -
> >  	zram_slot_lock(zram, index);
> >  	handle = zram_get_handle(zram, index);
> > +	if (unlikely(!handle || zram_test_flag(zram, index, ZRAM_SAME))) {
> > +		void *mem;
> 
> 
> is this branch really unlikely()? ZRAM_SAME ratio really depends,
> on some setups it can be quite likely, I suspect.

Yub. Let's drop it.

> 
> 
> another question, "!handle  ==  value & ZRAM_SAME"? if so, then why not
> just check for `flags & ZRAM_SAME'? if not then:
> 
> -  for `value & ZRAM_SAME' you fill the page with zram_get_element(zram, index)
>    and return 0. ok.
> 
> -  for !handle.... you also fill the page with zram_get_element(zram, index)
>    and return 0. is this ok? shouldn't !handle return error in this case?

We discussed it before that we shouldn't return error.
Userspace can ask reading unallocated buffer freely.

And in this case, it fills the buffer zero because handle and element is unified.
However, if your concern is readability, I will make it more explict.

> 
> 
> I really suspect that there are some paths that can lead to !handle
> entry, that will not be ZRAM_SAME. e.g. error return from compression
> path.

Could you be more specific?

Thanks for the review, Sergey!