> From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Sent: Wednesday, April 21, 2021 3:07 AM
> To: Dexuan Cui <decui@xxxxxxxxxxxxx>
> Cc: linux-hyperv@xxxxxxxxxxxxxxx
> 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
