| Message ID | 20230629085115.180499-3-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/3,v4] wifi: mwifiex: prefer strscpy() over strlcpy() | expand |
On Thu, Jun 29, 2023 at 11:51:02AM +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 Hi Dimitry, a minor nit courtesy of checkpatch.pl --codespell: headrom -> headroom ...
On Thu, Jun 29, 2023 at 11:51:02AM +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 adjust > format specifiers to match actual 'unsigned int' type. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > v4: initial version to match series > --- > drivers/net/wireless/marvell/mwifiex/sta_tx.c | 13 +++++++++---- > drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 13 +++++++++---- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c > index 13c0e67ededf..d43f6ec1ad37 100644 > --- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c > +++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c > @@ -39,14 +39,19 @@ void *mwifiex_process_sta_txpd(struct mwifiex_private *priv, > u16 pkt_type, pkt_offset; > int hroom = adapter->intf_hdr_len; > > - if (!skb->len) { > + if (unlikely(!skb->len)) { > mwifiex_dbg(adapter, ERROR, > - "Tx: bad packet length: %d\n", skb->len); > + "Tx: bad packet length: %u\n", skb->len); > 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\n", > + skb_headroom(skb)); > + tx_info->status_code = -1; > + return NULL; I'm not sure why this return (NULL) should be different than the one for skb->len==0 (skb->data). mwifiex_process_tx() has...weird handling for both. For NULL, we fall into a default (ret==-1) case where the error message is wrong ("mwifiex_write_data_async failed: ..."). For non-NULL skb->data, we still try to queue or transmit the skb...which seems wrong. I think they should both be returning NULL, and mwifiex_process_tx() should improve its error handling to more explicitly handle that case, instead of printing the wrong error message. (Now, I expect neither failure cases are actually exercised in practice, which makes most of this moot...) I'm also not sure why this is part of the same series as the others. Brian > + } > > 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..b27266742795 100644 > --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c > @@ -452,14 +452,19 @@ void *mwifiex_process_uap_txpd(struct mwifiex_private *priv, > u16 pkt_type, pkt_offset; > int hroom = adapter->intf_hdr_len; > > - if (!skb->len) { > + if (unlikely(!skb->len)) { > mwifiex_dbg(adapter, ERROR, > - "Tx: bad packet length: %d\n", skb->len); > + "Tx: bad packet length: %u\n", skb->len); > 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\n", > + skb_headroom(skb)); > + tx_info->status_code = -1; > + return NULL; > + } > > pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0; > > -- > 2.41.0 >
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c index 13c0e67ededf..d43f6ec1ad37 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c @@ -39,14 +39,19 @@ void *mwifiex_process_sta_txpd(struct mwifiex_private *priv, u16 pkt_type, pkt_offset; int hroom = adapter->intf_hdr_len; - if (!skb->len) { + if (unlikely(!skb->len)) { mwifiex_dbg(adapter, ERROR, - "Tx: bad packet length: %d\n", skb->len); + "Tx: bad packet length: %u\n", skb->len); 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\n", + skb_headroom(skb)); + tx_info->status_code = -1; + 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..b27266742795 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c @@ -452,14 +452,19 @@ void *mwifiex_process_uap_txpd(struct mwifiex_private *priv, u16 pkt_type, pkt_offset; int hroom = adapter->intf_hdr_len; - if (!skb->len) { + if (unlikely(!skb->len)) { mwifiex_dbg(adapter, ERROR, - "Tx: bad packet length: %d\n", skb->len); + "Tx: bad packet length: %u\n", skb->len); 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\n", + skb_headroom(skb)); + tx_info->status_code = -1; + 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 adjust format specifiers to match actual 'unsigned int' type. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- v4: initial version to match series --- drivers/net/wireless/marvell/mwifiex/sta_tx.c | 13 +++++++++---- drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 13 +++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-)