From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933812AbcBQOz2 (ORCPT ); Wed, 17 Feb 2016 09:55:28 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:50469 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756422AbcBQOz0 (ORCPT ); Wed, 17 Feb 2016 09:55:26 -0500 X-AuditID: cbfec7f5-f79b16d000005389-14-56c489dc9ef4 Subject: Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage To: Arnd Bergmann References: <20160217023127.GG17997@ZenIV.linux.org.uk> <1455712889-10633-1-git-send-email-a.hajda@samsung.com> <10818862.KbfsAs5PD4@wuerfel> Cc: linux-kernel@vger.kernel.org, Bartlomiej Zolnierkiewicz , Marek Szyprowski , viro@zeniv.linux.org.uk From: Andrzej Hajda Message-id: <56C48993.1050508@samsung.com> Date: Wed, 17 Feb 2016 15:54:11 +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: <10818862.KbfsAs5PD4@wuerfel> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrDLMWRmVeSWpSXmKPExsVy+t/xy7p3Oo+EGVx9zGHxd9IxdouNM9az WlzeNYfNYu2Ru+wW5/8eZ3Vg9fj9axKjR9+WVYwenzfJeWx68pYpgCWKyyYlNSezLLVI3y6B K2PDgcyC22IV7/sPMjYwLhbqYuTkkBAwkTh8cSsrhC0mceHeerYuRi4OIYGljBITN5yDcp4z SmzYO5cdpEpYwFZi28bFzCC2iICixNQXz5ghiiYySjz+voQVxGEWWMAocfj0FrAONgFNib+b b7KB2LwCWhKPJs5nBLFZBFQl9iw6xQJiiwpESBzu7GKHqBGU+DH5HlicE6j+5ceNQBs4gIbq Sdy/qAUSZhaQl9i85i3zBEaBWUg6ZiFUzUJStYCReRWjaGppckFxUnqukV5xYm5xaV66XnJ+ 7iZGSBh/3cG49JjVIUYBDkYlHt4VWYfDhFgTy4orcw8xSnAwK4nwdjcdCRPiTUmsrEotyo8v Ks1JLT7EKM3BoiTOO3PX+xAhgfTEktTs1NSC1CKYLBMHp1QDo5lk6K5jOp93eXCfb8xQ33a+ +Xv080mrYv0FL7dFlfff9woI12OYkr6j8oDq9LIuntRHzX49R0LzUzNePnm969m0x5Z2HbMa pvOGbwgOjehTtpxTq1tyR+rPYWn+8kLXM0Uv2qIbNOeGd3DsW/X2Fs9RrxuqLh/Wcyww5RJS mcO0d0bp1sPnlFiKMxINtZiLihMBT8gwOl8CAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/17/2016 02:42 PM, Arnd Bergmann wrote: > 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 > > I am not sure if you are aware of the fact these structures are exposed to user space. Is it OK to add such unions to them? Regards Andrzej