[bug report] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[bug report] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dan Carpenter]

Hello Dexuan Cui,

The patch ca9c54d2d6a5: "net: mana: Add a driver for Microsoft Azure
Network Adapter (MANA)" from Apr 16, 2021, leads to the following
static checker warning:

	drivers/net/ethernet/microsoft/mana/hw_channel.c:292 mana_hwc_comp_event()
	warn: 'comp_read' unsigned <= 0

drivers/net/ethernet/microsoft/mana/hw_channel.c
   281  static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
   282  {
   283          struct hwc_rx_oob comp_data = {};
   284          struct gdma_comp *completions;
   285          struct hwc_cq *hwc_cq = ctx;
   286          u32 comp_read, i;
                ^^^^^^^^^^^^^^^^^
These should be int.  I think GCC is encouraging everyone to use u32 but
the u32 type is really a terrible default type to use.  It causes a lot
of bugs.  This is my second or third signedness bug to deal with today.

Normally int is the best type to use.  In the kernel we're mostly using
small numbers where int is fine.  In filesystems etc then we're dealing
with huge numbers so u64 is the right type.  But it's a very narrow band
between 2 and 4 million where u32 is appropriate.

   287  
   288          WARN_ON_ONCE(hwc_cq->gdma_cq != q_self);
   289  
   290          completions = hwc_cq->comp_buf;
   291          comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);

mana_gd_poll_cq() returns int.  It returns -1 on error.

   292          WARN_ON_ONCE(comp_read <= 0 || comp_read > hwc_cq->queue_depth);

comp_read can't be less than zero because it's unsigned but the warning
for "comp_read > hwc_cq->queue_depth" will trigger.

   293  
   294          for (i = 0; i < comp_read; ++i) {
                            ^^^^^^^^^^^^^
If "comp_read" was declared as an int then this loop would be a harmless
no-op but because it's UINT_MAX on error then this will crash the system.

   295                  comp_data = *(struct hwc_rx_oob *)completions[i].cqe_data;
   296  
   297                  if (completions[i].is_sq)
   298                          hwc_cq->tx_event_handler(hwc_cq->tx_event_ctx,
   299                                                  completions[i].wq_num,
   300                                                  &comp_data);
   301                  else
   302                          hwc_cq->rx_event_handler(hwc_cq->rx_event_ctx,
   303                                                  completions[i].wq_num,
   304                                                  &comp_data);
   305          }
   306  
   307          mana_gd_arm_cq(q_self);
   308  }

regards,
dan carpenter

RE: [bug report] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui]

> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Wednesday, April 21, 2021 3:07 AM
> To: Dexuan Cui <decui@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org
> Subject: [bug report] net: mana: Add a driver for Microsoft Azure Network
> Adapter (MANA)
> 
> Hello Dexuan Cui,
> 
> The patch ca9c54d2d6a5: "net: mana: Add a driver for Microsoft Azure
> Network Adapter (MANA)" from Apr 16, 2021, leads to the following
> static checker warning:
> 
> 	drivers/net/ethernet/microsoft/mana/hw_channel.c:292
> mana_hwc_comp_event()
> 	warn: 'comp_read' unsigned <= 0
> 
> drivers/net/ethernet/microsoft/mana/hw_channel.c
>    281  static void mana_hwc_comp_event(void *ctx, struct gdma_queue
> *q_self)
>    282  {
>    283          struct hwc_rx_oob comp_data = {};
>    284          struct gdma_comp *completions;
>    285          struct hwc_cq *hwc_cq = ctx;
>    286          u32 comp_read, i;
>                 ^^^^^^^^^^^^^^^^^
> These should be int.  I think GCC is encouraging everyone to use u32 but
> the u32 type is really a terrible default type to use.  It causes a lot
> of bugs.  This is my second or third signedness bug to deal with today.
> 
> Normally int is the best type to use.  In the kernel we're mostly using
> small numbers where int is fine.  In filesystems etc then we're dealing
> with huge numbers so u64 is the right type.  But it's a very narrow band
> between 2 and 4 million where u32 is appropriate.
> 
>    287
>    288          WARN_ON_ONCE(hwc_cq->gdma_cq != q_self);
>    289
>    290          completions = hwc_cq->comp_buf;
>    291          comp_read = mana_gd_poll_cq(q_self, completions,
> hwc_cq->queue_depth);
> 
> mana_gd_poll_cq() returns int.  It returns -1 on error.
> 
>    292          WARN_ON_ONCE(comp_read <= 0 || comp_read >
> hwc_cq->queue_depth);
> 
> comp_read can't be less than zero because it's unsigned but the warning
> for "comp_read > hwc_cq->queue_depth" will trigger.
> 
>    293
>    294          for (i = 0; i < comp_read; ++i) {
>                             ^^^^^^^^^^^^^
> If "comp_read" was declared as an int then this loop would be a harmless
> no-op but because it's UINT_MAX on error then this will crash the system.
> 
>    295                  comp_data = *(struct hwc_rx_oob
> *)completions[i].cqe_data;
>    296
>    297                  if (completions[i].is_sq)
>    298
> hwc_cq->tx_event_handler(hwc_cq->tx_event_ctx,
>    299
> completions[i].wq_num,
>    300
> &comp_data);
>    301                  else
>    302
> hwc_cq->rx_event_handler(hwc_cq->rx_event_ctx,
>    303
> completions[i].wq_num,
>    304
> &comp_data);
>    305          }
>    306
>    307          mana_gd_arm_cq(q_self);
>    308  }
> 
> regards,
> dan carpenter

Hi Dan,
Thanks for reporting the bug! I'll learn how to use a static tool to avoid
this kind of bugs in future. :-)

I'll post the below patch with your Reported-by:

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 462bc577692a..70bdb9e13fb2 100644
--- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
+++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
@@ -283,7 +283,7 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
        struct hwc_rx_oob comp_data = {};
        struct gdma_comp *completions;
        struct hwc_cq *hwc_cq = ctx;
-       u32 comp_read, i;
+       int comp_read, i;

        WARN_ON_ONCE(hwc_cq->gdma_cq != q_self);

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index a744ca0b6c19..04d067243457 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1061,7 +1061,7 @@ static void mana_process_rx_cqe(struct mana_rxq *rxq, struct mana_cq *cq,
 static void mana_poll_rx_cq(struct mana_cq *cq)
 {
        struct gdma_comp *comp = cq->gdma_comp_buf;
-       u32 comp_read, i;
+       int comp_read, i;

        comp_read = mana_gd_poll_cq(cq->gdma_cq, comp, CQE_P

Thanks,
-- Dexuan

Re: [bug report] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dan Carpenter]

On Thu, Apr 22, 2021 at 08:06:36AM +0000, Dexuan Cui wrote:
> 
> Hi Dan,
> Thanks for reporting the bug! I'll learn how to use a static tool to avoid
> this kind of bugs in future. :-)
> 
> I'll post the below patch with your Reported-by:

Thanks!

regards,
dan carpenter