From patchwork Thu Jun 18 02:51:09 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hin-Tak Leung X-Patchwork-Id: 6632651 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 0D9EA9F3A0 for ; Thu, 18 Jun 2015 02:51:21 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B88B42070E for ; Thu, 18 Jun 2015 02:51:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3D62D20707 for ; Thu, 18 Jun 2015 02:51:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750983AbbFRCvM (ORCPT ); Wed, 17 Jun 2015 22:51:12 -0400 Received: from mail-wi0-f171.google.com ([209.85.212.171]:38580 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751367AbbFRCvL convert rfc822-to-8bit (ORCPT ); Wed, 17 Jun 2015 22:51:11 -0400 Received: by wibdq8 with SMTP id dq8so73044816wib.1 for ; Wed, 17 Jun 2015 19:51:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=lWFBx2fVc4Cfj0AE2P+OnV+32kQJ7yv6rsC4/r1U2nw=; b=mWb9bQ2TGZ1CnQIByRGxKr7wlvgiXZvNOf9XmSmHJHbnopnnep1vpCBidWnlZNnTZt BWvbd2TuCGTe3cstPv6nFQleuhcaFKaTBCRVfeuMgICVozPvz7UwHt7omk3udc0fC4CR nZeQr0IKRnZqNvJ6OYPyYhN19SSPEI7PAh01Cz4u84N+t7VCWlZderxsCdeCL48kpp1v 6UW3FGlsu0ZSgKqYeqVFAkMlDbpYN5VehIeabqtdgDHj7E4viw3sTK5zoenIgPERMeMx oQiptEq7VpfZDBqk/ZzUSdHea6kq/A6DBy5RMsgvSz2b1jScJ6CeFr2sIEYFcYFs+FQa ozhQ== MIME-Version: 1.0 X-Received: by 10.180.206.84 with SMTP id lm20mr12110511wic.48.1434595869818; Wed, 17 Jun 2015 19:51:09 -0700 (PDT) Received: by 10.27.172.143 with HTTP; Wed, 17 Jun 2015 19:51:09 -0700 (PDT) In-Reply-To: <1434584504.1063.YahooMailBasic@web172304.mail.ir2.yahoo.com> References: <1434584504.1063.YahooMailBasic@web172304.mail.ir2.yahoo.com> Date: Thu, 18 Jun 2015 03:51:09 +0100 Message-ID: Subject: Re: [PATCH] hfsplus: release bnode pages after use, not before From: Hin-Tak Leung To: Hin-Tak Leung Cc: Anton Altaparmakov , Andrew Morton , linux-fsdevel@vger.kernel.org, Sasha Levin , Anton Altaparmakov , Al Viro , Christoph Hellwig , Vyacheslav Dubeyko , Sougata Santra , Sergei Antonov 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=ham 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 Hi Andrew and everybody else, I looked through the pre-git history and seem to have found the reason of how the current code had come to be, and why Sergei's fix seems to involve re-enable "#if 0"'ed out code. It is this - I think the code was wrong on entry, in that the first !REF_PAGES should be REF_PAGES. Then the "remove old debug code" commit below did not remove the right stuff. Looking at the old code, I think REF_PAGES may have started out being 1 (i.e. pages are released right after create, then get/put when needed, no need to release on bnode_free), then the code was modified for efficiency so that pages are not released on bnode_create and not put/get at bnode_put/get but release at bnode_free. But it was still largely working in the REF_PAGE=1 mode (and with the commented out release at bnode_free, which is supposed to be spanned by a !REF_PAGE). Then it was flipped over, and seems to work, and things were forgotten. I think the release at bnode_free was commented out because the person who wrote it wasn't sure - I am not sure either, since it seems like there might be other/more places that one might need to release the pages than just at bnode_free(). Does this train of thought make sense? Hin-Tak commit a5e3985fa014029eb6795664c704953720cc7f7d Author: Roman Zippel Date: Tue Sep 6 15:18:47 2005 -0700 [PATCH] hfs: remove debug code This removes some old debug code, which is no longer needed. Signed-off-by: Roman Zippel Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds @@ -612,13 +608,6 @@ void hfs_bnode_get(struct hfs_bnode *node) { if (node) { atomic_inc(&node->refcnt); -#if REF_PAGES - { - int i; - for (i = 0; i < node->tree->pages_per_bnode; i++) - get_page(node->page[i]); - } -#endif dprint(DBG_BNODE_REFS, "get_node(%d:%d): %d\n", node->tree->cnid, node->this, atomic_read(&node->refcnt)); } @@ -635,20 +624,12 @@ void hfs_bnode_put(struct hfs_bnode *node) node->tree->cnid, node->this, atomic_read(&node->refcnt)); if (!atomic_read(&node->refcnt)) BUG(); - if (!atomic_dec_and_lock(&node->refcnt, &tree->hash_lock)) { -#if REF_PAGES - for (i = 0; i < tree->pages_per_bnode; i++) - put_page(node->page[i]); -#endif + if (!atomic_dec_and_lock(&node->refcnt, &tree->hash_lock)) return; - } for (i = 0; i < tree->pages_per_bnode; i++) { if (!node->page[i]) continue; mark_page_accessed(node->page[i]); -#if REF_PAGES - put_page(node->page[i]); -#endif } if (test_bit(HFS_BNODE_DELETED, &node->flags)) { On 18 June 2015 at 00:41, Hin-Tak Leung wrote: > ------------------------------ > On Tue, Jun 9, 2015 7:06 PM BST Anton Altaparmakov wrote: > >>Hi Andrew, >> >>Can you please take this patch up and get it merged into mainline? Despite Vyacheslav's lamentations this patch is obviously correct. __hfs_bnode_create() is called in two places from the driver and the pages it returned are kmap()-ed and used (both read and write) so it is quite obvious they cannot be page_cache_release()-d until after they are kunmap()-ed instead of before they are even kmap()-ed as happens without this patch and this patch fixes exactly this by moving the page_cache_release() calls to after the pages are kunmap()-ed. >> >>Feel free to add: >> >>Reviewed-by: Anton Altaparmakov >> >>Thanks a lot in advance! >> >>Best regards, >> >> Anton > > I went around and looked at the code and Anton is right - __hfs_bnode_create() is called from hfs_bnode_create() and bfs_bnode_find(), and the pages are played with afterwards a bit further, so page_cache_release() at where the current location is, is wrong. > > Since I have a way of seeing a likely-related problem, I'll try testing this change in the next few days and possibly adding a Signed-off at the point. > > BTW, there is a similiar problem with the hfs (no plus) driver also; the "if 0" part is commented out with // c++ style comments instead. > I'm looking over the pre-git history to see how that comes about. > > Hin-Tak > >>> On 7 Jun 2015, at 03:42, Sergei Antonov wrote: >>> >>> Fix this bugreport by Sasha Levin: >>> http://lkml.org/lkml/2015/2/20/85 ("use after free") >>> Make sure mapped pages are available for the entire lifetime of hfs_bnode. >>> >>> Cc: Al Viro >>> Cc: Christoph Hellwig >>> Cc: Andrew Morton >>> Cc: Vyacheslav Dubeyko >>> Cc: Hin-Tak Leung >>> Cc: Sougata Santra >>> Reported-by: Sasha Levin >>> Signed-off-by: Sergei Antonov >>> --- >>> 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); >>> } >>> >>> -- >>> 2.3.0 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >>-- >>Anton Altaparmakov (replace at with @) >>Lead in File System Development, Tuxera Inc., http://www.tuxera.com/ >>Linux NTFS maintainer >> > --- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c index a096c5a..3d5cdc6 100644 --- a/fs/hfs/bnode.c +++ b/fs/hfs/bnode.c @@ -13,8 +13,6 @@ #include "btree.h" -#define REF_PAGES 0 - void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len) { @@ -289,9 +287,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid) page_cache_release(page); goto fail; } -#if !REF_PAGES page_cache_release(page); -#endif node->page[i] = page; } @@ -449,13 +445,6 @@ void hfs_bnode_get(struct hfs_bnode *node) { if (node) { atomic_inc(&node->refcnt); -#if REF_PAGES - { - int i; - for (i = 0; i < node->tree->pages_per_bnode; i++) - get_page(node->page[i]); - } -#endif dprint(DBG_BNODE_REFS, "get_node(%d:%d): %d\n", node->tree->cnid, node->this, atomic_read(&node->refcnt)); } @@ -472,20 +461,12 @@ void hfs_bnode_put(struct hfs_bnode *node) node->tree->cnid, node->this, atomic_read(&node->refcnt)); if (!atomic_read(&node->refcnt)) BUG(); - if (!atomic_dec_and_lock(&node->refcnt, &tree->hash_lock)) { -#if REF_PAGES - for (i = 0; i < tree->pages_per_bnode; i++) - put_page(node->page[i]); -#endif + if (!atomic_dec_and_lock(&node->refcnt, &tree->hash_lock)) return; - } for (i = 0; i < tree->pages_per_bnode; i++) { if (!node->page[i]) continue; mark_page_accessed(node->page[i]); -#if REF_PAGES - put_page(node->page[i]); -#endif } if (test_bit(HFS_BNODE_DELETED, &node->flags)) { diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c index 8868d3b..b85abc6 100644 --- a/fs/hfsplus/bnode.c +++ b/fs/hfsplus/bnode.c @@ -18,8 +18,6 @@ #include "hfsplus_fs.h" #include "hfsplus_raw.h" -#define REF_PAGES 0 - /* Copy a specified range of bytes from the raw data of a node */ void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len) { @@ -450,9 +448,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid) page_cache_release(page); goto fail; } -#if !REF_PAGES page_cache_release(page); -#endif node->page[i] = page; }