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 <[email protected]>
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
> On Aug 31, 2016, at 08:39, Arnd Bergmann <[email protected]> wrote:
>=20
> A bugfix introduced a harmless gcc warning in nfs4_slot_seqid_in_use:
>=20
> fs/nfs/nfs4session.c:203:54: error: 'cur_seq' may be used uninitialized i=
n this function [-Werror=3Dmaybe-uninitialized]
>=20
> 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.
>=20
> Signed-off-by: Arnd Bergmann <[email protected]>
> Fixes: e09c978aae5b ("NFSv4.1: Fix Oopsable condition in server callback =
races")
> ---
> fs/nfs/nfs4session.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>=20
> 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.
>=20
> I mentioned earlier that I got the new warning for net-next, but
> failed to notice that it had come from mainline instead.
>=20
> 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_tab=
le *tbl, u32 slotid,
> =09__must_hold(&tbl->slot_tbl_lock)
> {
> =09struct nfs4_slot *slot;
> +=09int ret;
>=20
> =09slot =3D nfs4_lookup_slot(tbl, slotid);
> -=09if (IS_ERR(slot))
> -=09=09return PTR_ERR(slot);
> -=09*seq_nr =3D slot->seq_nr;
> -=09return 0;
> +=09ret =3D PTR_ERR_OR_ZERO(slot);
> +=09if (!ret)
> +=09=09*seq_nr =3D slot->seq_nr;
> +
> +=09return ret;
> }
>=20
What version of gcc are you using? I=92m unable to reproduce with gcc 6.1.1=
..
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
DQo+IE9uIEF1ZyAzMSwgMjAxNiwgYXQgMDk6MzcsIEFybmQgQmVyZ21hbm4gPGFybmRAYXJuZGIu
ZGU+IHdyb3RlOg0KPiANCj4gT24gV2VkbmVzZGF5LCBBdWd1c3QgMzEsIDIwMTYgMToxNzo0OCBQ
TSBDRVNUIFRyb25kIE15a2xlYnVzdCB3cm90ZToNCj4+IFdoYXQgdmVyc2lvbiBvZiBnY2MgYXJl
IHlvdSB1c2luZz8gSeKAmW0gdW5hYmxlIHRvIHJlcHJvZHVjZSB3aXRoIGdjYyA2LjEuMS4uDQo+
IA0KPiBUaGlzIGlzIGFsc28gb24gNi4xLjEgZm9yIEFSTS4gTm90ZSB0aGF0IDZlOGQ2NjZlOTI1
MyAoIkRpc2FibGUNCj4gIm1heWJlLXVuaW5pdGlhbGl6ZWQiIHdhcm5pbmcgZ2xvYmFsbHkiKSB0
dXJuZWQgb2ZmIHRob3NlIHdhcm5pbmdzLCBzbw0KPiB1bmxlc3MgeW91IGV4cGxpY2l0bHkgcGFz
cyAtV21heWJlLXVuaW5pdGlhbGl6ZWQgKGUuZy4gYnkgYnVpbGRpbmcgd2l0aA0KPiAibWFrZSBX
PTEiKSwgeW91IHdvbid0IGdldCBpdC4NCj4gDQoNCknigJltIG5vdCBnZXR0aW5nIHRoYXQgZXJy
b3Igb24gZ2NjIDYuMS4xIGZvciB4ODZfNjQgd2l0aCBlaXRoZXIg4oCcbWFrZSBXPTHigJ0gb3Ig
4oCcbWFrZSBXPTLigJ0uDQrigJxtYWtlIFc9M+KAnSBkb2VzIGdpdmVzIHJpc2UgdG8gb25lIHdh
cm5pbmcgaW4gbmZzNF9zbG90X2dldF9zZXFpZDoNCg0KL2hvbWUvdHJvbmRteS9kZXZlbC9rZXJu
ZWwvbGludXgvZnMvbmZzL25mczRzZXNzaW9uLmM6IEluIGZ1bmN0aW9uIOKAmG5mczRfc2xvdF9n
ZXRfc2VxaWTigJk6DQovaG9tZS90cm9uZG15L2RldmVsL2tlcm5lbC9saW51eC9mcy9uZnMvbmZz
NHNlc3Npb24uYzoxODQ6MTA6IHdhcm5pbmc6IGNvbnZlcnNpb24gdG8g4oCYaW504oCZIGZyb20g
4oCYbG9uZyBpbnTigJkgbWF5IGFsdGVyIGl0cyB2YWx1ZSBbLVdjb252ZXJzaW9uXQ0KICAgcmV0
dXJuIFBUUl9FUlIoc2xvdCk7DQogICAgICAgICAgXn5+fn5+fn5+fn5+fg0KDQood2hpY2ggaXMg
YW5vdGhlciBmYWxzZSBwb3NpdGl2ZSkgYnV0IHRoYXTigJlzIGFsbC4uLg0KDQo+IFRoZSByZWFz
b24gSSdtIHN0aWxsIHNlbmRpbmcgdGhlIHBhdGNoZXMgZm9yIHRoaXMgd2FybmluZyBpcyB0aGF0
DQo+IHdlIGRvIGdldCBhIG51bWJlciBvZiB2YWxpZCBvbmVzICh0aGlzIHdhcyB0aGUgb25seSBm
YWxzZSBwb3NpdGl2ZQ0KPiBvdXQgb2YgdGhlIHNldmVuIHN1Y2ggd2FybmluZ3Mgc2luY2UgbGFz
dCB3ZWVrKS4NCg0KVGhlcmUgaXMgYSBaZW4tbGlrZSBxdWFsaXR5IHRvIElTX0VSUigpIHdoZW4g
aXQgY2FzdHMgYSBjb25zdCBwb2ludGVyIHRvIGFuIHVuc2lnbmVkIGxvbmcsIGJhY2sgdG8gYSBu
b24tY29uc3QgcG9pbnRlciwgYW5kIHRoZW4gYmFjayB0byBhbiB1bnNpZ25lZCBsb25nIGJlZm9y
ZSBjb21wYXJpbmcgaXQgdG8gYW5vdGhlciB1bnNpZ25lZCBsb25nIGNhc3QgY29uc3RhbnQgbmVn
YXRpdmUgaW50ZWdlci4gSG93ZXZlciwgSeKAmW0gbm90IHN1cmUgdGhlIEM5OSBzdGFuZGFyZCB3
b3VsZCBhZ3JlZSB0aGF0IGEgcG9zaXRpdmUgdGVzdCByZXN1bHQgaW1wbGllcyB3ZSBjYW4gYXNz
dW1lIHRoYXQgYSBzaW1wbGUgY2FzdCBvZiB0aGUgc2FtZSBwb2ludGVyIHRvIGEgc2lnbmVkIGxv
bmcgd2lsbCByZXN1bHQgaW4gYSBuZWdhdGl2ZSwgbm9uLXplcm8gdmFsdWVkIGVycm5vLg0KDQpJ
IHN1c3BlY3QgdGhhdCBpZiB3ZSByZWFsbHkgd2FudCB0byBmaXggdGhlc2UgZmFsc2UgbmVnYXRp
dmVzLCB3ZSBzaG91bGQgcHJvYmFibHkgYWRkcmVzcyB0aGF0IGlzc3VlLg0KDQpDaGVlcnMNCiAg
VHJvbmQ=
On Wednesday, August 31, 2016 3:02:42 PM CEST Trond Myklebust wrote:
> > On Aug 31, 2016, at 09:37, Arnd Bergmann <[email protected]> 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