| Message ID | 1510827964-11100-1-git-send-email-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Nov 16, 2017 at 11:26 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > With gcc-4.1.2: > > drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: > drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function > > Indeed, if nl_client is not found in any of the scanned has buckets, ret > will be used uninitialized. > > Preinitialize ret to zero to fix this. > > Fixes: 30dc5e63d6a5ad24 ("RDMA/core: Add support for iWARP Port Mapper user space service") > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > RFC as I have no idea if this can ever happen, and if yes, what's the > correct behavior to handle it: > - return 0, > - return an error code, > - don't send anything, > - anything else? This looks like a reasonable warning. I don't see on my box with any compiler version. Do you have a configuration I can use to reproduce it, I'd just like to see out of curiosity which other compilers report it. Looking at one caller (iwpm_mapping_info_cb), it seems that we try to make sure that nl_client is valid first by calling iwpm_valid_client(), and returning -EINVAL otherwise, so that seems like an appropriate return code if any caller forgets to test for iwpm_valid_client() first. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On Thu, Nov 16, 2017 at 11:50 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Nov 16, 2017 at 11:26 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> With gcc-4.1.2: >> >> drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: >> drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function >> >> Indeed, if nl_client is not found in any of the scanned has buckets, ret >> will be used uninitialized. >> >> Preinitialize ret to zero to fix this. >> >> Fixes: 30dc5e63d6a5ad24 ("RDMA/core: Add support for iWARP Port Mapper user space service") >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> >> --- >> RFC as I have no idea if this can ever happen, and if yes, what's the >> correct behavior to handle it: >> - return 0, >> - return an error code, >> - don't send anything, >> - anything else? > > This looks like a reasonable warning. I don't see on my box with any compiler > version. Do you have a configuration I can use to reproduce it, I'd just like > to see out of curiosity which other compilers report it. m68k/allmodconfig Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 16, 2017 at 11:50:48AM +0100, Arnd Bergmann wrote: > On Thu, Nov 16, 2017 at 11:26 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > With gcc-4.1.2: > > > > drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: > > drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function > > > > Indeed, if nl_client is not found in any of the scanned has buckets, ret > > will be used uninitialized. > > > > Preinitialize ret to zero to fix this. > > > > Fixes: 30dc5e63d6a5ad24 ("RDMA/core: Add support for iWARP Port Mapper user space service") > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- > > RFC as I have no idea if this can ever happen, and if yes, what's the > > correct behavior to handle it: > > - return 0, > > - return an error code, > > - don't send anything, > > - anything else? > > This looks like a reasonable warning. I don't see on my box with any compiler > version. Do you have a configuration I can use to reproduce it, I'd just like > to see out of curiosity which other compilers report it. > > Looking at one caller (iwpm_mapping_info_cb), it seems that we try to make > sure that nl_client is valid first by calling iwpm_valid_client(), and returning > -EINVAL otherwise, so that seems like an appropriate return code if any > caller forgets to test for iwpm_valid_client() first. The checks of nl_client are leftovers from RDMA core netlink refactoring work. The call to iwpm_mapping_info_cb() can occur only if core checked and identified this client as safe. Thanks > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 16, 2017 at 12:05 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Arnd, > > On Thu, Nov 16, 2017 at 11:50 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Thu, Nov 16, 2017 at 11:26 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> With gcc-4.1.2: >>> >>> drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: >>> drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function >>> >>> Indeed, if nl_client is not found in any of the scanned has buckets, ret >>> will be used uninitialized. >>> >>> Preinitialize ret to zero to fix this. >>> >>> Fixes: 30dc5e63d6a5ad24 ("RDMA/core: Add support for iWARP Port Mapper user space service") >>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> >>> --- >>> RFC as I have no idea if this can ever happen, and if yes, what's the >>> correct behavior to handle it: >>> - return 0, >>> - return an error code, >>> - don't send anything, >>> - anything else? >> >> This looks like a reasonable warning. I don't see on my box with any compiler >> version. Do you have a configuration I can use to reproduce it, I'd just like >> to see out of curiosity which other compilers report it. > > m68k/allmodconfig I see my problem now, my randconfig test series has a patch I never sent out, see https://pastebin.com/ZJDHP7g4 ;-) With plan linux-next, I see the warning on x86 with gcc-4.3 and older, but not on newer gcc versions or clang (I tried them all). I also see that x86 builds broke again in recent kernels with gcc-4.2 and older unless you revert ec1e1b610917 ("objtool: Prevent GCC from merging annotate_unreachable(), take 2"). Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On Thu, Nov 16, 2017 at 12:32 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Nov 16, 2017 at 12:05 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > I see my problem now, my randconfig test series has a patch I never sent > out, see https://pastebin.com/ZJDHP7g4 ;-) I was already wondering why I suddenly started seeing many non-false positives reaching Linus' tree ;-) Lately you've been catching all of them in -next, before they had a chance to appear in upstream. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 16, 2017 at 1:03 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Arnd, > > On Thu, Nov 16, 2017 at 12:32 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Thu, Nov 16, 2017 at 12:05 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >> I see my problem now, my randconfig test series has a patch I never sent >> out, see https://pastebin.com/ZJDHP7g4 ;-) > > I was already wondering why I suddenly started seeing many non-false > positives reaching Linus' tree ;-) > Lately you've been catching all of them in -next, before they had a chance > to appear in upstream. The patch is only for older compilers that I don't regularly test with, it should not have made a big difference here. I was going to submit the patch but took a look at some of the remaining warnings and came across the bug leading to https://lkml.org/lkml/2017/9/15/384, so I ended up not sending it, but still had it in my queue. Not sure what happened to that patch though, I think it was in linux-next at some point (which means I dropped it during rebasing), but it's not there now. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 16, 2017 at 11:26:04AM +0100, Geert Uytterhoeven wrote: > With gcc-4.1.2: > > drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: > drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function > > Indeed, if nl_client is not found in any of the scanned has buckets, ret > will be used uninitialized. > > Preinitialize ret to zero to fix this. Did we come to a conclusion if we should apply this to the RMDA tree? The patch was marked RFC.. Thanks, Jason > Fixes: 30dc5e63d6a5ad24 ("RDMA/core: Add support for iWARP Port Mapper user space service") > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > RFC as I have no idea if this can ever happen, and if yes, what's the > correct behavior to handle it: > - return 0, > - return an error code, > - don't send anything, > - anything else? > drivers/infiniband/core/iwpm_util.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/iwpm_util.c b/drivers/infiniband/core/iwpm_util.c > index 3c4faadb8cddd7fd..eb000b540495acd1 100644 > +++ b/drivers/infiniband/core/iwpm_util.c > @@ -644,7 +644,7 @@ int iwpm_send_mapinfo(u8 nl_client, int iwpm_pid) > int i = 0, nlmsg_bytes = 0; > unsigned long flags; > const char *err_str = ""; > - int ret; > + int ret = 0; > > skb = dev_alloc_skb(NLMSG_GOODSIZE); > if (!skb) { -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jason, On Wed, Nov 29, 2017 at 12:08 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Thu, Nov 16, 2017 at 11:26:04AM +0100, Geert Uytterhoeven wrote: >> With gcc-4.1.2: >> >> drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: >> drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function >> >> Indeed, if nl_client is not found in any of the scanned has buckets, ret >> will be used uninitialized. >> >> Preinitialize ret to zero to fix this. > > Did we come to a conclusion if we should apply this to the RMDA tree? The > patch was marked RFC.. So far no one commented on what's the correct behavior in case of failure, which was the actual reason for the RFC. >> Fixes: 30dc5e63d6a5ad24 ("RDMA/core: Add support for iWARP Port Mapper user space service") >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> >> RFC as I have no idea if this can ever happen, and if yes, what's the >> correct behavior to handle it: >> - return 0, >> - return an error code, >> - don't send anything, >> - anything else? >> drivers/infiniband/core/iwpm_util.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/core/iwpm_util.c b/drivers/infiniband/core/iwpm_util.c >> index 3c4faadb8cddd7fd..eb000b540495acd1 100644 >> +++ b/drivers/infiniband/core/iwpm_util.c >> @@ -644,7 +644,7 @@ int iwpm_send_mapinfo(u8 nl_client, int iwpm_pid) >> int i = 0, nlmsg_bytes = 0; >> unsigned long flags; >> const char *err_str = ""; >> - int ret; >> + int ret = 0; >> >> skb = dev_alloc_skb(NLMSG_GOODSIZE); >> if (!skb) { Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 29, 2017 at 9:10 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Jason, > > On Wed, Nov 29, 2017 at 12:08 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: >> On Thu, Nov 16, 2017 at 11:26:04AM +0100, Geert Uytterhoeven wrote: >>> With gcc-4.1.2: >>> >>> drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: >>> drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function >>> >>> Indeed, if nl_client is not found in any of the scanned has buckets, ret >>> will be used uninitialized. >>> >>> Preinitialize ret to zero to fix this. >> >> Did we come to a conclusion if we should apply this to the RMDA tree? The >> patch was marked RFC.. > > So far no one commented on what's the correct behavior in case of failure, > which was the actual reason for the RFC. As I said above, I think initializing to -EINVAL would be better than 0 here, but initializing 'ret' at declaration time is appropriate here (though I normally try to avoid doing so, see https://rusty.ozlabs.org/?p=232) Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On Wed, Nov 29, 2017 at 9:20 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Nov 29, 2017 at 9:10 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Wed, Nov 29, 2017 at 12:08 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: >>> On Thu, Nov 16, 2017 at 11:26:04AM +0100, Geert Uytterhoeven wrote: >>>> With gcc-4.1.2: >>>> >>>> drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: >>>> drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function >>>> >>>> Indeed, if nl_client is not found in any of the scanned has buckets, ret >>>> will be used uninitialized. >>>> >>>> Preinitialize ret to zero to fix this. >>> >>> Did we come to a conclusion if we should apply this to the RMDA tree? The >>> patch was marked RFC.. >> >> So far no one commented on what's the correct behavior in case of failure, >> which was the actual reason for the RFC. > > As I said above, I think initializing to -EINVAL would be better than 0 here, Sorry, I misread your comment as the -EINVAL being part of another function. > but initializing 'ret' at declaration time is appropriate here (though > I normally > try to avoid doing so, see https://rusty.ozlabs.org/?p=232) +1, but if loops are involved, you have not much choice. I could move the preinitialization to just before the loop? Would you like that? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 29, 2017 at 9:24 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Arnd, > > On Wed, Nov 29, 2017 at 9:20 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Wed, Nov 29, 2017 at 9:10 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Wed, Nov 29, 2017 at 12:08 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: >>>> On Thu, Nov 16, 2017 at 11:26:04AM +0100, Geert Uytterhoeven wrote: >>>>> With gcc-4.1.2: >>>>> >>>>> drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: >>>>> drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function >>>>> >>>>> Indeed, if nl_client is not found in any of the scanned has buckets, ret >>>>> will be used uninitialized. >>>>> >>>>> Preinitialize ret to zero to fix this. >>>> >>>> Did we come to a conclusion if we should apply this to the RMDA tree? The >>>> patch was marked RFC.. >>> >>> So far no one commented on what's the correct behavior in case of failure, >>> which was the actual reason for the RFC. >> >> As I said above, I think initializing to -EINVAL would be better than 0 here, > > Sorry, I misread your comment as the -EINVAL being part of another function. > >> but initializing 'ret' at declaration time is appropriate here (though >> I normally >> try to avoid doing so, see https://rusty.ozlabs.org/?p=232) > > +1, but if loops are involved, you have not much choice. > I could move the preinitialization to just before the loop? > Would you like that? I don't think it makes much difference in this particular case, since the function is mostly just that loop, it's fine either way. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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/drivers/infiniband/core/iwpm_util.c b/drivers/infiniband/core/iwpm_util.c index 3c4faadb8cddd7fd..eb000b540495acd1 100644 --- a/drivers/infiniband/core/iwpm_util.c +++ b/drivers/infiniband/core/iwpm_util.c @@ -644,7 +644,7 @@ int iwpm_send_mapinfo(u8 nl_client, int iwpm_pid) int i = 0, nlmsg_bytes = 0; unsigned long flags; const char *err_str = ""; - int ret; + int ret = 0; skb = dev_alloc_skb(NLMSG_GOODSIZE); if (!skb) {
With gcc-4.1.2: drivers/infiniband/core/iwpm_util.c: In function ‘iwpm_send_mapinfo’: drivers/infiniband/core/iwpm_util.c:647: warning: ‘ret’ may be used uninitialized in this function Indeed, if nl_client is not found in any of the scanned has buckets, ret will be used uninitialized. Preinitialize ret to zero to fix this. Fixes: 30dc5e63d6a5ad24 ("RDMA/core: Add support for iWARP Port Mapper user space service") Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> --- RFC as I have no idea if this can ever happen, and if yes, what's the correct behavior to handle it: - return 0, - return an error code, - don't send anything, - anything else? --- drivers/infiniband/core/iwpm_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)