From patchwork Mon Nov 7 15:49:19 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Marshall X-Patchwork-Id: 9415435 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 29DBD60720 for ; Mon, 7 Nov 2016 15:49:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1C12928AA5 for ; Mon, 7 Nov 2016 15:49:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 10BC928B5C; Mon, 7 Nov 2016 15:49:42 +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=-6.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID 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 13D2F28A47 for ; Mon, 7 Nov 2016 15:49:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932359AbcKGPtb (ORCPT ); Mon, 7 Nov 2016 10:49:31 -0500 Received: from mail-yw0-f178.google.com ([209.85.161.178]:35393 "EHLO mail-yw0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753456AbcKGPt3 (ORCPT ); Mon, 7 Nov 2016 10:49:29 -0500 Received: by mail-yw0-f178.google.com with SMTP id i145so45476486ywg.2 for ; Mon, 07 Nov 2016 07:49:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=omnibond-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id; bh=dCpThcF4ihOMFvyzJK05mplNdkAMaIeKjdVc0EqMQx4=; b=LGEjAgYX479jQIyMlro1cblwdxYKjHGMKpyT9BrWojVsNecWB6KzVL2atBM46AGZms PY1/z2SGlZws9QIIQx2SHW7y8/o7LusuiAHo5AwjUCuMVBXZ/wYxbDUvF0U1+EFOXxiw Rl/1fKOzMZFL1MFZZlcfSg6DS3O3hwhqoaszojW2VgJV4BtF6YxScwGR2C90gdDiYe8w ujBuHbUUs35MKpCoePC46we8C7h/Lga1BQnqe8beGMDj0LLTJMHozsrT52kPPQq9qFVS fuwjLnzmZXirgwUoh/6PARS4MGUtXXPfdgtqMrhR8Pfd+fAITWkdB9X4bIDwSOpEsxn7 5i9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=dCpThcF4ihOMFvyzJK05mplNdkAMaIeKjdVc0EqMQx4=; b=hwYKHTfaqMl2uTZObp6oUbcq8WJ6c3VbC8hGflSn4cXdreq5ZiZVneEarNGAjIGjqA +YC74jijyhS5gyYAIu82GuG4Lnn/M/s0Fhprj9+ssl5mjRmzEXt+ckl9+UQweHgoHg4D S/PH3Jwk2PM0UWwgvGz1W7Ls5J99zlwKR5RS7neEGrZPrcYJYGNtreqSIH28kScicU19 aTBjzP1Qf5wVAB/rMaEvRyJzLrIvnqABFCWNbeAh8kGfrAoloosA47Zp2p0tpX92szvz 9zHUWSeYIZV70GqNZzRlwL5sgxfmsbJ8D/bjfB/MPYFt8zOIT4+l91gOWw4cl3zvsZwr BIwA== X-Gm-Message-State: ABUngvdwMHyfLqAmgpq2CurJ172nx06mT4Txit8fsesNEcJLHNlbKJnGOKG8iet6MzLovQ== X-Received: by 10.129.166.214 with SMTP id d205mr7258147ywh.170.1478533768358; Mon, 07 Nov 2016 07:49:28 -0800 (PST) Received: from logtruck.clemson.edu (logtruck.clemson.edu. [130.127.148.78]) by smtp.gmail.com with ESMTPSA id c199sm5145536ywb.3.2016.11.07.07.49.26 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 07 Nov 2016 07:49:27 -0800 (PST) From: Mike Marshall To: viro@zeniv.linux.org.uk Cc: Mike Marshall , linux-fsdevel@vger.kernel.org, Martin Brandenburg Subject: [PATCH] orangefs: clean up debugfs Date: Mon, 7 Nov 2016 10:49:19 -0500 Message-Id: <1478533759-21755-1-git-send-email-hubcap@omnibond.com> X-Mailer: git-send-email 1.8.3.1 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 We recently refactored the Orangefs debugfs code. The refactor seemed to trigger dan.carpenter@oracle.com's static tester to find a possible double-free in the code. While designing the fix we saw a condition under which the buffer being freed could also be overflowed. We also realized how to rebuild the related debugfs file's "contents" (a string) without deleting and re-creating the file. This fix should eliminate the possible double-free, the potential overflow and improve code readability. Signed-off-by: Mike Marshall Signed-off-by: Martin Brandenburg --- fs/orangefs/orangefs-debugfs.c | 147 ++++++++++++++++++----------------------- fs/orangefs/orangefs-mod.c | 6 +- 2 files changed, 68 insertions(+), 85 deletions(-) diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c index eb09aa0..d484068 100644 --- a/fs/orangefs/orangefs-debugfs.c +++ b/fs/orangefs/orangefs-debugfs.c @@ -141,6 +141,9 @@ static ssize_t orangefs_debug_write(struct file *, */ static DEFINE_MUTEX(orangefs_debug_lock); +/* Used to protect data in ORANGEFS_KMOD_DEBUG_HELP_FILE */ +static DEFINE_MUTEX(orangefs_help_file_lock); + /* * initialize kmod debug operations, create orangefs debugfs dir and * ORANGEFS_KMOD_DEBUG_HELP_FILE. @@ -289,6 +292,8 @@ static void *help_start(struct seq_file *m, loff_t *pos) gossip_debug(GOSSIP_DEBUGFS_DEBUG, "help_start: start\n"); + mutex_lock(&orangefs_help_file_lock); + if (*pos == 0) payload = m->private; @@ -305,6 +310,7 @@ static void *help_next(struct seq_file *m, void *v, loff_t *pos) static void help_stop(struct seq_file *m, void *p) { gossip_debug(GOSSIP_DEBUGFS_DEBUG, "help_stop: start\n"); + mutex_unlock(&orangefs_help_file_lock); } static int help_show(struct seq_file *m, void *v) @@ -610,32 +616,54 @@ static int orangefs_prepare_cdm_array(char *debug_array_string) * /sys/kernel/debug/orangefs/debug-help can be catted to * see all the available kernel and client debug keywords. * - * When the kernel boots, we have no idea what keywords the + * When orangefs.ko initializes, we have no idea what keywords the * client supports, nor their associated masks. * - * We pass through this function once at boot and stamp a + * We pass through this function once at module-load and stamp a * boilerplate "we don't know" message for the client in the * debug-help file. We pass through here again when the client * starts and then we can fill out the debug-help file fully. * * The client might be restarted any number of times between - * reboots, we only build the debug-help file the first time. + * module reloads, we only build the debug-help file the first time. */ int orangefs_prepare_debugfs_help_string(int at_boot) { - int rc = -EINVAL; - int i; - int byte_count = 0; char *client_title = "Client Debug Keywords:\n"; char *kernel_title = "Kernel Debug Keywords:\n"; + size_t string_size = DEBUG_HELP_STRING_SIZE; + size_t result_size; + size_t i; + char *new; + int rc = -EINVAL; gossip_debug(GOSSIP_UTILS_DEBUG, "%s: start\n", __func__); - if (at_boot) { - byte_count += strlen(HELP_STRING_UNINITIALIZED); + if (at_boot) client_title = HELP_STRING_UNINITIALIZED; - } else { - /* + + /* build a new debug_help_string. */ + new = kzalloc(DEBUG_HELP_STRING_SIZE, GFP_KERNEL); + if (!new) { + rc = -ENOMEM; + goto out; + } + + /* + * strlcat(dst, src, size) will append at most + * "size - strlen(dst) - 1" bytes of src onto dst, + * null terminating the result, and return the total + * length of the string it tried to create. + * + * We'll just plow through here building our new debug + * help string and let strlcat take care of assuring that + * dst doesn't overflow. + */ + strlcat(new, client_title, string_size); + + if (!at_boot) { + + /* * fill the client keyword/mask array and remember * how many elements there were. */ @@ -644,64 +672,40 @@ int orangefs_prepare_debugfs_help_string(int at_boot) if (cdm_element_count <= 0) goto out; - /* Count the bytes destined for debug_help_string. */ - byte_count += strlen(client_title); - for (i = 0; i < cdm_element_count; i++) { - byte_count += strlen(cdm_array[i].keyword + 2); - if (byte_count >= DEBUG_HELP_STRING_SIZE) { - pr_info("%s: overflow 1!\n", __func__); - goto out; - } + strlcat(new, "\t", string_size); + strlcat(new, cdm_array[i].keyword, string_size); + strlcat(new, "\n", string_size); } - - gossip_debug(GOSSIP_UTILS_DEBUG, - "%s: cdm_element_count:%d:\n", - __func__, - cdm_element_count); } - byte_count += strlen(kernel_title); + strlcat(new, "\n", string_size); + strlcat(new, kernel_title, string_size); + for (i = 0; i < num_kmod_keyword_mask_map; i++) { - byte_count += - strlen(s_kmod_keyword_mask_map[i].keyword + 2); - if (byte_count >= DEBUG_HELP_STRING_SIZE) { - pr_info("%s: overflow 2!\n", __func__); - goto out; - } + strlcat(new, "\t", string_size); + strlcat(new, s_kmod_keyword_mask_map[i].keyword, string_size); + result_size = strlcat(new, "\n", string_size); } - /* build debug_help_string. */ - debug_help_string = kzalloc(DEBUG_HELP_STRING_SIZE, GFP_KERNEL); - if (!debug_help_string) { - rc = -ENOMEM; + /* See if we tried to put too many bytes into "new"... */ + if (result_size >= string_size) { + kfree(new); goto out; } - strcat(debug_help_string, client_title); - - if (!at_boot) { - for (i = 0; i < cdm_element_count; i++) { - strcat(debug_help_string, "\t"); - strcat(debug_help_string, cdm_array[i].keyword); - strcat(debug_help_string, "\n"); - } - } - - strcat(debug_help_string, "\n"); - strcat(debug_help_string, kernel_title); - - for (i = 0; i < num_kmod_keyword_mask_map; i++) { - strcat(debug_help_string, "\t"); - strcat(debug_help_string, s_kmod_keyword_mask_map[i].keyword); - strcat(debug_help_string, "\n"); + if (at_boot) { + debug_help_string = new; + } else { + mutex_lock(&orangefs_help_file_lock); + memset(debug_help_string, 0, DEBUG_HELP_STRING_SIZE); + strlcat(debug_help_string, new, string_size); + mutex_unlock(&orangefs_help_file_lock); } rc = 0; -out: - - return rc; +out: return rc; } @@ -959,8 +963,12 @@ int orangefs_debugfs_new_client_string(void __user *arg) ret = copy_from_user(&client_debug_array_string, (void __user *)arg, ORANGEFS_MAX_DEBUG_STRING_LEN); - if (ret != 0) + + if (ret != 0) { + pr_info("%s: CLIENT_STRING: copy_from_user failed\n", + __func__); return -EIO; + } /* * The real client-core makes an effort to ensure @@ -975,45 +983,18 @@ int orangefs_debugfs_new_client_string(void __user *arg) client_debug_array_string[ORANGEFS_MAX_DEBUG_STRING_LEN - 1] = '\0'; - if (ret != 0) { - pr_info("%s: CLIENT_STRING: copy_from_user failed\n", - __func__); - return -EIO; - } - pr_info("%s: client debug array string has been received.\n", __func__); if (!help_string_initialized) { - /* Free the "we don't know yet" default string... */ - kfree(debug_help_string); - - /* build a proper debug help string */ + /* Build a proper debug help string. */ if (orangefs_prepare_debugfs_help_string(0)) { gossip_err("%s: no debug help string \n", __func__); return -EIO; } - /* Replace the boilerplate boot-time debug-help file. */ - debugfs_remove(help_file_dentry); - - help_file_dentry = - debugfs_create_file( - ORANGEFS_KMOD_DEBUG_HELP_FILE, - 0444, - debug_dir, - debug_help_string, - &debug_help_fops); - - if (!help_file_dentry) { - gossip_err("%s: debugfs_create_file failed for" - " :%s:!\n", - __func__, - ORANGEFS_KMOD_DEBUG_HELP_FILE); - return -EIO; - } } debug_mask_to_string(&client_debug_mask, 1); diff --git a/fs/orangefs/orangefs-mod.c b/fs/orangefs/orangefs-mod.c index 2e5b030..4113eb0 100644 --- a/fs/orangefs/orangefs-mod.c +++ b/fs/orangefs/orangefs-mod.c @@ -124,7 +124,7 @@ static int __init orangefs_init(void) * unknown at boot time. * * orangefs_prepare_debugfs_help_string will be used again - * later to rebuild the debug-help file after the client starts + * later to rebuild the debug-help-string after the client starts * and passes along the needed info. The argument signifies * which time orangefs_prepare_debugfs_help_string is being * called. @@ -152,7 +152,9 @@ static int __init orangefs_init(void) ret = register_filesystem(&orangefs_fs_type); if (ret == 0) { - pr_info("orangefs: module version %s loaded\n", ORANGEFS_VERSION); + pr_info("%s: module version %s loaded\n", + __func__, + ORANGEFS_VERSION); ret = 0; goto out; }