LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Nick Desaulniers <ndesaulniers@google.com>
To: Will Deacon <will@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	kernel-team <kernel-team@android.com>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Peter Zijlstra <peterz@infradead.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Segher Boessenkool <segher@kernel.crashing.org>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Peter Oberparleiter <oberpar@linux.ibm.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Pablo Neira Ayuso <pablo@netfilter.org>,
	Jozsef Kadlecsik <kadlec@netfilter.org>,
	Florian Westphal <fw@strlen.de>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v2 02/10] netfilter: Avoid assigning 'const' pointer to non-const pointer
Date: Thu, 23 Jan 2020 11:07:59 -0800
Message-ID: <CAKwvOdm2snorniFunMF=0nDH8-RFwm7wtjYK_Tcwkd+JZinYPg@mail.gmail.com> (raw)
In-Reply-To: <20200123153341.19947-3-will@kernel.org>

On Thu, Jan 23, 2020 at 7:33 AM Will Deacon <will@kernel.org> wrote:
>
> nf_remove_net_hook() uses WRITE_ONCE() to assign a 'const pointer to a
> 'non-const' pointer. Cleanups to the implementation of WRITE_ONCE() mean
> that this will give rise to a compiler warning, just like a plain old
> assignment would do:
>
>   | In file included from ./include/linux/export.h:43,
>   |                  from ./include/linux/linkage.h:7,
>   |                  from ./include/linux/kernel.h:8,
>   |                  from net/netfilter/core.c:9:
>   | net/netfilter/core.c: In function ‘nf_remove_net_hook’:
>   | ./include/linux/compiler.h:216:30: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>   |   *(volatile typeof(x) *)&(x) = (val);  \
>   |                               ^
>   | net/netfilter/core.c:379:3: note: in expansion of macro ‘WRITE_ONCE’
>   |    WRITE_ONCE(orig_ops[i], &dummy_ops);
>   |    ^~~~~~~~~~
>
> Follow the pattern used elsewhere in this file and add a cast to 'void *'
> to squash the warning.
>
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  net/netfilter/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index 78f046ec506f..3ac7c8c1548d 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -376,7 +376,7 @@ static bool nf_remove_net_hook(struct nf_hook_entries *old,
>                 if (orig_ops[i] != unreg)
>                         continue;
>                 WRITE_ONCE(old->hooks[i].hook, accept_all);
> -               WRITE_ONCE(orig_ops[i], &dummy_ops);
> +               WRITE_ONCE(orig_ops[i], (void *)&dummy_ops);

Good thing it's the variable being modified was not declared const; I
get spooked when I see -Wdiscarded-qualifiers because of Section
6.7.3.6 of the ISO C11 draft spec:

```
If an attempt is made to modify an object defined with a
const-qualified type through use
of an lvalue with non-const-qualified type, the behavior is undefined.
If an attempt is
made to refer to an object defined with a volatile-qualified type
through use of an lvalue
with non-volatile-qualified type, the behavior is undefined.133)

133) This applies to those objects that behave as if they were defined
with qualified types, even if they are
never actually defined as objects in the program (such as an object at
a memory-mapped input/output
address).
```

Which is about the modification of a const-declared variable (explicit
UB which Clang actively exploits), and doesn't apply in this case.  I
agree that this is the best way to fix this due to the use of typeof()
and it's semantics of dropping qualifiers; declaring `dummy_ops` as
non-const would be another, but that is worse IMO.  Thanks for the
patch.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>                 return true;
>         }
>
> --
> 2.25.0.341.g760bfbb309-goog
>


-- 
Thanks,
~Nick Desaulniers

  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-23 15:33 [PATCH v2 00/10] Rework READ_ONCE() to improve codegen Will Deacon
