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

[Dexuan Cui <decui@microsoft.com>]

> 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