[PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Geert Uytterhoeven]

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(-)

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) {
-- 
2.7.4

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Arnd Bergmann]

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

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Geert Uytterhoeven]

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

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Arnd Bergmann]

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

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Geert Uytterhoeven]

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

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Arnd Bergmann]

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

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Jason Gunthorpe]

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) {

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Geert Uytterhoeven]

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

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Arnd Bergmann]

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

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Geert Uytterhoeven]

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

Re: [PATCH/RFC] RDMA/iwpm: Fix uninitialized error code in iwpm_send_mapinfo()

[Arnd Bergmann]

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