From patchwork Tue Jul 10 08:02:04 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Howells X-Patchwork-Id: 10516387 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 495C4600CA for ; Tue, 10 Jul 2018 08:10:20 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3E4DB28C7A for ; Tue, 10 Jul 2018 08:10:20 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 31AFE28C95; Tue, 10 Jul 2018 08:10:20 +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=unavailable 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 D085928C90 for ; Tue, 10 Jul 2018 08:10:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932968AbeGJICI convert rfc822-to-8bit (ORCPT ); Tue, 10 Jul 2018 04:02:08 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:54228 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932743AbeGJICG (ORCPT ); Tue, 10 Jul 2018 04:02:06 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E845040122D4; Tue, 10 Jul 2018 08:02:05 +0000 (UTC) Received: from warthog.procyon.org.uk (ovpn-120-149.rdu2.redhat.com [10.10.120.149]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0591F2026D6B; Tue, 10 Jul 2018 08:02:04 +0000 (UTC) Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 From: David Howells In-Reply-To: <20180710011741.GA1014@sol.localdomain> References: <20180710011741.GA1014@sol.localdomain> <20180708210154.10423-8-ebiggers3@gmail.com> <20180708210154.10423-1-ebiggers3@gmail.com> <3014.1531139469@warthog.procyon.org.uk> To: Eric Biggers Cc: dhowells@redhat.com, Alexander Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Biggers Subject: Re: [PATCH 07/18] fs_context: fix double free of legacy_fs_context data MIME-Version: 1.0 Content-ID: <25102.1531209724.1@warthog.procyon.org.uk> Date: Tue, 10 Jul 2018 09:02:04 +0100 Message-ID: <25103.1531209724@warthog.procyon.org.uk> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 10 Jul 2018 08:02:05 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Tue, 10 Jul 2018 08:02:05 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'dhowells@redhat.com' RCPT:'' Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Eric Biggers wrote: > Why isn't this done in the same place that ->init_fs_context() would otherwise > be called? It logically does the same thing, right? Fair point. How about the attached incremental change? It breaks the legacy context initialisation out into its own init function and just sets that as the default if the fs doesn't supply its own. It also makes the freeing conditional. David diff --git a/fs/fs_context.c b/fs/fs_context.c index 8af0542ab8b6..f388ab29d37d 100644 --- a/fs/fs_context.c +++ b/fs/fs_context.c @@ -42,6 +42,7 @@ struct legacy_fs_context { enum legacy_fs_param param_type; }; +static int legacy_init_fs_context(struct fs_context *fc, struct dentry *dentry); static const struct fs_context_operations legacy_fs_context_ops; static const match_table_t common_set_sb_flag = { @@ -239,6 +240,7 @@ struct fs_context *vfs_new_fs_context(struct file_system_type *fs_type, unsigned int sb_flags, enum fs_context_purpose purpose) { + int (*init_fs_context)(struct fs_context *, struct dentry *); struct fs_context *fc; int ret = -ENOMEM; @@ -246,15 +248,6 @@ struct fs_context *vfs_new_fs_context(struct file_system_type *fs_type, if (!fc) return ERR_PTR(-ENOMEM); - if (!fs_type->init_fs_context) { - fc->fs_private = kzalloc(sizeof(struct legacy_fs_context), - GFP_KERNEL); - if (!fc->fs_private) - goto err_fc; - - fc->ops = &legacy_fs_context_ops; - } - fc->purpose = purpose; fc->sb_flags = sb_flags; fc->fs_type = get_filesystem(fs_type); @@ -285,11 +278,13 @@ struct fs_context *vfs_new_fs_context(struct file_system_type *fs_type, /* TODO: Make all filesystems support this unconditionally */ - if (fc->fs_type->init_fs_context) { - ret = fc->fs_type->init_fs_context(fc, reference); - if (ret < 0) - goto err_fc; - } + init_fs_context = fc->fs_type->init_fs_context; + if (!init_fs_context) + init_fs_context = legacy_init_fs_context; + + ret = (*init_fs_context)(fc, reference); + if (ret < 0) + goto err_fc; /* Do the security check last because ->init_fs_context may change the * namespace subscriptions. @@ -499,19 +494,21 @@ static void legacy_fs_context_free(struct fs_context *fc) { struct legacy_fs_context *ctx = fc->fs_private; - free_secdata(ctx->secdata); - switch (ctx->param_type) { - case LEGACY_FS_UNSET_PARAMS: - case LEGACY_FS_NO_PARAMS: - break; - case LEGACY_FS_MAGIC_PARAMS: - break; /* ctx->data is a weird pointer */ - default: - kfree(ctx->legacy_data); - break; - } + if (ctx) { + free_secdata(ctx->secdata); + switch (ctx->param_type) { + case LEGACY_FS_UNSET_PARAMS: + case LEGACY_FS_NO_PARAMS: + break; + case LEGACY_FS_MAGIC_PARAMS: + break; /* ctx->data is a weird pointer */ + default: + kfree(ctx->legacy_data); + break; + } - kfree(ctx); + kfree(ctx); + } } /* @@ -707,3 +704,18 @@ static const struct fs_context_operations legacy_fs_context_ops = { .validate = legacy_validate, .get_tree = legacy_get_tree, }; + +/* + * Initialise a legacy context for a filesystem that doesn't support + * fs_context. + */ +static int legacy_init_fs_context(struct fs_context *fc, struct dentry *dentry) +{ + + fc->fs_private = kzalloc(sizeof(struct legacy_fs_context), GFP_KERNEL); + if (!fc->fs_private) + return -ENOMEM; + + fc->ops = &legacy_fs_context_ops; + return 0; +}