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 <redacted>
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.
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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(+)
On Aug 31, 2016, at 08:39, Arnd Bergmann [off-list ref] 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.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 31, 2016, at 09:37, Arnd Bergmann [off-list ref] wrote:
On Wednesday, August 31, 2016 1:17:48 PM CEST Trond Myklebust wrote:
quoted
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��칻
�&�~�&���+-��ݶ��w��˛���m�b��g~ȧ���ܨ}���Ơz�&j:+v����n�r��6;靫3��\
nnX��f�z��2�ޙ���&�)ߡ�a����
�G���h��j:+v���w�٥
On Wednesday, August 31, 2016 3:02:42 PM CEST Trond Myklebust wrote:
quoted
On Aug 31, 2016, at 09:37, Arnd Bergmann [off-list ref] wrote:
On Wednesday, August 31, 2016 1:17:48 PM CEST Trond Myklebust wrote:
quoted
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.
quoted
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
On Wednesday, August 31, 2016 6:39:04 PM CEST David Howells wrote:
Arnd Bergmann [off-list ref] wrote:
quoted
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
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
On Wednesday, August 31, 2016 9:26:21 PM CEST David Howells wrote:
Arnd Bergmann [off-list ref] wrote:
quoted
+ } 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