From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030244AbcBQIrK (ORCPT ); Wed, 17 Feb 2016 03:47:10 -0500 Received: from mailout3.w1.samsung.com ([210.118.77.13]:43528 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030204AbcBQIrG (ORCPT ); Wed, 17 Feb 2016 03:47:06 -0500 X-AuditID: cbfec7f4-f79026d00000418a-fb-56c433873efe Subject: Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage To: Al Viro References: <1455546925-22119-1-git-send-email-a.hajda@samsung.com> <1455546925-22119-2-git-send-email-a.hajda@samsung.com> <20160217023127.GG17997@ZenIV.linux.org.uk> Cc: linux-kernel@vger.kernel.org, Bartlomiej Zolnierkiewicz , Marek Szyprowski , Pablo Neira Ayuso , Patrick McHardy , Jozsef Kadlecsik , "David S. Miller" , Alexey Kuznetsov , James Morris , Hideaki YOSHIFUJI , netfilter-devel@vger.kernel.org, coreteam@netfilter.org From: Andrzej Hajda Message-id: <56C4333E.5070905@samsung.com> Date: Wed, 17 Feb 2016 09:45:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-version: 1.0 In-reply-to: <20160217023127.GG17997@ZenIV.linux.org.uk> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprIIsWRmVeSWpSXmKPExsVy+t/xa7rtxkfCDK5NF7PYOGM9q8Xfne3M FnPOt7BYrFu/mMniweaLrBaX+6YxW1xoewVk7ZrDZrH2yF12iwnrTrFYTH9zldni/N/jrBZf 93axOPB6nG7ayOKxZeVNJo+2a6uYPLb8/s7m0fM92ePt7xNMHn1bVjF6LJ20kMnj8yY5j01P 3jIFcEVx2aSk5mSWpRbp2yVwZWw6P5etYJpQxZcrG1gbGDfxdTFyckgImEh83jyPGcIWk7hw bz1bFyMXh5DAUkaJda8PsEM4zxkluhcdAqsSFrCSePWgkRXEFhFQlbhz6gwTRNFqRonFB9vB EswCz5gltsyMBbHZBDQl/m6+yQZi8wpoSVy6swushgWoef6abYwgtqhAhMThzi52iBpBiR+T 77GA2JwCFhLvXi8HinMAzdSTuH9RC2K8vMTmNW+ZJzAKzELSMQuhahaSqgWMzKsYRVNLkwuK k9JzDfWKE3OLS/PS9ZLzczcxQiLqyw7GxcesDjEKcDAq8fAGZBwOE2JNLCuuzD3EKMHBrCTC W6x3JEyINyWxsiq1KD++qDQntfgQozQHi5I479xd70OEBNITS1KzU1MLUotgskwcnFINjOJv YiY+z055r2OdlCF+1iF92xPhmnLWZ2nCER8dOBcdjZe7OKOQY0n9Ne+IV4oPDWr7/7r3Horq mZZ48sS5yP2vCwVYb7970rJ654Gr6ZunMijFHWMTfvjqSpkT727lhueXYuO6Vpz3neg+gd0q 6/YhHgEOtdjyBWe/OvNIFK2wvxXZqvZOVYmlOCPRUIu5qDgRAK7GRPekAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/17/2016 03:31 AM, Al Viro wrote: > On Mon, Feb 15, 2016 at 03:35:19PM +0100, Andrzej Hajda wrote: >> IS_ERR_VALUE should be used only with unsigned long type. >> Otherwise it can work incorrectly. To achieve this function >> xt_percpu_counter_alloc is modified to return unsigned long, >> and its result is assigned to temporary variable to perform >> error checking, before assigning to .pcnt field. > Wrong fix, IMO. Just have an anon union of u64 pcnt and > struct xt_counters __percpu *pcpu in there. And make this > >> +static inline unsigned long xt_percpu_counter_alloc(void) >> { >> if (nr_cpu_ids > 1) { >> void __percpu *res = __alloc_percpu(sizeof(struct xt_counters), >> sizeof(struct xt_counters)); >> >> if (res == NULL) >> - return (u64) -ENOMEM; >> + return -ENOMEM; >> >> - return (u64) (__force unsigned long) res; >> + return (__force unsigned long) res; >> } >> >> return 0; > take struct xt_counters * and return 0 or -ENOMEM. Storing the result of > allocation in ->pcpu of passed structure. > > I mean, look at the callers - > >> - e->counters.pcnt = xt_percpu_counter_alloc(); >> - if (IS_ERR_VALUE(e->counters.pcnt)) >> + pcnt = xt_percpu_counter_alloc(); >> + if (IS_ERR_VALUE(pcnt)) >> return -ENOMEM; >> + e->counters.pcnt = pcnt; > should be > if (xt_percpu_counter_alloc(&e->counters) < 0) > return -ENOMEM; > > and similar for the rest of callers. Moreover, if you look at the _users_ > of that fields, you'll see that a bunch of those actually want to use > ->pcpu instead of doing all those casts. > > Really, that's the point - IS_ERR_VALUE is a big red flag saying "we need > to figure out what's going on in that place", which does include reading > through the code. Mechanical "solutions" like that only hide the problem. > > I just tried to make the patch the least invasive :) The problem with your proposition is that struct xt_counters is exposed to userspace as well as the structs containing it: struct arpt_entry, struct ipt_entry, struct ip6t_entry Mixing __percpu pointer into these structures seems problematic. Maybe it would be better to skip adding union and do ugly casting in xt_percpu_counter_alloc(struct xt_counters *) and friends. Regards Andrzej