From patchwork Fri Dec 14 22:14:23 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Sandeen X-Patchwork-Id: 10731741 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id A3A6C746 for ; Fri, 14 Dec 2018 22:14:26 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 50E3D29D83 for ; Fri, 14 Dec 2018 22:14:26 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 422BE2A193; Fri, 14 Dec 2018 22:14:26 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1B8EC29D83 for ; Fri, 14 Dec 2018 22:14:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731266AbeLNWOY (ORCPT ); Fri, 14 Dec 2018 17:14:24 -0500 Received: from sandeen.net ([63.231.237.45]:52794 "EHLO sandeen.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730956AbeLNWOY (ORCPT ); Fri, 14 Dec 2018 17:14:24 -0500 Received: from [10.0.0.4] (liberator [10.0.0.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by sandeen.net (Postfix) with ESMTPSA id 616653274; Fri, 14 Dec 2018 16:14:12 -0600 (CST) Subject: [PATCH 2/3 V2] mm: don't search past page end in is_partially_uptodate From: Eric Sandeen To: linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org Cc: hch@lst.de References: <1544739929-21651-1-git-send-email-sandeen@sandeen.net> <1544739929-21651-3-git-send-email-sandeen@sandeen.net> Openpgp: preference=signencrypt Autocrypt: addr=sandeen@sandeen.net; prefer-encrypt=mutual; keydata= xsFNBE6x99QBEADMR+yNFBc1Y5avoUhzI/sdR9ANwznsNpiCtZlaO4pIWvqQJCjBzp96cpCs nQZV32nqJBYnDpBDITBqTa/EF+IrHx8gKq8TaSBLHUq2ju2gJJLfBoL7V3807PQcI18YzkF+ WL05ODFQ2cemDhx5uLghHEeOxuGj+1AI+kh/FCzMedHc6k87Yu2ZuaWF+Gh1W2ix6hikRJmQ vj5BEeAx7xKkyBhzdbNIbbjV/iGi9b26B/dNcyd5w2My2gxMtxaiP7q5b6GM2rsQklHP8FtW ZiYO7jsg/qIppR1C6Zr5jK1GQlMUIclYFeBbKggJ9mSwXJH7MIftilGQ8KDvNuV5AbkronGC sEEHj2khs7GfVv4pmUUHf1MRIvV0x3WJkpmhuZaYg8AdJlyGKgp+TQ7B+wCjNTdVqMI1vDk2 BS6Rg851ay7AypbCPx2w4d8jIkQEgNjACHVDU89PNKAjScK1aTnW+HNUqg9BliCvuX5g4z2j gJBs57loTWAGe2Ve3cMy3VoQ40Wt3yKK0Eno8jfgzgb48wyycINZgnseMRhxc2c8hd51tftK LKhPj4c7uqjnBjrgOVaVBupGUmvLiePlnW56zJZ51BR5igWnILeOJ1ZIcf7KsaHyE6B1mG+X dmYtjDhjf3NAcoBWJuj8euxMB6TcQN2MrSXy5wSKaw40evooGwARAQABzSVFcmljIFIuIFNh bmRlZW4gPHNhbmRlZW5Ac2FuZGVlbi5uZXQ+wsF7BBMBAgAlAhsDBgsJCAcDAgYVCAIJCgsE FgIDAQIeAQIXgAUCUzMzbAIZAQAKCRAgrhaS4T3e4Fr7D/wO+fenqVvHjq21SCjDCrt8HdVj aJ28B1SqSU2toxyg5I160GllAxEHpLFGdbFAhQfBtnmlY9eMjwmJb0sCIrkrB6XNPSPA/B2B UPISh0z2odJv35/euJF71qIFgWzp2czJHkHWwVZaZpMWWNvsLIroXoR+uA9c2V1hQFVAJZyk EE4xzfm1+oVtjIC12B9tTCuS00pY3AUy21yzNowT6SSk7HAzmtG/PJ/uSB5wEkwldB6jVs2A sjOg1wMwVvh/JHilsQg4HSmDfObmZj1d0RWlMWcUE7csRnCE0ZWBMp/ttTn+oosioGa09HAS 9jAnauznmYg43oQ5Akd8iQRxz5I58F/+JsdKvWiyrPDfYZtFS+UIgWD7x+mHBZ53Qjazszox gjwO9ehZpwUQxBm4I0lPDAKw3HJA+GwwiubTSlq5PS3P7QoCjaV8llH1bNFZMz2o8wPANiDx 5FHgpRVgwLHakoCU1Gc+LXHXBzDXt7Cj02WYHdFzMm2hXaslRdhNGowLo1SXZFXa41KGTlNe 4di53y9CK5ynV0z+YUa+5LR6RdHrHtgywdKnjeWdqhoVpsWIeORtwWGX8evNOiKJ7j0RsHha WrePTubr5nuYTDsQqgc2r4aBIOpeSRR2brlT/UE3wGgy9LY78L4EwPR0MzzecfE1Ws60iSqw Pu3vhb7h3c7BTQROsffUARAA0DrUifTrXQzqxO8aiQOC5p9Tz25Np/Tfpv1rofOwL8VPBMvJ X4P5l1V2yd70MZRUVgjmCydEyxLJ6G2YyHO2IZTEajUY0Up+b3ErOpLpZwhvgWatjifpj6bB SKuDXeThqFdkphF5kAmgfVAIkan5SxWK3+S0V2F/oxstIViBhMhDwI6XsRlnVBoLLYcEilxA 2FlRUS7MOZGmRJkRtdGD5koVZSM6xVZQSmfEBaYQ/WJBGJQdPy94nnlAVn3lH3+N7pXvNUuC GV+t4YUt3tLcRuIpYBCOWlc7bpgeCps5Xa0dIZgJ8Louu6OBJ5vVXjPxTlkFdT0S0/uerCG5 1u8p6sGRLnUeAUGkQfIUqGUjW2rHaXgWNvzOV6i3tf9YaiXKl3avFaNW1kKBs0T5M1cnlWZU Utl6k04lz5OjoNY9J/bGyV3DSlkblXRMK87iLYQSrcV6cFz9PRl4vW1LGff3xRQHngeN5fPx ze8X5NE3hb+SSwyMSEqJxhVTXJVfQWWW0dQxP7HNwqmOWYF/6m+1gK/Y2gY3jAQnsWTru4RV TZGnKwEPmOCpSUvsTRXsVHgsWJ70qd0yOSjWuiv4b8vmD3+QFgyvCBxPMdP3xsxN5etheLMO gRwWpLn6yNFq/xtgs+ECgG+gR78yXQyA7iCs5tFs2OrMqV5juSMGmn0kxJUAEQEAAcLBXwQY AQIACQUCTrH31AIbDAAKCRAgrhaS4T3e4BKwD/0ZOOmUNOZCSOLAMjZx3mtYtjYgfUNKi0ki YPveGoRWTqbis8UitPtNrG4XxgzLOijSdOEzQwkdOIp/QnZhGNssMejCnsluK0GQd+RkFVWN mcQT78hBeGcnEMAXZKq7bkIKzvc06GFmkMbX/gAl6DiNGv0UNAX+5FYh+ucCJZSyAp3sA+9/ LKjxnTedX0aygXA6rkpX0Y0FvN/9dfm47+LGq7WAqBOyYTU3E6/+Z72bZoG/cG7ANLxcPool LOrU43oqFnD8QwcN56y4VfFj3/jDF2MX3xu4v2OjglVjMEYHTCxP3mpxesGHuqOit/FR+mF0 MP9JGfj6x+bj/9JMBtCW1bY/aPeMdPGTJvXjGtOVYblGZrSjXRn5++Uuy36CvkcrjuziSDG+ JEexGxczWwN4mrOQWhMT5Jyb+18CO+CWxJfHaYXiLEW7dI1AynL4jjn4W0MSiXpWDUw+fsBO Pk6ah10C4+R1Jc7dyUsKksMfvvhRX1hTIXhth85H16706bneTayZBhlZ/hK18uqTX+s0onG/ m1F3vYvdlE4p2ts1mmixMF7KajN9/E5RQtiSArvKTbfsB6Two4MthIuLuf+M0mI4gPl9SPlf fWCYVPhaU9o83y1KFbD/+lh1pjP7bEu/YudBvz7F2Myjh4/9GUAijrCTNeDTDAgvIJDjXuLX pA== Message-ID: <5338b144-4501-25a7-2064-79103c7afb8e@sandeen.net> Date: Fri, 14 Dec 2018 16:14:23 -0600 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1544739929-21651-3-git-send-email-sandeen@sandeen.net> Content-Language: en-US Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP When generic_file_buffered_read() calls ->is_partially_uptodate() on a page today, it passes in iov->count as the length to check, which may extend past the end of the page. The original implementation, block_is_partially_uptodate(), clamps the length to the end of the page. However, the iomap implementation does not do so, and tries to check all blocks to the end of the iovec, which may be beyond the page, and may be beyond the iop->uptodate bitmap as well. I think the worst that will happen is that we may eventually find a zero bit and return "not partially uptodate" when it would have otherwise returned true, and skip the optimization. Still, it's clearly an invalid memory access that must be fixed. Zorro noticed this when KASAN went off for 512 byte blocks on a 64k page system: BUG: KASAN: slab-out-of-bounds in iomap_is_partially_uptodate+0x1a0/0x1e0 Read of size 8 at addr ffff800120c3a318 by task fsstress/22337 and dchinner pointed out the underlying flaw. Fix this by limiting the range we ask for in the caller (generic_file_buffered_read()) so that offset+len doesn't extend past the page in question, and remove the clamping from the block variant. This also hoists the "don't bother checking each block if we span the whole page" optimization from the block implementation. It changes the arg types too, because we no longer need a long to hold offsets or lengths that will fit within a page. Reported-by: Zorro Lang Signed-off-by: Eric Sandeen --- V2: Move the common code (clamping & optimizations) to the caller, rather than re-implementing in the iomap implementation. This makes my patch 3/3 unnecessary Dave, if you've already done and tested the same thing, happy to just use your patch if it's in better shape. diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking index efea228c..b7cc610 100644 --- a/Documentation/filesystems/Locking +++ b/Documentation/filesystems/Locking @@ -208,7 +208,7 @@ prototypes: int (*migratepage)(struct address_space *, struct page *, struct page *); void (*putback_page) (struct page *); int (*launder_page)(struct page *); - int (*is_partially_uptodate)(struct page *, unsigned long, unsigned long); + int (*is_partially_uptodate)(struct page *, unsigned, unsigned); int (*error_remove_page)(struct address_space *, struct page *); int (*swap_activate)(struct file *); int (*swap_deactivate)(struct file *); diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index 5f71a25..ed6d581 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -648,8 +648,7 @@ struct address_space_operations { void (*putback_page) (struct page *); int (*launder_page) (struct page *); - int (*is_partially_uptodate) (struct page *, unsigned long, - unsigned long); + int (*is_partially_uptodate) (struct page *, unsigned, unsigned); void (*is_dirty_writeback) (struct page *, bool *, bool *); int (*error_remove_page) (struct mapping *mapping, struct page *page); int (*swap_activate)(struct file *); diff --git a/fs/buffer.c b/fs/buffer.c index 1286c2b..b8605e5 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -2170,8 +2170,8 @@ int generic_write_end(struct file *file, struct address_space *mapping, * Returns true if all buffers which correspond to a file portion * we want to read are uptodate. */ -int block_is_partially_uptodate(struct page *page, unsigned long from, - unsigned long count) +int block_is_partially_uptodate(struct page *page, unsigned int from, + unsigned int count) { unsigned block_start, block_end, blocksize; unsigned to; @@ -2183,10 +2183,7 @@ int block_is_partially_uptodate(struct page *page, unsigned long from, head = page_buffers(page); blocksize = head->b_size; - to = min_t(unsigned, PAGE_SIZE - from, count); - to = from + to; - if (from < blocksize && to > PAGE_SIZE - blocksize) - return 0; + to = from + count; bh = head; block_start = 0; diff --git a/fs/iomap.c b/fs/iomap.c index d6bc98a..6fec633 100644 --- a/fs/iomap.c +++ b/fs/iomap.c @@ -493,8 +493,7 @@ struct iomap_readpage_ctx { EXPORT_SYMBOL_GPL(iomap_readpages); int -iomap_is_partially_uptodate(struct page *page, unsigned long from, - unsigned long count) +iomap_is_partially_uptodate(struct page *page, unsigned from, unsigned count) { struct iomap_page *iop = to_iomap_page(page); struct inode *inode = page->mapping->host; diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h index 7b73ef7..0fef1e5 100644 --- a/include/linux/buffer_head.h +++ b/include/linux/buffer_head.h @@ -220,8 +220,8 @@ int __block_write_full_page(struct inode *inode, struct page *page, get_block_t *get_block, struct writeback_control *wbc, bh_end_io_t *handler); int block_read_full_page(struct page*, get_block_t*); -int block_is_partially_uptodate(struct page *page, unsigned long from, - unsigned long count); +int block_is_partially_uptodate(struct page *page, unsigned from, + unsigned count); int block_write_begin(struct address_space *mapping, loff_t pos, unsigned len, unsigned flags, struct page **pagep, get_block_t *get_block); int __block_write_begin(struct page *page, loff_t pos, unsigned len, diff --git a/include/linux/fs.h b/include/linux/fs.h index c95c080..95a0ec6 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -378,8 +378,7 @@ struct address_space_operations { bool (*isolate_page)(struct page *, isolate_mode_t); void (*putback_page)(struct page *); int (*launder_page) (struct page *); - int (*is_partially_uptodate) (struct page *, unsigned long, - unsigned long); + int (*is_partially_uptodate) (struct page *, unsigned, unsigned); void (*is_dirty_writeback) (struct page *, bool *, bool *); int (*error_remove_page)(struct address_space *, struct page *); diff --git a/include/linux/iomap.h b/include/linux/iomap.h index edcdb3a..d59e986 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -127,8 +127,8 @@ ssize_t iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *from, int iomap_readpages(struct address_space *mapping, struct list_head *pages, unsigned nr_pages, const struct iomap_ops *ops); int iomap_set_page_dirty(struct page *page); -int iomap_is_partially_uptodate(struct page *page, unsigned long from, - unsigned long count); +int iomap_is_partially_uptodate(struct page *page, unsigned from, + unsigned count); int iomap_releasepage(struct page *page, gfp_t gfp_mask); void iomap_invalidatepage(struct page *page, unsigned int offset, unsigned int len); diff --git a/mm/filemap.c b/mm/filemap.c index 81adec8..0fab63f 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1985,7 +1985,8 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb, pgoff_t index; pgoff_t last_index; pgoff_t prev_index; - unsigned long offset; /* offset into pagecache page */ + unsigned offset, count; /* offset into pagecache page */ + unsigned blocksize; unsigned int prev_offset; int error = 0; @@ -2045,9 +2046,21 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb, if (PageUptodate(page)) goto page_ok; - if (inode->i_blkbits == PAGE_SHIFT || + blocksize = i_blocksize(inode); + /* There can be no partial for page-sized blocks */ + if (blocksize == PAGE_SIZE || !mapping->a_ops->is_partially_uptodate) goto page_not_up_to_date; + /* Limit query to within a single page */ + count = min_t(unsigned, PAGE_SIZE - offset, + iter->count); + /* + * If the range spans all blocks in the page, and the + * page is not uptodate, we can't be partially uptodate. + */ + if (offset < blocksize && + offset + count > PAGE_SIZE - blocksize) + goto page_not_up_to_date; /* pipes can't handle partially uptodate pages */ if (unlikely(iov_iter_is_pipe(iter))) goto page_not_up_to_date; @@ -2057,7 +2070,7 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb, if (!page->mapping) goto page_not_up_to_date_locked; if (!mapping->a_ops->is_partially_uptodate(page, - offset, iter->count)) + offset, count)) goto page_not_up_to_date_locked; unlock_page(page); }