2020-01-23 15:33 ` [PATCH v2 01/10] compiler/gcc: Emit build-time warning for GCC prior to version 4.8 Will Deacon
2020-01-23 15:33 ` [PATCH v2 02/10] netfilter: Avoid assigning 'const' pointer to non-const pointer Will Deacon
2020-01-23 19:07   ` Nick Desaulniers [this message]
2020-01-24  8:24     ` Peter Zijlstra
2020-01-24 17:20       ` Nick Desaulniers
2020-01-27 12:04         ` David Laight
2020-01-24 17:36       ` Linus Torvalds
2020-01-24 22:00         ` Peter Zijlstra
2020-01-27 12:21         ` David Laight
2020-01-23 15:33 ` [PATCH v2 03/10] fault_inject: Don't rely on "return value" from WRITE_ONCE() Will Deacon
2020-01-23 15:33 ` [PATCH v2 04/10] READ_ONCE: Simplify implementations of {READ,WRITE}_ONCE() Will Deacon
2020-01-23 15:33 ` [PATCH v2 05/10] READ_ONCE: Enforce atomicity for {READ,WRITE}_ONCE() memory accesses Will Deacon
2020-01-25  8:27   ` Peter Zijlstra
2020-01-29 10:49     ` Peter Zijlstra
2020-01-23 15:33 ` [PATCH v2 06/10] READ_ONCE: Drop pointer qualifiers when reading from scalar types Will Deacon
2020-01-23 15:33 ` [PATCH v2 07/10] locking/barriers: Use '__unqual_scalar_typeof' for load-acquire macros Will Deacon
2020-01-23 15:33 ` [PATCH v2 08/10] arm64: barrier: Use '__unqual_scalar_typeof' for acquire/release macros Will Deacon
2020-01-23 15:33 ` [PATCH v2 09/10] compiler/gcc: Raise minimum GCC version for kernel builds to 4.8 Will Deacon
2020-01-23 18:36   ` Nick Desaulniers
2020-01-24  8:26     ` Peter Zijlstra
2020-01-24 17:05       ` Nick Desaulniers
2020-01-24 23:29         ` Peter Zijlstra
2020-01-25 10:34         ` Michael Ellerman
2020-01-23 15:33 ` [PATCH v2 10/10] gcov: Remove old GCC 3.4 support Will Deacon
2020-01-23 18:51   ` Nick Desaulniers
2020-01-28 14:56   ` Peter Oberparleiter
2020-01-23 17:07 ` [PATCH v2 00/10] Rework READ_ONCE() to improve codegen David Laight
2020-01-23 17:16   ` Will Deacon
2020-01-23 17:32     ` David Laight
2020-01-23 18:45       ` Nick Desaulniers
2020-01-23 19:01         ` Arvind Sankar
2020-01-24 10:11           ` David Laight
2020-01-26  1:10           ` Qais Yousef
2020-01-27  7:26             ` Arnd Bergmann
2020-01-23 17:59 ` Linus Torvalds
2020-01-24  8:33   ` Peter Zijlstra
2020-01-24 10:41     ` Peter Zijlstra
2020-02-10  9:50     ` Masahiro Yamada
2020-02-10  9:59       ` Will Deacon
2020-01-31 10:20 ` David Howells

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='CAKwvOdm2snorniFunMF=0nDH8-RFwm7wtjYK_Tcwkd+JZinYPg@mail.gmail.com' \
    --to=ndesaulniers@google.com \
    --cc=arnd@arndb.de \
    --cc=borntraeger@de.ibm.com \
    --cc=davem@davemloft.net \
    --cc=fw@strlen.de \
    --cc=kadlec@netfilter.org \
    --cc=kernel-team@android.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luc.vanoostenryck@gmail.com \
    --cc=masahiroy@kernel.org \
    --cc=mpe@ellerman.id.au \
    --cc=oberpar@linux.ibm.com \
    --cc=pablo@netfilter.org \
    --cc=peterz@infradead.org \
    --cc=segher@kernel.crashing.org \
    --cc=torvalds@linux-foundation.org \
    --cc=will@kernel.org \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git