[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)
@ 2021-04-21 10:06 Dan Carpenter
  2021-04-22  8:06 ` Dexuan Cui
  0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-04-21 10:06 UTC (permalink / raw)
  To: decui; +Cc: linux-hyperv

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

^ permalink raw reply	[flat|nested] 3+ messages in thread