Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752184AbcC2Iq0 (ORCPT <rfc822;w@1wt.eu>);
	Tue, 29 Mar 2016 04:46:26 -0400
Received: from outbound-smtp12.blacknight.com ([46.22.139.17]:48309 "EHLO
	outbound-smtp12.blacknight.com" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S1751569AbcC2IqY (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 29 Mar 2016 04:46:24 -0400
Date: Tue, 29 Mar 2016 09:46:19 +0100
From: Mel Gorman <mgorman@techsingularity.net>
To: Nicolai Stange <nicstange@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        Al Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.com>,
        Johannes Weiner <hannes@cmpxchg.org>, Michal Hocko <mhocko@suse.com>,
        Ross Zwisler <ross.zwisler@linux.intel.com>,
        Junichi Nomura <j-nomura@ce.jp.nec.com>,
        Hugh Dickins <hughd@google.com>,
        Matthew Wilcox <willy@linux.intel.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/filemap: generic_file_read_iter(): check for zero
 reads unconditionally
Message-ID: <20160329084619.GK31585@techsingularity.net>
References: <1458817738-2753-1-git-send-email-nicstange@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-15
Content-Disposition: inline
In-Reply-To: <1458817738-2753-1-git-send-email-nicstange@gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2244
Lines: 56

On Thu, Mar 24, 2016 at 12:08:58PM +0100, Nicolai Stange wrote:
> If
> - generic_file_read_iter() gets called with a zero read length,
> - the read offset is at a page boundary,
> - IOCB_DIRECT is not set
> - and the page in question hasn't made it into the page cache yet,
> then do_generic_file_read() will trigger a readahead with a req_size hint
> of zero.
> 
> Since roundup_pow_of_two(0) is undefined, UBSAN reports
> 
>   UBSAN: Undefined behaviour in include/linux/log2.h:63:13
>   shift exponent 64 is too large for 64-bit type 'long unsigned int'
>   CPU: 3 PID: 1017 Comm: sa1 Tainted: G L 4.5.0-next-20160318+ #14
>   [...]
>   Call Trace:
>    [...]
>    [<ffffffff813ef61a>] ondemand_readahead+0x3aa/0x3d0
>    [<ffffffff813ef61a>] ? ondemand_readahead+0x3aa/0x3d0
>    [<ffffffff813c73bd>] ? find_get_entry+0x2d/0x210
>    [<ffffffff813ef9c3>] page_cache_sync_readahead+0x63/0xa0
>    [<ffffffff813cc04d>] do_generic_file_read+0x80d/0xf90
>    [<ffffffff813cc955>] generic_file_read_iter+0x185/0x420
>    [...]
>    [<ffffffff81510b06>] __vfs_read+0x256/0x3d0
>    [...]
> 
> when get_init_ra_size() gets called from ondemand_readahead().
> 
> The net effect is that the initial readahead size is arch dependent for
> requested read lengths of zero: for example, since
> 
>   1UL << (sizeof(unsigned long) * 8)
> 
> evaluates to 1 on x86 while its result is 0 on ARMv7, the initial readahead
> size becomes 4 on the former and 0 on the latter.
> 
> What's more, whether or not the file access timestamp is updated for zero
> length reads is decided differently for the two cases of IOCB_DIRECT
> being set or cleared: in the first case, generic_file_read_iter()
> explicitly skips updating that timestamp while in the latter case, it is
> always updated through the call to do_generic_file_read().
> 
> According to POSIX, zero length reads "do not modify the last data access
> timestamp" and thus, the IOCB_DIRECT behaviour is POSIXly correct.
> 
> Let generic_file_read_iter() unconditionally check the requested read
> length at its entry and return immediately with success if it is zero.
> 
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs