From patchwork Thu Jul 2 15:35:55 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Sergei Antonov X-Patchwork-Id: 6711131 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 71AE2C05AC for ; Thu, 2 Jul 2015 15:36:13 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 6413720769 for ; Thu, 2 Jul 2015 15:36:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5E4892075F for ; Thu, 2 Jul 2015 15:36:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753076AbbGBPgF (ORCPT ); Thu, 2 Jul 2015 11:36:05 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:33963 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752430AbbGBPgD (ORCPT ); Thu, 2 Jul 2015 11:36:03 -0400 Received: by wiar9 with SMTP id r9so105131829wia.1; Thu, 02 Jul 2015 08:36:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:mime-version:content-type :content-transfer-encoding; bh=0573j4wKkp4IcaTVbejhmmohmdjodW4rcojz+E9S7YY=; b=fSMZYqHLnjaRV3J6YmbqF8I/ldRpotZcGKa/NqGJouI4u8QwW0PkW5kxdak3ZxKYwN tEcVdTkf1zcTaEJFSndTgIiNGRrOAskCqI9iHsVm9+H49M06rawdvogn4PH8QB2+Ak7T GSAlqcPAKSi5iLKsu/6RKo1RfUwHh/1iFVfHM1gnFdD02KQvSYo5nHtRMCJbN7AlHbqv bAWaadCTIlUBhyQP4vc340DIoYxxnkyEnxKhal+cHmaUwbPjPss/C7wQD9JFB0+BYHoj QZXoV4KmHxArmscVyoMcm4jgaqrm1vA9tKHaYAYhj73t1swpN6sp5QUTOyVAAu1Ue8vZ /UcQ== X-Received: by 10.180.218.227 with SMTP id pj3mr17397413wic.59.1435851361806; Thu, 02 Jul 2015 08:36:01 -0700 (PDT) Received: from localhost.localdomain (HSI-KBW-078-042-106-148.hsi3.kabel-badenwuerttemberg.de. [78.42.106.148]) by mx.google.com with ESMTPSA id fa8sm9208778wib.14.2015.07.02.08.35.59 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 02 Jul 2015 08:36:00 -0700 (PDT) From: Sergei Antonov To: linux-fsdevel@vger.kernel.org, Sasha Levin , =?UTF-8?q?Tomi=20Pievil=C3=A4inen?= , Chris Murphy , David Leder , Michael Fox <415fox@gmail.com>, Hin-Tak Leung , "Reported-by: Luc Pionchon" Cc: stable@vger.kernel.org, Al Viro , Christoph Hellwig , Andrew Morton , Vyacheslav Dubeyko , Sougata Santra , Sergei Antonov Subject: [PATCH v2 2.6 to 4.1] hfsplus: release bnode pages after use, not before Date: Thu, 2 Jul 2015 17:35:55 +0200 Message-Id: <1435851355-1136-1-git-send-email-saproj@gmail.com> X-Mailer: git-send-email 2.3.0 MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP v1 -> v2: * Explained what the bug leads to. * A list of related bug reports was expanded. * More specific technical explanation of the change. Fix the bug which manifests itself in either of two ways. 1. A crash in hfsplus_bnode_read()/hfsplus_brec_lenoff() at an attempt to read unavailable memory. 2. Dmesg errors "recoff %d too large\n" and "keylen %d too large\n" followed by inaccessibility of some or all directories on the mounted hfsplus volume. Example (by Luc Pionchon): $ ls foo/ ls: reading directory foo/: Invalid argument These problems have been reported multiple times: * https://lkml.org/lkml/2015/2/20/85 * https://lkml.org/lkml/2014/10/11/229 * https://bugzilla.kernel.org/show_bug.cgi?id=42342 * https://bugzilla.kernel.org/show_bug.cgi?id=63841 * https://bugzilla.kernel.org/show_bug.cgi?id=78761 * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/740814 * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1027887 * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1332950 * http://comments.gmane.org/gmane.linux.file-systems/73200 * A "du"-related issue mentioned by Hin-Tak Leung on linux-fsdevel@ several times. The following quotation summarizes it. >>> an issue I have had which is quite reproducible: >>> If I merely run "du" repeatedly on a large directory on an HFS+ >>> formatted drive, on a somewhat resource-tight system (having firefox >>> running with lots windows seems to make it happens sooner, and it was >>> on a system with only 2GB memory). >> >>Then what? > > The kernel getting very confused, not being able to read some > files/directories on the HFS+ volume, and having lots of messages in > the kernel log about inconsistent b-tree. > > This appears to be entirely in-memory and has no effect on on-disk data; > also I can reproduce the problem by mounting read-only. If I reboot, > fsck always says the hfs+ volume is clean, and I can repeat the exercise. The bug happened because __hfs_bnode_create() called page_cache_release() right after calling read_mapping_page(). Later the read and immediately released "struct page*" was passed to kmap() and the memory page was read, which worked most of the time under normal circumstances. But under tight memory conditions, by the time the page was kmap()-ped and read, it was either unavailable (see manifestation #1 above) or contained wrong data (see manifestation #2). The patch removes the call to page_cache_release() from __hfs_bnode_create() and adds it to hfs_bnode_free(). Thus making sure mapped pages are available for the entire lifetime of hfs_bnode. In other words, the broken logic of working with b-nodes: "read_mapping_page(), page_cache_release(), kmap(), kunmap()" was changed to the correct one: "read_mapping_page(), kmap(), kunmap(), page_cache_release()". In yet other words, release bnode pages after use, not the other way round. Cc: stable@vger.kernel.org Cc: Al Viro Cc: Christoph Hellwig Cc: Andrew Morton Cc: Vyacheslav Dubeyko Cc: Sougata Santra Reviewed-by: Anton Altaparmakov Signed-off-by: Sergei Antonov Reported-by: Tomi Pieviläinen Reported-by: Chris Murphy Reported-by: David Leder Reported-by: Michael Fox <415fox@gmail.com> Reported-by: Sasha Levin Reported-by: Hin-Tak Leung Reported-by: Luc Pionchon --- fs/hfsplus/bnode.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c index 759708f..5af50fb 100644 --- a/fs/hfsplus/bnode.c +++ b/fs/hfsplus/bnode.c @@ -454,7 +454,6 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid) page_cache_release(page); goto fail; } - page_cache_release(page); node->page[i] = page; } @@ -566,13 +565,12 @@ node_error: void hfs_bnode_free(struct hfs_bnode *node) { -#if 0 int i; - for (i = 0; i < node->tree->pages_per_bnode; i++) + for (i = 0; i < node->tree->pages_per_bnode; i++) { if (node->page[i]) page_cache_release(node->page[i]); -#endif + } kfree(node); }