| Message ID | 20160831123911.3467676-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Delegated to: | Trond Myklebust |
| Headers | show |
> 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.. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } /*
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.