From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423078AbcBQNm2 (ORCPT ); Wed, 17 Feb 2016 08:42:28 -0500 Received: from mout.kundenserver.de ([217.72.192.75]:50289 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1422804AbcBQNm0 (ORCPT ); Wed, 17 Feb 2016 08:42:26 -0500 From: Arnd Bergmann To: Andrzej Hajda Cc: linux-kernel@vger.kernel.org, Bartlomiej Zolnierkiewicz , Marek Szyprowski , viro@zeniv.linux.org.uk Subject: Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage Date: Wed, 17 Feb 2016 14:42:16 +0100 Message-ID: <10818862.KbfsAs5PD4@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <1455712889-10633-1-git-send-email-a.hajda@samsung.com> References: <20160217023127.GG17997@ZenIV.linux.org.uk> <1455712889-10633-1-git-send-email-a.hajda@samsung.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:QAPxYd9I+eTnp7cJgLo37VSY8Q1Gz/DVYjCRpHbs1trWg6vZdll AULdwvIJtgHb3/bumXkCHvKddOYeKBCHMfYsjf2DH3x2QMWhc8CGa/0sjsvZkTZSvlWTP1M hDvqWPQ+pzHEMrPnmnKzcqAtG8ZfGZTdWHDHSU/K++NqOXhe+fzsRdbmLJPj7Y+eThY/S57 4phhoMsVV5bEw9/rx77fA== X-UI-Out-Filterresults: notjunk:1;V01:K0:fbKT3/MsQbs=:Z54uDPoJkv6a/56AOnHMvz s1bV0ryjeiUlGXDOn8YZAc93oD82y7hScLEMsFS4n+ZIHRn5LnrAPl3GUF39dY2Gkh+M/QaKm HVtmkcuBF98biMNllf0XFMw7ShETgjBvcy33cRxzBv//JhojtiFV+fZKFxH1YW14Wm8K1Tng7 KEDJIpELSZLVAWNmokr+a+lFJDOd592L+3Gz/azOylX1wWZ8vA/ahpzWXm393pOrFJpPTlyEE BgQ+ogodWuh2kwuE0dKmMMST0E6ckMSYefUHlsovHuNqt6kbcFFBJoRd3gV3ZCm8HNPtWD22c WP+fD+35a5GjAzz3YloPUMvYpl9riN6DB7npkZVADBSAfj9/8Coo/HTI9sRSShpXFsvYZogS9 hpPwT5U7CcoLsrxhZWW9EeHoZIUSvYKJmgv7ENm3T53KeGBfFYKGcdERPhIyLRetQJ3Q/jZo1 nGlUwuildOnizzhu5+6ofNL7nbnnOHgZRx0l+FNhUp9aWr6MK5gACxpN5NfNLAiHDvaZ2injR SpJ0mx2A9N5t3K3maprmpnIyv5ZHlSWuTWmFfAmKQSkYzjvOd5keayMxJLN5knu+myUJYG+T1 Wa+vDreHzJDQiUacfm6WaRKBEInCz5CgnhwpdlpCy9gW9BJefN8npJRd7paBv4QExKLcLLcgn cbI1bB0ERo7PK/b1LuLs0PdoORdUukyIc/7VwyNaEvOey5UdaIbWE/FodBnruKdYtqwtH1JOi 3dmvnsVn/r+lqRDQ Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday 17 February 2016 13:41:29 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 only error code, pointer to counters is passed as an > argument. Helper union have been created to avoid ugly typecasting and > make code more readable. > > The patch follows conclusion from discussion on LKML [1][2]. > > [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927 > [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581 I think it would be helpful to mention here how the current code is actually broken, i.e. that we set the u64 value to (u64)-ENOMEM on failure but then compare it to (unsigned long)-MAX_ERRNO, which is much smaller on a 32-bit architecture, and basically relies on never even needing the range of the u64 variable. It works because we only do this comparison at allocation time, while in the non-SMP case it might be larger than (unsigned long)-MAX_ERRNO later but then we don't do the IS_ERR_VALUE comparison any more. > -/* On SMP, ip(6)t_entry->counters.pcnt holds address of the > - * real (percpu) counter. On !SMP, its just the packet count, > - * so nothing needs to be done there. > - * > - * xt_percpu_counter_alloc returns the address of the percpu > - * counter, or 0 on !SMP. We force an alignment of 16 bytes > - * so that bytes/packets share a common cache line. > - * > - * Hence caller must use IS_ERR_VALUE to check for error, this > - * allows us to return 0 for single core systems without forcing > - * callers to deal with SMP vs. NONSMP issues. > +/* > + * On SMP, (ip|ip6|arp)t_entry->counters holds address of the real (percpu) > + * counter. On !SMP, it is just the packet count. union ext_counters is used > + * to model this ambiguity in kernel without changing (ip|ip6|arp)t_entry > + * structures as these are exposed to userspace. > */ > -static inline u64 xt_percpu_counter_alloc(void) > +union xt_smp_counters { > + struct xt_counters counters; > + struct xt_counters __percpu *smp_counters; > +}; > + > +static inline union xt_smp_counters *to_xt_smp_counters(struct xt_counters *cnt) > +{ > + return container_of(cnt, union xt_smp_counters, counters); > +} The union is a bit ugly, but I can't think of a much better way to do this. However, could you put the union into the three users (struct arpt_entry etc) to avoid having to cast the inner structure into the union using container_of()? It doesn't feel right to use container_of() in this way here. Arnd