| Message ID | 20201009171307.864608-4-Jerome.Pouiller@silabs.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | staging: wfx: fix issues reported by Smatch | expand |
Jerome Pouiller <Jerome.Pouiller@silabs.com> writes: > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > Smatch complains: > > drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif' > drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif' > > Indeed, if the vif id returned by the device does not exist anymore, > wdev_to_wvif() could return NULL. > > In add, the error is not handled uniformly in the code, sometime a > WARN() is displayed but code continue, sometime a dev_warn() is > displayed, sometime it is just not tested, ... > > This patch standardize that. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > --- > drivers/staging/wfx/data_tx.c | 5 ++++- > drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++---------- > drivers/staging/wfx/sta.c | 4 ++++ > 3 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c > index b4d5dd3d2d23..8db0be08daf8 100644 > --- a/drivers/staging/wfx/data_tx.c > +++ b/drivers/staging/wfx/data_tx.c > @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb) > sizeof(struct hif_req_tx) + > req->fc_offset; > > - WARN_ON(!wvif); > + if (!wvif) { > + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__); > + return; > + } I'm not really a fan of using function names in warning or error messages as it clutters the log. In debug messages I think they are ok.
On Friday 9 October 2020 20:52:47 CEST Kalle Valo wrote: > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes: > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > Smatch complains: > > > > drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif' > > drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif' > > > > Indeed, if the vif id returned by the device does not exist anymore, > > wdev_to_wvif() could return NULL. > > > > In add, the error is not handled uniformly in the code, sometime a > > WARN() is displayed but code continue, sometime a dev_warn() is > > displayed, sometime it is just not tested, ... > > > > This patch standardize that. > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > --- > > drivers/staging/wfx/data_tx.c | 5 ++++- > > drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++---------- > > drivers/staging/wfx/sta.c | 4 ++++ > > 3 files changed, 32 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c > > index b4d5dd3d2d23..8db0be08daf8 100644 > > --- a/drivers/staging/wfx/data_tx.c > > +++ b/drivers/staging/wfx/data_tx.c > > @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb) > > sizeof(struct hif_req_tx) + > > req->fc_offset; > > > > - WARN_ON(!wvif); > > + if (!wvif) { > > + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__); > > + return; > > + } > > I'm not really a fan of using function names in warning or error > messages as it clutters the log. In debug messages I think they are ok. In the initial code, I used WARN() that far more clutters the log (I have stated that a backtrace won't provide any useful information, so pr_warn() was better suited). In add, in my mind, these warnings are debug messages. If they appears, the user should probably report a bug. Finally, in this patch, I use the same message several times (ok, not this particular one). So the function name is a way to differentiate them.
On Sat, Oct 10, 2020 at 02:22:13PM +0200, Jérôme Pouiller wrote: > On Friday 9 October 2020 20:52:47 CEST Kalle Valo wrote: > > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes: > > > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > Smatch complains: > > > > > > drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif' > > > drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif' > > > > > > Indeed, if the vif id returned by the device does not exist anymore, > > > wdev_to_wvif() could return NULL. > > > > > > In add, the error is not handled uniformly in the code, sometime a > > > WARN() is displayed but code continue, sometime a dev_warn() is > > > displayed, sometime it is just not tested, ... > > > > > > This patch standardize that. > > > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > --- > > > drivers/staging/wfx/data_tx.c | 5 ++++- > > > drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++---------- > > > drivers/staging/wfx/sta.c | 4 ++++ > > > 3 files changed, 32 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c > > > index b4d5dd3d2d23..8db0be08daf8 100644 > > > --- a/drivers/staging/wfx/data_tx.c > > > +++ b/drivers/staging/wfx/data_tx.c > > > @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb) > > > sizeof(struct hif_req_tx) + > > > req->fc_offset; > > > > > > - WARN_ON(!wvif); > > > + if (!wvif) { > > > + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__); > > > + return; > > > + } > > > > I'm not really a fan of using function names in warning or error > > messages as it clutters the log. In debug messages I think they are ok. > > In the initial code, I used WARN() that far more clutters the log (I > have stated that a backtrace won't provide any useful information, so > pr_warn() was better suited). > > In add, in my mind, these warnings are debug messages. If they appears, > the user should probably report a bug. > > Finally, in this patch, I use the same message several times (ok, not > this particular one). So the function name is a way to differentiate > them. You should use dev_*() for these, that way you can properly determine the exact device as well. thanks, greg k-h
On Saturday 10 October 2020 14:40:34 CEST Greg Kroah-Hartman wrote: > On Sat, Oct 10, 2020 at 02:22:13PM +0200, Jérôme Pouiller wrote: > > On Friday 9 October 2020 20:52:47 CEST Kalle Valo wrote: > > > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes: > > > > > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > > > Smatch complains: > > > > > > > > drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif' > > > > drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif' > > > > > > > > Indeed, if the vif id returned by the device does not exist anymore, > > > > wdev_to_wvif() could return NULL. > > > > > > > > In add, the error is not handled uniformly in the code, sometime a > > > > WARN() is displayed but code continue, sometime a dev_warn() is > > > > displayed, sometime it is just not tested, ... > > > > > > > > This patch standardize that. > > > > > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > --- > > > > drivers/staging/wfx/data_tx.c | 5 ++++- > > > > drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++---------- > > > > drivers/staging/wfx/sta.c | 4 ++++ > > > > 3 files changed, 32 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c > > > > index b4d5dd3d2d23..8db0be08daf8 100644 > > > > --- a/drivers/staging/wfx/data_tx.c > > > > +++ b/drivers/staging/wfx/data_tx.c > > > > @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb) > > > > sizeof(struct hif_req_tx) + > > > > req->fc_offset; > > > > > > > > - WARN_ON(!wvif); > > > > + if (!wvif) { > > > > + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__); > > > > + return; > > > > + } > > > > > > I'm not really a fan of using function names in warning or error > > > messages as it clutters the log. In debug messages I think they are ok. > > > > In the initial code, I used WARN() that far more clutters the log (I > > have stated that a backtrace won't provide any useful information, so > > pr_warn() was better suited). > > > > In add, in my mind, these warnings are debug messages. If they appears, > > the user should probably report a bug. > > > > Finally, in this patch, I use the same message several times (ok, not > > this particular one). So the function name is a way to differentiate > > them. > > You should use dev_*() for these, that way you can properly determine > the exact device as well. Totally agree. I initially did that. However, the device is a field of wvif which is NULL in this case. I could have changed the code to get the real pointer to the device. But I didn't want to clutter the code just for a debug message (and also because I was a bit lazy).
diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c index b4d5dd3d2d23..8db0be08daf8 100644 --- a/drivers/staging/wfx/data_tx.c +++ b/drivers/staging/wfx/data_tx.c @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb) sizeof(struct hif_req_tx) + req->fc_offset; - WARN_ON(!wvif); + if (!wvif) { + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__); + return; + } wfx_tx_policy_put(wvif, req->retry_policy_index); skb_pull(skb, offset); ieee80211_tx_status_irqsafe(wvif->wdev->hw, skb); diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c index d6dfab094b03..ca09467cba05 100644 --- a/drivers/staging/wfx/hif_rx.c +++ b/drivers/staging/wfx/hif_rx.c @@ -110,9 +110,9 @@ static int hif_receive_indication(struct wfx_dev *wdev, const struct hif_ind_rx *body = buf; if (!wvif) { - dev_warn(wdev->dev, "ignore rx data for non-existent vif %d\n", - hif->interface); - return 0; + dev_warn(wdev->dev, "%s: ignore rx data for non-existent vif %d\n", + __func__, hif->interface); + return -EIO; } skb_pull(skb, sizeof(struct hif_msg) + sizeof(struct hif_ind_rx)); wfx_rx_cb(wvif, body, skb); @@ -128,8 +128,8 @@ static int hif_event_indication(struct wfx_dev *wdev, int type = le32_to_cpu(body->event_id); if (!wvif) { - dev_warn(wdev->dev, "received event for non-existent vif\n"); - return 0; + dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__); + return -EIO; } switch (type) { @@ -161,7 +161,10 @@ static int hif_pm_mode_complete_indication(struct wfx_dev *wdev, { struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface); - WARN_ON(!wvif); + if (!wvif) { + dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__); + return -EIO; + } complete(&wvif->set_pm_mode_complete); return 0; @@ -173,7 +176,11 @@ static int hif_scan_complete_indication(struct wfx_dev *wdev, { struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface); - WARN_ON(!wvif); + if (!wvif) { + dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__); + return -EIO; + } + wfx_scan_complete(wvif); return 0; @@ -185,7 +192,10 @@ static int hif_join_complete_indication(struct wfx_dev *wdev, { struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface); - WARN_ON(!wvif); + if (!wvif) { + dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__); + return -EIO; + } dev_warn(wdev->dev, "unattended JoinCompleteInd\n"); return 0; @@ -195,11 +205,15 @@ static int hif_suspend_resume_indication(struct wfx_dev *wdev, const struct hif_msg *hif, const void *buf) { - struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface); const struct hif_ind_suspend_resume_tx *body = buf; + struct wfx_vif *wvif; if (body->bc_mc_only) { - WARN_ON(!wvif); + wvif = wdev_to_wvif(wdev, hif->interface); + if (!wvif) { + dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__); + return -EIO; + } if (body->resume) wfx_suspend_resume_mc(wvif, STA_NOTIFY_AWAKE); else diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c index a246f0d1d6e9..2320a81eae0b 100644 --- a/drivers/staging/wfx/sta.c +++ b/drivers/staging/wfx/sta.c @@ -619,6 +619,10 @@ int wfx_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta, bool set) struct wfx_sta_priv *sta_dev = (struct wfx_sta_priv *)&sta->drv_priv; struct wfx_vif *wvif = wdev_to_wvif(wdev, sta_dev->vif_id); + if (!wvif) { + dev_warn(wdev->dev, "%s: received event for non-existent vif\n", __func__); + return -EIO; + } schedule_work(&wvif->update_tim_work); return 0; }