From: Andrzej Hajda <a.hajda@samsung.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: linux-kernel@vger.kernel.org,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Marek Szyprowski <m.szyprowski@samsung.com>,
viro@zeniv.linux.org.uk
Subject: Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage
Date: Wed, 17 Feb 2016 15:54:11 +0100 [thread overview]
Message-ID: <56C48993.1050508@samsung.com> (raw)
In-Reply-To: <10818862.KbfsAs5PD4@wuerfel>
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
next prev parent reply other threads:[~2016-02-17 14:55 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-15 14:35 [PATCH 0/7] fix IS_ERR_VALUE usage Andrzej Hajda
2016-02-15 14:35 ` [PATCH 1/7] netfilter: " Andrzej Hajda
2016-02-17 2:31 ` Al Viro
2016-02-17 8:45 ` Andrzej Hajda
2016-02-17 12:41 ` [PATCH v2 " Andrzej Hajda
2016-02-17 13:42 ` Arnd Bergmann
2016-02-17 14:54 ` Andrzej Hajda [this message]
2016-02-17 15:40 ` Arnd Bergmann
2016-02-15 14:35 ` [PATCH 2/7] MIPS: module: fix incorrect IS_ERR_VALUE macro usages Andrzej Hajda
2016-02-15 14:35 ` [PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage Andrzej Hajda
2016-02-17 2:33 ` Al Viro
2016-02-15 14:35 ` [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage Andrzej Hajda
2016-02-17 2:33 ` Al Viro
2016-02-21 16:04 ` Guennadi Liakhovetski
2016-02-15 14:35 ` [PATCH 5/7] serial: clps711x: " Andrzej Hajda
2016-02-17 2:33 ` Al Viro
2016-02-15 14:35 ` [PATCH 6/7] fbdev: exynos: " Andrzej Hajda
2016-02-16 13:36 ` Tomi Valkeinen
2016-02-15 14:35 ` [PATCH 7/7] usb: gadget: fsl_qe_udc: " Andrzej Hajda
2016-02-17 10:48 ` [PATCH 0/7] " Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56C48993.1050508@samsung.com \
--to=a.hajda@samsung.com \
--cc=arnd@arndb.de \
--cc=b.zolnierkie@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).