| Message ID | 20230726072114.51964-2-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/5] wifi: mwifiex: fix memory leak in mwifiex_histogram_read() | expand |
On Wed, Jul 26, 2023 at 10:20:53AM +0300, Dmitry Antipov wrote: > Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and > 'mwifiex_process_uap_txpd()'. In case of insufficient > headrom, issue warning and return NULL, which should be > gracefully handled in 'mwifiex_process_tx()'. Also mark > error handling branches with 'unlikely()' and simplify > error messages (there is no need to use formatted output > to print the value which is known to be zero). > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> You already sent a version of this patch, and I already gave you feedback: https://lore.kernel.org/all/ZJ3lyIQy7GPbA9YL@google.com/ You didn't respond to or integrate that feedback. Brian
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c index 13c0e67ededf..5995a81f1ce9 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c @@ -39,14 +39,17 @@ void *mwifiex_process_sta_txpd(struct mwifiex_private *priv, u16 pkt_type, pkt_offset; int hroom = adapter->intf_hdr_len; - if (!skb->len) { - mwifiex_dbg(adapter, ERROR, - "Tx: bad packet length: %d\n", skb->len); + if (unlikely(!skb->len)) { + mwifiex_dbg(adapter, ERROR, "Tx: empty skb\n"); tx_info->status_code = -1; return skb->data; } - - BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN); + if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) { + mwifiex_dbg(adapter, ERROR, + "Tx: insufficient skb headroom: %u < %u\n", + skb_headroom(skb), MWIFIEX_MIN_DATA_HEADER_LEN); + return NULL; + } pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0; diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c index e495f7eaea03..ff953e8e7413 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c @@ -452,14 +452,17 @@ void *mwifiex_process_uap_txpd(struct mwifiex_private *priv, u16 pkt_type, pkt_offset; int hroom = adapter->intf_hdr_len; - if (!skb->len) { - mwifiex_dbg(adapter, ERROR, - "Tx: bad packet length: %d\n", skb->len); + if (unlikely(!skb->len)) { + mwifiex_dbg(adapter, ERROR, "Tx: empty skb\n"); tx_info->status_code = -1; return skb->data; } - - BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN); + if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) { + mwifiex_dbg(adapter, ERROR, + "Tx: insufficient skb headroom: %u < %u\n", + skb_headroom(skb), MWIFIEX_MIN_DATA_HEADER_LEN); + return NULL; + } pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and 'mwifiex_process_uap_txpd()'. In case of insufficient headrom, issue warning and return NULL, which should be gracefully handled in 'mwifiex_process_tx()'. Also mark error handling branches with 'unlikely()' and simplify error messages (there is no need to use formatted output to print the value which is known to be zero). Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- drivers/net/wireless/marvell/mwifiex/sta_tx.c | 13 ++++++++----- drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 13 ++++++++----- 2 files changed, 16 insertions(+), 10 deletions(-)