Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1473420 > unrolled thread

[PATCH 1/2] NFSv4.1: work around -Wmaybe-uninitialized warning

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] NFSv4.1: work around -Wmaybe-uninitialized warning Arnd Bergmann <arnd@arndb.de> - 2016-08-31 14:40 +0200
    [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released Arnd Bergmann <arnd@arndb.de> - 2016-08-31 14:40 +0200
      Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released David Howells <dhowells@redhat.com> - 2016-08-31 19:40 +0200
        Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released Arnd Bergmann <arnd@arndb.de> - 2016-08-31 21:50 +0200
      Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released David Howells <dhowells@redhat.com> - 2016-08-31 22:30 +0200
        Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released Arnd Bergmann <arnd@arndb.de> - 2016-08-31 22:40 +0200
          Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released David Howells <dhowells@redhat.com> - 2016-08-31 23:10 +0200
      Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released David Howells <dhowells@redhat.com> - 2016-08-31 22:30 +0200
        Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released Arnd Bergmann <arnd@arndb.de> - 2016-08-31 22:40 +0200
        Re: [PATCH 2/2] rxrpc: fix undefined behavior in  rxrpc_mark_call_released David Miller <davem@davemloft.net> - 2016-08-31 23:00 +0200
    Re: [PATCH 1/2] NFSv4.1: work around -Wmaybe-uninitialized warning Trond Myklebust <trondmy@primarydata.com> - 2016-08-31 15:20 +0200
      Re: [PATCH 1/2] NFSv4.1: work around -Wmaybe-uninitialized warning Arnd Bergmann <arnd@arndb.de> - 2016-08-31 15:40 +0200
        Re: [PATCH 1/2] NFSv4.1: work around -Wmaybe-uninitialized warning Trond Myklebust <trondmy@primarydata.com> - 2016-08-31 17:10 +0200
          Re: [PATCH 1/2] NFSv4.1: work around -Wmaybe-uninitialized warning Arnd Bergmann <arnd@arndb.de> - 2016-08-31 18:00 +0200

#1473420 — [PATCH 1/2] NFSv4.1: work around -Wmaybe-uninitialized warning

A bugfix introduced a harmless gcc warning in nfs4_slot_seqid_in_use:

fs/nfs/nfs4session.c:203:54: error: 'cur_seq' may be used uninitialized in this function [-Werror=maybe-uninitialized]

gcc is not smart enough to conclude that the IS_ERR/PTR_ERR pair
results in a nonzero return value here. Using PTR_ERR_OR_ZERO()
instead makes this clear to the compiler.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: e09c978aae5b ("NFSv4.1: Fix Oopsable condition in server callback races")
---
 fs/nfs/nfs4session.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

The patch that caused this just came in for v4.8-rc5. As the warning
is now disabled by default and this is harmless, this can probably
get queued for v4.9 instead.

I mentioned earlier that I got the new warning for net-next, but
failed to notice that it had come from mainline instead.

diff --git a/fs/nfs/nfs4session.c b/fs/nfs/nfs4session.c
index b62973045a3e..150c5a1879bf 100644
--- a/fs/nfs/nfs4session.c
+++ b/fs/nfs/nfs4session.c
@@ -178,12 +178,14 @@ static int nfs4_slot_get_seqid(struct nfs4_slot_table  *tbl, u32 slotid,
 	__must_hold(&tbl->slot_tbl_lock)
 {
 	struct nfs4_slot *slot;
+	int ret;
 
 	slot = nfs4_lookup_slot(tbl, slotid);
-	if (IS_ERR(slot))
-		return PTR_ERR(slot);
-	*seq_nr = slot->seq_nr;
-	return 0;
+	ret = PTR_ERR_OR_ZERO(slot);
+	if (!ret)
+		*seq_nr = slot->seq_nr;
+
+	return ret;
 }
 
 /*
-- 
2.9.0

[toc] | [next] | [standalone]

#1473422 — [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

gcc -Wmaybe-initialized correctly points out a newly introduced bug
through which we can end up calling rxrpc_queue_call() for a dead
connection:

net/rxrpc/call_object.c: In function 'rxrpc_mark_call_released':
net/rxrpc/call_object.c:600:5: error: 'sched' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This sets the 'sched' variable to zero to restore the previous
behavior.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: f5c17aaeb2ae ("rxrpc: Calls should only have one terminal state")
---
 net/rxrpc/call_object.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index 104ee8b1de06..2daec1eaec6f 100644
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -595,6 +595,8 @@ static void rxrpc_mark_call_released(struct rxrpc_call *call)
 		sched = __rxrpc_abort_call(call, RX_CALL_DEAD, ECONNRESET);
 		if (!test_and_set_bit(RXRPC_CALL_EV_RELEASE, &call->events))
 			sched = true;
+	} else {
+		sched = 0;
 	}
 	write_unlock(&call->state_lock);
 	if (sched)
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1473742 — Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

Arnd Bergmann <arnd@arndb.de> wrote:

> gcc -Wmaybe-initialized correctly points out a newly introduced bug
> through which we can end up calling rxrpc_queue_call() for a dead
> connection:

How do you turn that on from within the Kbuild system?

David

[toc] | [prev] | [next] | [standalone]

#1473814 — Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

On Wednesday, August 31, 2016 6:39:04 PM CEST David Howells wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > gcc -Wmaybe-initialized correctly points out a newly introduced bug
> > through which we can end up calling rxrpc_queue_call() for a dead
> > connection:
> 
> How do you turn that on from within the Kbuild system?

You don't, my mistake. My build bot runs with 6e8d666e9253 ("Disable
"maybe-uninitialized" warning globally") disabled, and I had
assumed that Linus left the warning enabled with "make W=1", but
that was incorrect as Trond Myklebust also pointed out.

You still get the warning with "make EXTRA_CFLAGS=-Wmaybe-uninitialized",
which of course nobody normally does.

I'll try to come up with a patch to enable the warning in the W=1
level in the same conditions that used to be enabled up to v4.7.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1473826 — Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

Arnd Bergmann <arnd@arndb.de> wrote:

> +	} else {
> +		sched = 0;

That should be false, not 0, btw.

David

[toc] | [prev] | [next] | [standalone]

#1473834 — Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

On Wednesday, August 31, 2016 9:26:21 PM CEST David Howells wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > +     } else {
> > +             sched = 0;
> 
> That should be false, not 0, btw.
> 

Right, sorry about that. Do you want me to resend the fixed version,
or do you apply and fix it yourself?

As patch 1/2 isn't actually meant for net-next anyway, the series
doesn't need to stay together.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1473881 — Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

Arnd Bergmann <arnd@arndb.de> wrote:

> Right, sorry about that. Do you want me to resend the fixed version,
> or do you apply and fix it yourself?

I can fix it up myself.  I'll pull it into my tree when I've finished doing
the fixing up I'm currently working on.

David

[toc] | [prev] | [next] | [standalone]

#1473830 — Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

Is there a 1/2 somewhere?  I don't see it.

David

[toc] | [prev] | [next] | [standalone]

#1473836 — Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

On Wednesday, August 31, 2016 9:25:46 PM CEST David Howells wrote:
> Is there a 1/2 somewhere?  I don't see it.
> 
> David

Sorry, mixed up the Cc list. It only went to netdev and lkml and isn't
really related. That one was a workaround for a false-positive
 -Wmaybe-uninitialized warning in NFS, see https://lkml.org/lkml/2016/8/31/412

	Arnd

[toc] | [prev] | [next] | [standalone]

#1473868 — Re: [PATCH 2/2] rxrpc: fix undefined behavior in rxrpc_mark_call_released

From: David Howells <dhowells@redhat.com>
Date: Wed, 31 Aug 2016 21:25:46 +0100

> Is there a 1/2 somewhere?  I don't see it.

It was an NFSv4 patch.

[toc] | [prev] | [next] | [standalone]

#1473450

> On Aug 31, 2016, at 08:39, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> A bugfix introduced a harmless gcc warning in nfs4_slot_seqid_in_use:
> 
> fs/nfs/nfs4session.c:203:54: error: 'cur_seq' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> gcc is not smart enough to conclude that the IS_ERR/PTR_ERR pair
> results in a nonzero return value here. Using PTR_ERR_OR_ZERO()
> instead makes this clear to the compiler.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: e09c978aae5b ("NFSv4.1: Fix Oopsable condition in server callback races")
> ---
> fs/nfs/nfs4session.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
> 
> The patch that caused this just came in for v4.8-rc5. As the warning
> is now disabled by default and this is harmless, this can probably
> get queued for v4.9 instead.
> 
> I mentioned earlier that I got the new warning for net-next, but
> failed to notice that it had come from mainline instead.
> 
> diff --git a/fs/nfs/nfs4session.c b/fs/nfs/nfs4session.c
> index b62973045a3e..150c5a1879bf 100644
> --- a/fs/nfs/nfs4session.c
> +++ b/fs/nfs/nfs4session.c
> @@ -178,12 +178,14 @@ static int nfs4_slot_get_seqid(struct nfs4_slot_table  *tbl, u32 slotid,
> 	__must_hold(&tbl->slot_tbl_lock)
> {
> 	struct nfs4_slot *slot;
> +	int ret;
> 
> 	slot = nfs4_lookup_slot(tbl, slotid);
> -	if (IS_ERR(slot))
> -		return PTR_ERR(slot);
> -	*seq_nr = slot->seq_nr;
> -	return 0;
> +	ret = PTR_ERR_OR_ZERO(slot);
> +	if (!ret)
> +		*seq_nr = slot->seq_nr;
> +
> +	return ret;
> }
> 

What version of gcc are you using? I’m unable to reproduce with gcc 6.1.1..

[toc] | [prev] | [next] | [standalone]

#1473475

On Wednesday, August 31, 2016 1:17:48 PM CEST Trond Myklebust wrote:
> What version of gcc are you using? I’m unable to reproduce with gcc 6.1.1..

This is also on 6.1.1 for ARM. Note that 6e8d666e9253 ("Disable
"maybe-uninitialized" warning globally") turned off those warnings, so
unless you explicitly pass -Wmaybe-uninitialized (e.g. by building with
"make W=1"), you won't get it.

The reason I'm still sending the patches for this warning is that
we do get a number of valid ones (this was the only false positive
out of the seven such warnings since last week).

	Arnd

[toc] | [prev] | [next] | [standalone]

#1473590

> On Aug 31, 2016, at 09:37, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> On Wednesday, August 31, 2016 1:17:48 PM CEST Trond Myklebust wrote:
>> What version of gcc are you using? I’m unable to reproduce with gcc 6.1.1..
> 
> This is also on 6.1.1 for ARM. Note that 6e8d666e9253 ("Disable
> "maybe-uninitialized" warning globally") turned off those warnings, so
> unless you explicitly pass -Wmaybe-uninitialized (e.g. by building with
> "make W=1"), you won't get it.
> 

I’m not getting that error on gcc 6.1.1 for x86_64 with either “make W=1” or “make W=2”.
“make W=3” does gives rise to one warning in nfs4_slot_get_seqid:

/home/trondmy/devel/kernel/linux/fs/nfs/nfs4session.c: In function ‘nfs4_slot_get_seqid’:
/home/trondmy/devel/kernel/linux/fs/nfs/nfs4session.c:184:10: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
   return PTR_ERR(slot);
          ^~~~~~~~~~~~~

(which is another false positive) but that’s all...

> The reason I'm still sending the patches for this warning is that
> we do get a number of valid ones (this was the only false positive
> out of the seven such warnings since last week).

There is a Zen-like quality to IS_ERR() when it casts a const pointer to an unsigned long, back to a non-const pointer, and then back to an unsigned long before comparing it to another unsigned long cast constant negative integer. However, I’m not sure the C99 standard would agree that a positive test result implies we can assume that a simple cast of the same pointer to a signed long will result in a negative, non-zero valued errno.

I suspect that if we really want to fix these false negatives, we should probably address that issue.

Cheers
  Trond

[toc] | [prev] | [next] | [standalone]

#1473629

On Wednesday, August 31, 2016 3:02:42 PM CEST Trond Myklebust wrote:
> > On Aug 31, 2016, at 09:37, Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> > On Wednesday, August 31, 2016 1:17:48 PM CEST Trond Myklebust wrote:
> >> What version of gcc are you using? I’m unable to reproduce with gcc 6.1.1..
> > 
> > This is also on 6.1.1 for ARM. Note that 6e8d666e9253 ("Disable
> > "maybe-uninitialized" warning globally") turned off those warnings, so
> > unless you explicitly pass -Wmaybe-uninitialized (e.g. by building with
> > "make W=1"), you won't get it.
> > 
> 
> I’m not getting that error on gcc 6.1.1 for x86_64 with either “make W=1” or “make W=2”.
> “make W=3” does gives rise to one warning in nfs4_slot_get_seqid:

Ok, I had not realized that the patch that Linus did disabled the warning
for all levels, I'll try to come up a patch to bring it back at W=1 level.

On my system, I had simply reverted the patch that turned off the
warning, but I have now verified that I get it with
"make EXTRA_CFLAGS=-Wmaybe-uninitialized" on an x86 defconfig with gcc-5 and
gcc-6.

> /home/trondmy/devel/kernel/linux/fs/nfs/nfs4session.c: In function ‘nfs4_slot_get_seqid’:
> /home/trondmy/devel/kernel/linux/fs/nfs/nfs4session.c:184:10: warning: conversion to ‘int’ from ‘long int’ may alter its value [-Wconversion]
>    return PTR_ERR(slot);
>           ^~~~~~~~~~~~~
> 
> (which is another false positive) but that’s all...

sure, W=3 is useless.

> > The reason I'm still sending the patches for this warning is that
> > we do get a number of valid ones (this was the only false positive
> > out of the seven such warnings since last week).
> 
> There is a Zen-like quality to IS_ERR() when it casts a const pointer to an unsigned long, back to a non-const pointer, and then back to an unsigned long before comparing it to another unsigned long cast constant negative integer. However, I’m not sure the C99 standard would agree that a positive test result implies we can assume that a simple cast of the same pointer to a signed long will result in a negative, non-zero valued errno.
> 
> I suspect that if we really want to fix these false negatives, we should probably address that issue.

I've looked into this before, as we've had a couple of these cases (I
think less than 10 in the whole kernel, but they keep coming up every
few releases), and I couldn't find a way to make IS_ERR more transparent.

Using IS_ERR_OR_ZERO() seems like a good enough solution, and will
probably result in slightly better code (I have not checked this
specific case though), as we can also skip the second runtime check.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web