diff mbox

[bug,report] IB/uverbs: clean up INIT_UDATA_BUF_OR_NULL usage

Message ID CAK8P3a1t6zyQXtfov6HguOb9V7E4i54pK8+y8BjPqGqvsc9mpQ@mail.gmail.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Arnd Bergmann Oct. 2, 2017, 11 a.m. UTC
On Mon, Oct 2, 2017 at 12:13 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Arnd Bergmann,
>
> The patch 12f727721eee: "IB/uverbs: clean up INIT_UDATA_BUF_OR_NULL
> usage" from Sep 6, 2017, leads to the following static checker
> warning:
>
>         drivers/infiniband/core/uverbs_std_types.c:249 create_udata()
>         error: uninitialized symbol 'inbuf'.
>
> drivers/infiniband/core/uverbs_std_types.c
>    222  static void create_udata(struct uverbs_attr_bundle *ctx,
>    223                           struct ib_udata *udata)
>    224  {
>    225          /*
>    226           * This is for ease of conversion. The purpose is to convert all drivers
>    227           * to use uverbs_attr_bundle instead of ib_udata.
>    228           * Assume attr == 0 is input and attr == 1 is output.
>    229           */
>    230          void __user *inbuf;
>    231          size_t inbuf_len = 0;
>    232          void __user *outbuf;
>    233          size_t outbuf_len = 0;
>    234          const struct uverbs_attr *uhw_in =
>    235                  uverbs_attr_get(ctx, UVERBS_UHW_IN);
>    236          const struct uverbs_attr *uhw_out =
>    237                  uverbs_attr_get(ctx, UVERBS_UHW_OUT);
>    238
>    239          if (!IS_ERR(uhw_in)) {
>    240                  inbuf = uhw_in->ptr_attr.ptr;
>    241                  inbuf_len = uhw_in->ptr_attr.len;
>    242          }
>    243
>    244          if (!IS_ERR(uhw_out)) {
>    245                  outbuf = uhw_out->ptr_attr.ptr;
>    246                  outbuf_len = uhw_out->ptr_attr.len;
>    247          }
>    248
>    249          ib_uverbs_init_udata_buf_or_null(udata, inbuf, outbuf, inbuf_len,
>    250                                           outbuf_len);
>
> In the original code, this was a macro so smatch understood that we
> don't use inbuf and outbuf if they aren't initialized.  Now it's a
> function and smatch generates a warning if you pass uninitialized
> variables to a function.
>
> It's not a bug, but it's really ugly.

I see. Should we maybe open-code the ib_uverbs_init_udata_buf_or_null()
call in this function?


       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

Comments

Doug Ledford Nov. 10, 2017, 5:58 p.m. UTC | #1
On Mon, 2017-10-02 at 13:00 +0200, Arnd Bergmann wrote:
> On Mon, Oct 2, 2017 at 12:13 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Hello Arnd Bergmann,
> > 
> > The patch 12f727721eee: "IB/uverbs: clean up INIT_UDATA_BUF_OR_NULL
> > usage" from Sep 6, 2017, leads to the following static checker
> > warning:
> > 
> >         drivers/infiniband/core/uverbs_std_types.c:249 create_udata()
> >         error: uninitialized symbol 'inbuf'.
> > 
> > drivers/infiniband/core/uverbs_std_types.c
> >    222  static void create_udata(struct uverbs_attr_bundle *ctx,
> >    223                           struct ib_udata *udata)
> >    224  {
> >    225          /*
> >    226           * This is for ease of conversion. The purpose is to convert all drivers
> >    227           * to use uverbs_attr_bundle instead of ib_udata.
> >    228           * Assume attr == 0 is input and attr == 1 is output.
> >    229           */
> >    230          void __user *inbuf;
> >    231          size_t inbuf_len = 0;
> >    232          void __user *outbuf;
> >    233          size_t outbuf_len = 0;
> >    234          const struct uverbs_attr *uhw_in =
> >    235                  uverbs_attr_get(ctx, UVERBS_UHW_IN);
> >    236          const struct uverbs_attr *uhw_out =
> >    237                  uverbs_attr_get(ctx, UVERBS_UHW_OUT);
> >    238
> >    239          if (!IS_ERR(uhw_in)) {
> >    240                  inbuf = uhw_in->ptr_attr.ptr;
> >    241                  inbuf_len = uhw_in->ptr_attr.len;
> >    242          }
> >    243
> >    244          if (!IS_ERR(uhw_out)) {
> >    245                  outbuf = uhw_out->ptr_attr.ptr;
> >    246                  outbuf_len = uhw_out->ptr_attr.len;
> >    247          }
> >    248
> >    249          ib_uverbs_init_udata_buf_or_null(udata, inbuf, outbuf, inbuf_len,
> >    250                                           outbuf_len);
> > 
> > In the original code, this was a macro so smatch understood that we
> > don't use inbuf and outbuf if they aren't initialized.  Now it's a
> > function and smatch generates a warning if you pass uninitialized
> > variables to a function.
> > 
> > It's not a bug, but it's really ugly.
> 
> I see. Should we maybe open-code the ib_uverbs_init_udata_buf_or_null()
> call in this function?
> 
> diff --git a/drivers/infiniband/core/uverbs_std_types.c
> b/drivers/infiniband/core/uverbs_std_types.c
> index b095bce7f238..039802ae7332 100644
> --- a/drivers/infiniband/core/uverbs_std_types.c
> +++ b/drivers/infiniband/core/uverbs_std_types.c
> @@ -227,27 +227,26 @@ static void create_udata(struct uverbs_attr_bundle *ctx,
>          * to use uverbs_attr_bundle instead of ib_udata.
>          * Assume attr == 0 is input and attr == 1 is output.
>          */
> -       void __user *inbuf;
> -       size_t inbuf_len = 0;
> -       void __user *outbuf;
> -       size_t outbuf_len = 0;
>         const struct uverbs_attr *uhw_in =
>                 uverbs_attr_get(ctx, UVERBS_UHW_IN);
>         const struct uverbs_attr *uhw_out =
>                 uverbs_attr_get(ctx, UVERBS_UHW_OUT);
> 
>         if (!IS_ERR(uhw_in)) {
> -               inbuf = uhw_in->ptr_attr.ptr;
> -               inbuf_len = uhw_in->ptr_attr.len;
> +               udata->inbuf = uhw_in->ptr_attr.ptr;
> +               udata->inbuf_len = uhw_in->ptr_attr.len;
> +       } else {
> +               udata->inbuf = NULL;
> +               udata->inbuf_len = 0;
>         }
> 
>         if (!IS_ERR(uhw_out)) {
> -               outbuf = uhw_out->ptr_attr.ptr;
> -               outbuf_len = uhw_out->ptr_attr.len;
> +               udata->outbuf = uhw_out->ptr_attr.ptr;
> +               udata->outbuf_len = uhw_out->ptr_attr.len;
> +       } else {
> +               udata->outbuf = NULL;
> +               udata->outbuf_len = 0;
>         }
> -
> -       ib_uverbs_init_udata_buf_or_null(udata, inbuf, outbuf, inbuf_len,
> -                                        outbuf_len);
>  }
> 
>  static int uverbs_create_cq_handler(struct ib_device *ib_dev,
> 
>        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

Arnd, can you send this as a proper patch with attribution and a Signed-
off-by: please.
Arnd Bergmann Nov. 10, 2017, 10:12 p.m. UTC | #2
On Fri, Nov 10, 2017 at 6:58 PM, Doug Ledford <dledford@redhat.com> wrote:
> On Mon, 2017-10-02 at 13:00 +0200, Arnd Bergmann wrote:
>> On Mon, Oct 2, 2017 at 12:13 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>
>>  static int uverbs_create_cq_handler(struct ib_device *ib_dev,
>>
> Arnd, can you send this as a proper patch with attribution and a Signed-
> off-by: please.
>

Submitted now as "infiniband: avoid uninitialized variable warning in
create_udata",
Message-Id: <20171110221053.3549147-1-arnd@arndb.de>.

       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 mbox

Patch

diff --git a/drivers/infiniband/core/uverbs_std_types.c
b/drivers/infiniband/core/uverbs_std_types.c
index b095bce7f238..039802ae7332 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -227,27 +227,26 @@  static void create_udata(struct uverbs_attr_bundle *ctx,
         * to use uverbs_attr_bundle instead of ib_udata.
         * Assume attr == 0 is input and attr == 1 is output.
         */
-       void __user *inbuf;
-       size_t inbuf_len = 0;
-       void __user *outbuf;
-       size_t outbuf_len = 0;
        const struct uverbs_attr *uhw_in =
                uverbs_attr_get(ctx, UVERBS_UHW_IN);
        const struct uverbs_attr *uhw_out =
                uverbs_attr_get(ctx, UVERBS_UHW_OUT);

        if (!IS_ERR(uhw_in)) {
-               inbuf = uhw_in->ptr_attr.ptr;
-               inbuf_len = uhw_in->ptr_attr.len;
+               udata->inbuf = uhw_in->ptr_attr.ptr;
+               udata->inbuf_len = uhw_in->ptr_attr.len;
+       } else {
+               udata->inbuf = NULL;
+               udata->inbuf_len = 0;
        }

        if (!IS_ERR(uhw_out)) {
-               outbuf = uhw_out->ptr_attr.ptr;
-               outbuf_len = uhw_out->ptr_attr.len;
+               udata->outbuf = uhw_out->ptr_attr.ptr;
+               udata->outbuf_len = uhw_out->ptr_attr.len;
+       } else {
+               udata->outbuf = NULL;
+               udata->outbuf_len = 0;
        }
-
-       ib_uverbs_init_udata_buf_or_null(udata, inbuf, outbuf, inbuf_len,
-                                        outbuf_len);
 }

 static int uverbs_create_cq_handler(struct ib_device *ib_dev,