wireless: mwifiex: initial commit for Marvell mwifiex driver

[ This is old, but maybe the driver is still really actively maintained
so maybe someone knows the answer. - dan ]

Hello Marvell Developers,

The patch 5e6e3a92b9a4: "wireless: mwifiex: initial commit for Marvell mwifiex driver" from Mar 21, 2011, leads to the following static checker warning:

	drivers/net/wireless/marvell/mwifiex/11n.c:505 mwifiex_11n_delete_tx_ba_stream_tbl_entry()
	error: we previously assumed 'tx_ba_tsr_tbl' could be null (see line 498)

drivers/net/wireless/marvell/mwifiex/11n.c
472 /*

   473   * This function checks if the given pointer is valid entry of
   474   * Tx BA Stream table.
   475   */
   476  static int mwifiex_is_tx_ba_stream_ptr_valid(struct mwifiex_private *priv,
   477                                  struct mwifiex_tx_ba_stream_tbl *tx_tbl_ptr)
                                                                         ^^^^^^^^^^

This is always NULL.

478 {
479 struct mwifiex_tx_ba_stream_tbl *tx_ba_tsr_tbl;
480
481 list_for_each_entry(tx_ba_tsr_tbl, &priv->tx_ba_stream_tbl_ptr, list) {
482 if (tx_ba_tsr_tbl == tx_tbl_ptr)

                            ^^^^^^^^^^^^^

tx_ba_tsr_tbl is the list iterator, which is never NULL so this will never return true.

483 return true;
484 }
485
486 return false;
487 }
488
489 /*

   490   * This function deletes the given entry in Tx BA Stream table.
   491   *
   492   * The function also performs a validity check on the supplied
   493   * pointer before trying to delete.
   494   */
   495  void mwifiex_11n_delete_tx_ba_stream_tbl_entry(struct mwifiex_private *priv,
   496                                  struct mwifiex_tx_ba_stream_tbl *tx_ba_tsr_tbl)
   497  {
   498          if (!tx_ba_tsr_tbl &&
                     ^^^^^^^^^^^^^

Check for NULL

499 mwifiex_is_tx_ba_stream_ptr_valid(priv, tx_ba_tsr_tbl))

                                                            ^^^^^^^^^^^^^

Which is passed to here. So maybe the NULL check is reversed?

500 return;
501
502 mwifiex_dbg(priv->adapter, INFO,
503 "info: tx_ba_tsr_tbl %p\n", tx_ba_tsr_tbl);
504

   505          list_del(&tx_ba_tsr_tbl->list);
                          ^^^^^^^^^^^^^^^^^^^

Unchecked NULL dereference

   506  
   507          kfree(tx_ba_tsr_tbl);
   508  }

regards,
dan carpenter

I'm not sure what your definition of "active" is :) I also don't claim to know "the" answer, but I'll provide one:

Maybe, but it also looks like this validity check has always been dead code, and only serves a redundant purpose (to check that the list entry is *really* part of the list that we're trying to delete from?). That sounds like we should just delete mwifiex_is_tx_ba_stream_ptr_valid().

Brian

Hi Dan/Brian

I think, it should have been like below:

if (!tx_ba_tsr_tbl || !mwifiex_is_tx_ba_stream_ptr_valid(priv, tx_ba_tsr_tbl)) . . .

Regards,
Ganapathi

Ah, of course.

But I think my point still stands:
It's currently dead code, and even if it were correctly-written, it would be redundant and unnecessary. So we should just remove it.

Brian