Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751887AbdF0DmL (ORCPT ); Mon, 26 Jun 2017 23:42:11 -0400 Received: from gateway31.websitewelcome.com ([192.185.143.31]:21889 "EHLO gateway31.websitewelcome.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751473AbdF0DmH (ORCPT ); Mon, 26 Jun 2017 23:42:07 -0400 Date: Mon, 26 Jun 2017 22:42:03 -0500 Message-ID: <20170626224203.Horde.ALtRsX1tRN9DvF9T3nJ53UG@gator4166.hostgator.com> From: "Gustavo A. R. Silva" To: Joe Perches Cc: Pablo Neira Ayuso , Jozsef Kadlecsik , Florian Westphal , "David S. Miller" , Alexey Kuznetsov , James Morris , Hideaki YOSHIFUJI , Patrick McHardy , netfilter-devel@vger.kernel.org, coreteam@netfilter.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] netfilter: ip_tables: remove useless variable assignment in get_info() References: <20170626223428.GA11614@embeddedgus> <1498531618.24295.57.camel@perches.com> In-Reply-To: <1498531618.24295.57.camel@perches.com> User-Agent: Horde Application Framework 5 Content-Type: text/plain; charset=utf-8; format=flowed; DelSp=Yes MIME-Version: 1.0 Content-Disposition: inline X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - gator4166.hostgator.com X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - embeddedor.com X-BWhitelist: no X-Source-IP: 108.167.133.22 X-Exim-ID: 1dPhO8-001077-4j X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: gator4166.hostgator.com [108.167.133.22]:29105 X-Source-Auth: garsilva@embeddedor.com X-Email-Count: 1 X-Source-Cap: Z3V6aWRpbmU7Z3V6aWRpbmU7Z2F0b3I0MTY2Lmhvc3RnYXRvci5jb20= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2056 Lines: 75 Hi Joe, Quoting Joe Perches : > On Mon, 2017-06-26 at 17:34 -0500, Gustavo A. R. Silva wrote: >> Value assigned to variable _ret_ at line 970 is overwritten either at >> line 986 or 988, before it can be used. This makes such variable >> assignment useless. >> >> Addresses-Coverity-ID: 1226932 > [] >> diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c > [] >> @@ -967,7 +967,7 @@ static int get_info(struct net *net, void __user *user, >> struct xt_table_info tmp; >> >> if (compat) { >> - ret = compat_table_info(private, &tmp); >> + compat_table_info(private, &tmp); > > why isn't it more appropriate to test the return value? > Oh, in this particular case, based on git blame, the code has been like that for more than 10 years. So my reasoning was that if it hasn't been fixed yet, maybe that return value is not relevant. But in case it turns out to actually be relevant, what do you think about the following patch: --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -968,7 +968,8 @@ static int get_info(struct net *net, void __user *user, if (compat) { ret = compat_table_info(private, &tmp); - xt_compat_flush_offsets(AF_INET); + if (!ret) + goto out; private = &tmp; } #endif @@ -986,14 +987,20 @@ static int get_info(struct net *net, void __user *user, ret = -EFAULT; else ret = 0; + } else + ret = -ENOENT; +out: + if (t) { xt_table_unlock(t); module_put(t->me); - } else - ret = -ENOENT; + } + #ifdef CONFIG_COMPAT - if (compat) + if (compat) { + xt_compat_flush_offsets(AF_INET); xt_compat_unlock(AF_INET); + } #endif return ret; } Thank you! -- Gustavo A. R. Silva