Re: [PATCH 2/4] stagign: wilc1000: validate cfg parameters before scheduling the work
On Thu, Oct 25, 2018 at 09:11:00PM +, adham.aboza...@microchip.com wrote: You'll need to resend these because your email from header is in the wrong format. Also there is a typo in the subject. s/stagign/staging/. regards, dan carpenter
Re: [PATCH] ath9k: Fix a locking bug in ath9k_add_interface()
On Wed, Oct 24, 2018 at 08:50:52AM +0300, Kalle Valo wrote:
> Dan Carpenter writes:
>
> > We tried to revert commit d9c52fd17cb4 ("ath9k: fix tx99 with monitor
> > mode interface") but accidentally missed part of the locking change.
> >
> > The lock has to be held earlier so that we're holding it when we do
> > "sc->tx99_vif = vif;" and also there in the current code there is a
> > stray unlock before we have taken the lock.
> >
> > Fixes: 6df0580be8bc ("ath9k: add back support for using active monitor
> > interfaces for tx99")
> > Signed-off-by: Dan Carpenter
>
> commit 6df0580be8bc is on it's way to v4.20 so should I also queue this
> to v4.20?
Yeah. Obviously this is a static checker thing and I haven't tested it.
I don't know if add_interface() is ever called in parallel, but I can
imagine that it might be. In that case the race condition is something
that would affect real life.
Anyway, it's a small obvious fix.
regards,
dan carpenter
Re: [PATCH] wireless: airo: potential buffer overflow in sprintf()
On Wed, Oct 24, 2018 at 11:56:53AM +0300, Kalle Valo wrote:
> Dan Carpenter writes:
>
> > It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
> > of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
> > (precision) so if the ssid doesn't have a NUL terminator this could lead
> > to an overflow.
> >
> > Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
> > Signed-off-by: Dan Carpenter
> > ---
> > Static analsysis. Not tested.
>
> IMHO this part (after "---" line) is important information and should be
> part of commit log. I can fix that.
>
In my experience most maintainers disagree (with varying degrees of
intensity).
regards,
dan carpenter
[PATCH] wireless: airo: potential buffer overflow in sprintf()
It looks like we wanted to print a maximum of BSSList_rid.ssidLen bytes
of the ssid, but we accidentally use "%*s" (width) instead of "%.*s"
(precision) so if the ssid doesn't have a NUL terminator this could lead
to an overflow.
Fixes: e174961ca1a0 ("net: convert print_mac to %pM")
Signed-off-by: Dan Carpenter
---
Static analsysis. Not tested.
drivers/net/wireless/cisco/airo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/cisco/airo.c
b/drivers/net/wireless/cisco/airo.c
index 04dd7a936593..5512c7f73fce 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -5462,7 +5462,7 @@ static int proc_BSSList_open( struct inode *inode, struct
file *file ) {
we have to add a spin lock... */
rc = readBSSListRid(ai, doLoseSync, _rid);
while(rc == 0 && BSSList_rid.index != cpu_to_le16(0x)) {
- ptr += sprintf(ptr, "%pM %*s rssi = %d",
+ ptr += sprintf(ptr, "%pM %.*s rssi = %d",
BSSList_rid.bssid,
(int)BSSList_rid.ssidLen,
BSSList_rid.ssid,
--
2.11.0
[PATCH] ath9k: Fix a locking bug in ath9k_add_interface()
We tried to revert commit d9c52fd17cb4 ("ath9k: fix tx99 with monitor
mode interface") but accidentally missed part of the locking change.
The lock has to be held earlier so that we're holding it when we do
"sc->tx99_vif = vif;" and also there in the current code there is a
stray unlock before we have taken the lock.
Fixes: 6df0580be8bc ("ath9k: add back support for using active monitor
interfaces for tx99")
Signed-off-by: Dan Carpenter
---
drivers/net/wireless/ath/ath9k/main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c
b/drivers/net/wireless/ath/ath9k/main.c
index 1e3b5f4a4cf9..f23cb2f3d296 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1251,6 +1251,7 @@ static int ath9k_add_interface(struct ieee80211_hw *hw,
struct ath_vif *avp = (void *)vif->drv_priv;
struct ath_node *an = >mcast_node;
+ mutex_lock(>mutex);
if (IS_ENABLED(CONFIG_ATH9K_TX99)) {
if (sc->cur_chan->nvifs >= 1) {
mutex_unlock(>mutex);
@@ -1259,8 +1260,6 @@ static int ath9k_add_interface(struct ieee80211_hw *hw,
sc->tx99_vif = vif;
}
- mutex_lock(>mutex);
-
ath_dbg(common, CONFIG, "Attach a VIF of type: %d\n", vif->type);
sc->cur_chan->nvifs++;
--
2.11.0
[bug report] iwlwifi: mvm: kill INACTIVE queue state
Hello Johannes Berg, The patch 724fe7710ac5: "iwlwifi: mvm: kill INACTIVE queue state" from Jul 4, 2018, leads to the following static checker warning: drivers/net/wireless/intel/iwlwifi/mvm/sta.c:1243 iwl_mvm_inactivity_check() warn: 'iwl_mvm_remove_inactive_tids(mvm, mvmsta, i, inactive_tid_bitmap, _queues, _queues)' is unsigned drivers/net/wireless/intel/iwlwifi/mvm/sta.c 1223 /* 1224 * If the STA doesn't exist anymore, it isn't an error. It could 1225 * be that it was removed since getting the queues, and in this 1226 * case it should've inactivated its queues anyway. 1227 */ 1228 if (IS_ERR_OR_NULL(sta)) 1229 continue; 1230 1231 mvmsta = iwl_mvm_sta_from_mac80211(sta); 1232 1233 /* this isn't so nice, but works OK due to the way we loop */ 1234 spin_unlock(>queue_info_lock); 1235 1236 /* and we need this locking order */ 1237 spin_lock(>lock); 1238 spin_lock(>queue_info_lock); 1239 ret = iwl_mvm_remove_inactive_tids(mvm, mvmsta, i, 1240 inactive_tid_bitmap, 1241 _queues, 1242 _queues); 1243 if (ret >= 0 && free_queue < 0) The iwl_mvm_remove_inactive_tids() returns a bool so it doesn't make sense to test for >= 0. Probably we should test for ret == true? 1244 free_queue = ret; 1245 /* only unlock sta lock - we still need the queue info lock */ 1246 spin_unlock(>lock); 1247 } regards, dan carpenter
[PATCH] ath10k: htt: remove some dead code
We added an unnecessary condition here in commit a904417fc876 ("ath10k:
add extended per sta tx statistics support"). "legacy_rate_idx" is a u8
so it can't be negative. The caller doesn't pass negatives either. I
have deleted this code.
Signed-off-by: Dan Carpenter
---
I didn't use the Fixes tag because it's not really a bug.
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c
b/drivers/net/wireless/ath/ath10k/htt_rx.c
index f2405258a6d3..13717a287848 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -2680,8 +2680,6 @@ ath10k_accumulate_per_peer_tx_stats(struct ath10k *ar,
STATS_OP_FMT(RETRY).ht[1][ht_idx] += pstats->retry_pkts;
} else {
mcs = legacy_rate_idx;
- if (mcs < 0)
- return;
STATS_OP_FMT(SUCC).legacy[0][mcs] += pstats->succ_bytes;
STATS_OP_FMT(SUCC).legacy[1][mcs] += pstats->succ_pkts;
[bug report] qtnfmac: cleanup and unify command error handling
Hello Sergey Matyukevich,
The patch c6ed298ffe09: "qtnfmac: cleanup and unify command error
handling" from Oct 5, 2018, leads to the following static checker
warning:
drivers/net/wireless/quantenna/qtnfmac/commands.c:132
qtnf_cmd_send_with_reply()
warn: variable dereferenced before check 'resp' (see line 117)
drivers/net/wireless/quantenna/qtnfmac/commands.c
112 ret = qtnf_trans_send_cmd_with_resp(bus, cmd_skb, _skb);
113 if (ret)
114 goto out;
115
116 resp = (const struct qlink_resp *)resp_skb->data;
117 ret = qtnf_cmd_check_reply_header(resp, cmd_id, mac_id, vif_id,
Dereference
118const_resp_size);
119 if (ret)
120 goto out;
121
122 /* Return length of variable part of response */
123 if (response_skb && var_resp_size)
124 *var_resp_size = le16_to_cpu(resp->mhdr.len) -
const_resp_size;
125
126 out:
127 if (response_skb)
128 *response_skb = resp_skb;
129 else
130 consume_skb(resp_skb);
131
132 if (!ret && resp)
This new check is not required.
133 return
qtnf_cmd_resp_result_decode(le16_to_cpu(resp->result));
134
135 pr_warn("VIF%u.%u: cmd 0x%.4X failed: %d\n",
136 mac_id, vif_id, le16_to_cpu(cmd->cmd_id), ret);
137
138 return ret;
139 }
regards,
dan carpenter
Re: [PATCH v3 00/29] staging: wilc1000: avoid static variables and cleanup changes
On Tue, Sep 25, 2018 at 11:53:15AM +0530, Ajay Singh wrote: > This patch series contains changes to avoid the use of static variables. > Cleanup changes to fix some checkpatch issues and return void for > function if their return value is not used. > Also deleted 'wilc_debugfs.c' file as it's not used. > > Changes since v2: >Included Joe's suggestion for patch#28 > - replaced previous patch with an improved version(refactor code) > > Changes since v1: >Address Dan's comment for patch#29 > - return the correct error for failure in the second iteration Btw, I hate re-reviewing patches and I know everyone hates sending them. If someone complains about a 28/29 or a 29/29, then you can just ask Greg to apply the first 27 and redo the others in a follow on patchset. regards, dan carpenter
[bug report] NFC: st21nfca: Fix some skb memory leaks
Hello Christophe Ricard,
The patch c490c557b67f: "NFC: st21nfca: Fix some skb memory leaks"
from Jan 25, 2015, leads to the following static checker warning:
drivers/nfc/st21nfca/core.c:742
st21nfca_hci_complete_target_discovered()
warn: 'nfcid_skb' was already freed.
drivers/nfc/st21nfca/core.c
712 /* NFC Forum Digital Protocol Table 44 */
713 if (target->sensf_res[0] == 0x01 &&
714 target->sensf_res[1] == 0xfe)
715 target->supported_protocols =
716
NFC_PROTO_NFC_DEP_MASK;
717 else
718 target->supported_protocols =
719
NFC_PROTO_FELICA_MASK;
720 } else {
721 kfree_skb(nfcid_skb);
Freed.
722 /* P2P in type A */
723 r = nfc_hci_get_param(hdev,
ST21NFCA_RF_READER_F_GATE,
724 ST21NFCA_RF_READER_F_NFCID1,
725 _skb);
^^
This is set to a different new skb on some error paths but if we return
-EADDRNOTAVAIL then it's still the same freed skb.
726 if (r < 0)
727 goto exit;
^
We hit this goto and double free.
728
729 if (nfcid_skb->len > NFC_NFCID1_MAXSIZE) {
730 r = -EPROTO;
731 goto exit;
732 }
733 memcpy(target->sensf_res, nfcid_skb->data,
734 nfcid_skb->len);
735 target->sensf_res_len = nfcid_skb->len;
736 target->supported_protocols =
NFC_PROTO_NFC_DEP_MASK;
737 }
738 target->hci_reader_gate = ST21NFCA_RF_READER_F_GATE;
739 }
740 r = 1;
741 exit:
742 kfree_skb(nfcid_skb);
743 return r;
744 }
regards,
dan carpenter
Re: [PATCH] mac80211: allow scans on radar channels, unless there is CAC or CSA
On Thu, Sep 20, 2018 at 03:30:05PM +0200, Simon Wunderlich wrote: > Hi Dan, > > whoops, right ... thank you! > > Will do in a v2, at least if this patch is wanted. :) > These are automated emails, I just look at them and forward them. I don't actually "love your patch" because I haven't even looked at it. The bot adds that text to sound more friendly... Presumably the patch is wanted, but I don't know. Anyway, yes, please do send a v2. regards, dan carpenter
Re: [PATCH] mac80211: allow scans on radar channels, unless there is CAC or CSA
Hi Simon,
I love your patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/Simon-Wunderlich/mac80211-allow-scans-on-radar-channels-unless-there-is-CAC-or-CSA/20180919-071924
base: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git
master
New smatch warnings:
net/mac80211/scan.c:508 ieee80211_can_scan() warn: signedness bug returning
'(-16)'
Old smatch warnings:
net/mac80211/scan.c:511 ieee80211_can_scan() warn: signedness bug returning
'(-16)'
#
https://github.com/0day-ci/linux/commit/ad9617f275c425ddf25eb83678062ab87d4c0870
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout ad9617f275c425ddf25eb83678062ab87d4c0870
vim +508 net/mac80211/scan.c
f3b85252 Johannes Berg 2009-04-23 503
133d40f9 Stanislaw Gruszka 2012-03-28 504 static bool
ieee80211_can_scan(struct ieee80211_local *local,
133d40f9 Stanislaw Gruszka 2012-03-28 505 struct
ieee80211_sub_if_data *sdata)
133d40f9 Stanislaw Gruszka 2012-03-28 506 {
ad9617f2 Simon Wunderlich 2018-09-18 507 if (sdata->wdev.cac_started)
ad9617f2 Simon Wunderlich 2018-09-18 @508 return -EBUSY;
^
ad9617f2 Simon Wunderlich 2018-09-18 509
ad9617f2 Simon Wunderlich 2018-09-18 510 if (sdata->vif.csa_active)
ad9617f2 Simon Wunderlich 2018-09-18 511 return -EBUSY;
^^
164eb02d Simon Wunderlich 2013-02-08 512
2eb278e0 Johannes Berg 2012-06-05 513 if
(!list_empty(>roc_list))
133d40f9 Stanislaw Gruszka 2012-03-28 514 return false;
133d40f9 Stanislaw Gruszka 2012-03-28 515
133d40f9 Stanislaw Gruszka 2012-03-28 516 if (sdata->vif.type ==
NL80211_IFTYPE_STATION &&
392b9ffb Stanislaw Gruszka 2013-08-27 517 sdata->u.mgd.flags &
IEEE80211_STA_CONNECTION_POLL)
133d40f9 Stanislaw Gruszka 2012-03-28 518 return false;
133d40f9 Stanislaw Gruszka 2012-03-28 519
133d40f9 Stanislaw Gruszka 2012-03-28 520 return true;
133d40f9 Stanislaw Gruszka 2012-03-28 521 }
133d40f9 Stanislaw Gruszka 2012-03-28 522
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH 29/29] staging: wilc1000: return exact error of register_netdev() from wilc_netdev_init()
I was waiting for you to send this like a spider waits for flies. You
fell directly into my trap. Mwuahahahahaha.
drivers/staging/wilc1000/linux_wlan.c
1056 int wilc_netdev_init(struct wilc **wilc, struct device *dev, int
io_type,
1057 const struct wilc_hif_func *ops)
1058 {
1059 int i, ret = -ENOMEM;
1060 struct wilc_vif *vif;
1061 struct net_device *ndev;
1062 struct wilc *wl;
1063
1064 wl = kzalloc(sizeof(*wl), GFP_KERNEL);
1065 if (!wl)
1066 return ret;
^^^
It's cleaner to return -ENOMEM so that we don't have to glance up to the
declaration block. This is especially true when "ret" is zero, btw,
because that can indicate a reversed test.
if (!ret)
return ret;
In this theoretically example it was supposed to be:
if (ret)
return ret;
Normally, reversed conditions are caught in testing, but for the kernel,
no one has the hardware to test everything so we do get reversed
conditions from time to time.
1067
1068 if (wilc_wlan_cfg_init(wl))
1069 goto free_wl;
1070
1071 *wilc = wl;
1072 wl->io_type = io_type;
1073 wl->hif_func = ops;
1074 wl->enable_ps = true;
1075 wl->chip_ps_state = CHIP_WAKEDUP;
1076 INIT_LIST_HEAD(>txq_head.list);
1077 INIT_LIST_HEAD(>rxq_head.list);
1078
1079 wl->hif_workqueue = create_singlethread_workqueue("WILC_wq");
1080 if (!wl->hif_workqueue)
1081 goto free_cfg;
1082
1083 register_inetaddr_notifier(_dev_notifier);
1084
1085 for (i = 0; i < NUM_CONCURRENT_IFC; i++) {
1086 struct wireless_dev *wdev;
1087
1088 ndev = alloc_etherdev(sizeof(struct wilc_vif));
1089 if (!ndev)
1090 goto free_ndev;
^^^
ret is zero on the second iteration through the loop.
1091
1092 vif = netdev_priv(ndev);
1093 memset(vif, 0, sizeof(struct wilc_vif));
1094
1095 if (i == 0) {
1096 strcpy(ndev->name, "wlan%d");
1097 vif->ifc_id = 1;
1098 } else {
1099 strcpy(ndev->name, "p2p%d");
1100 vif->ifc_id = 0;
1101 }
1102 vif->wilc = *wilc;
1103 vif->ndev = ndev;
1104 wl->vif[i] = vif;
1105 wl->vif_num = i;
1106 vif->idx = wl->vif_num;
1107
1108 ndev->netdev_ops = _netdev_ops;
1109
1110 wdev = wilc_create_wiphy(ndev, dev);
if (!wdev) {
1112 netdev_err(ndev, "Can't register WILC Wiphy\n");
1113 goto free_ndev;
^^^
Here too.
1114 }
1115
1116 SET_NETDEV_DEV(ndev, dev);
1117
1118 vif->ndev->ieee80211_ptr = wdev;
1119 vif->ndev->ml_priv = vif;
1120 wdev->netdev = vif->ndev;
1121 vif->netstats.rx_packets = 0;
1122 vif->netstats.tx_packets = 0;
1123 vif->netstats.rx_bytes = 0;
1124 vif->netstats.tx_bytes = 0;
1125
1126 ret = register_netdev(ndev);
1127 if (ret)
1128 goto free_ndev;
ret is cleared on the first iteration through the loop.
1129
1130 vif->iftype = STATION_MODE;
1131 vif->mac_opened = 0;
1132 }
1133
1134 return 0;
regards,
dan carpenter
[bug report] mt76: unify wait_for_mac
Hello Stanislaw Gruszka,
The patch 2735a6dd7df3: "mt76: unify wait_for_mac" from Aug 29, 2018,
leads to the following static checker warning:
drivers/net/wireless/mediatek/mt76/mt76x02_mac.h:60 mt76x02_wait_for_mac()
warn: signedness bug returning '(-5)'
drivers/net/wireless/mediatek/mt76/mt76x02_mac.h
53 static inline bool mt76x02_wait_for_mac(struct mt76_dev *dev)
54 {
55 const u32 MAC_CSR0 = 0x1000;
56 int i;
57
58 for (i = 0; i < 500; i++) {
59 if (test_bit(MT76_REMOVED, >state))
60 return -EIO;
This is supposed to be true or false.
61
62 switch (dev->bus->rr(dev, MAC_CSR0)) {
63 case 0:
64 case ~0:
65 break;
66 default:
67 return true;
68 }
69 usleep_range(5000, 1);
70 }
71 return false;
72 }
regards,
dan carpenter
[PATCH] cfg80211: fix a type issue in ieee80211_chandef_to_operating_class()
The "chandef->center_freq1" variable is a u32 but "freq" is a u16 so we
are truncating away the high bits. I noticed this bug because in commit
9cf0a0b4b64a ("cfg80211: Add support for 60GHz band channels 5 and 6")
we made "freq <= 56160 + 2160 * 6" a valid requency when before it was
only "freq <= 56160 + 2160 * 4" that was valid. It introduces a static
checker warning:
net/wireless/util.c:1571 ieee80211_chandef_to_operating_class()
warn: always true condition '(freq <= 56160 + 2160 * 6) => (0-u16max <=
69120)'
But really we probably shouldn't have been truncating the high bits
away to begin with.
Signed-off-by: Dan Carpenter
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 2a89db5f2db7..4293f980e9c4 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1456,7 +1456,7 @@ bool ieee80211_chandef_to_operating_class(struct
cfg80211_chan_def *chandef,
u8 *op_class)
{
u8 vht_opclass;
- u16 freq = chandef->center_freq1;
+ u32 freq = chandef->center_freq1;
if (freq >= 2412 && freq <= 2472) {
if (chandef->width > NL80211_CHAN_WIDTH_40)
Re: [PATCH 20/24] staging: wilc1000: avoid line over 80 chars in tcp_process()
On Mon, Aug 27, 2018 at 10:54:38AM +0530, Ajay Singh wrote:
> Hi Claudiu,
>
> On Fri, 24 Aug 2018 12:31:28 +0300
> Claudiu Beznea wrote:
>
> > On 23.08.2018 13:33, Ajay Singh wrote:
> > > On Thu, 23 Aug 2018 11:12:08 +0300
> > > Claudiu Beznea wrote:
> > >
> > >> On 14.08.2018 09:50, Ajay Singh wrote:
> > >>> Cleanup patch to avoid line over 80 chars issue reported by
> > >>> checkpatch.pl script.
> > >>>
> > >>> Signed-off-by: Ajay Singh
> > >>> ---
> > >>> drivers/staging/wilc1000/wilc_wlan.c | 7 ++-
> > >>> 1 file changed, 6 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/staging/wilc1000/wilc_wlan.c
> > >>> b/drivers/staging/wilc1000/wilc_wlan.c index 041c9dd..f0743d9
> > >>> 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c
> > >>> +++ b/drivers/staging/wilc1000/wilc_wlan.c
> > >>> @@ -137,6 +137,11 @@ static inline int add_tcp_pending_ack(struct
> > >>> wilc_vif *vif, u32 ack, return 0;
> > >>> }
> > >>>
> > >>> +static inline void clear_tcp_session_txq(struct wilc_vif *vif,
> > >>> int index) +{
> > >>> + vif->ack_filter.pending_acks_info[index].txqe = NULL;
> > >>> +}
> > >>> +
> > >>
> > >> This seems useless to me...
> > >
> > > Sorry, this point is not fully clear to me.
> > >
> > > Did you mean setting of 'NULL' to 'pending_acks_info[index].txqe' is
> > > not required?
> > >
> >
> > No, having a new function that sets a variable just to avoid line
> > over 80 warning.
>
> Okay got it.
> How about using a temp variable to hold the value of
> 'tqe->tcp_pending_ack_idx'. It can also help to overcome the
> checkpatch warning.
Just ignore the checkpatch warning... Don't add a temporary variable
just to please checkpatch... It's good to fix checkpatch warnings if
it makes the code cleaner, but I hate when people do:
tmp = some_long_variable_name;
some_other_long_variable = tmp;
The tmp variable is only used one time and a simple statement becomes
two statements and it breaks grep.
You could consider renaming some of the variables. I think the "_info"
doesn't add anything, because obviously it's information. Maybe
"tcp_pending_ack_index" could just become "->ack_idx".
vif->ack_filter.pending_ack[tqe->ack_idx].txqe = NULL;
It's still very searchable. If you grep for "ack_idx" then the pending
and TCP are right there so no information is lost.
regards,
dan carpenter
Re: [PATCH 23/24] staging: wilc1000: move 'wilc_connecting' static variable to 'wilc_vif' struct
On Thu, Aug 23, 2018 at 04:57:48PM +0530, Ajay Singh wrote:
> Hi Greg,
>
> On Thu, 23 Aug 2018 12:55:27 +0200
> Greg KH wrote:
>
> > On Tue, Aug 14, 2018 at 12:20:15PM +0530, Ajay Singh wrote:
> > > --- a/drivers/staging/wilc1000/wilc_wfi_netdevice.h
> > > +++ b/drivers/staging/wilc1000/wilc_wfi_netdevice.h
> > > @@ -151,6 +151,7 @@ struct wilc_vif {
> > > struct timer_list periodic_rssi;
> > > struct rf_info periodic_stat;
> > > struct tcp_ack_filter ack_filter;
> > > + int connecting;
> >
> > Shouldn't this be a boolean?
> >
>
> Yes, 'connecting' only have value as 0 or 1. I will change it to
> bool and rename it to 'is_connecting'.
I think just the name "connecting" implies bool so there is no need for
the "is_".
regards,
dan carpenter
[PATCH] rt2x00: use simple_read_from_buffer()
The problem with this copy_to_user() calls is that they don't ensure
that "size" is less than the "length" which the user provided.
Obviously, this is debugfs and "size" is normally going to be very small
so it probably doesn't matter, but this is the correct thing to do.
Signed-off-by: Dan Carpenter
---
>From static analysis. Not tested.
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
index acc399b5574e..61ba573e8bf1 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
@@ -464,11 +464,7 @@ static ssize_t rt2x00debug_read_##__name(struct file
*file,\
\
size = sprintf(line, __format, value); \
\
- if (copy_to_user(buf, line, size)) \
- return -EFAULT; \
- \
- *offset += size;\
- return size;\
+ return simple_read_from_buffer(buf, length, offset, line, size); \
}
#define RT2X00DEBUGFS_OPS_WRITE(__name, __type)\
@@ -545,11 +541,7 @@ static ssize_t rt2x00debug_read_dev_flags(struct file
*file,
size = sprintf(line, "0x%.8x\n", (unsigned int)intf->rt2x00dev->flags);
- if (copy_to_user(buf, line, size))
- return -EFAULT;
-
- *offset += size;
- return size;
+ return simple_read_from_buffer(buf, length, offset, line, size);
}
static const struct file_operations rt2x00debug_fop_dev_flags = {
@@ -574,11 +566,7 @@ static ssize_t rt2x00debug_read_cap_flags(struct file
*file,
size = sprintf(line, "0x%.8x\n", (unsigned
int)intf->rt2x00dev->cap_flags);
- if (copy_to_user(buf, line, size))
- return -EFAULT;
-
- *offset += size;
- return size;
+ return simple_read_from_buffer(buf, length, offset, line, size);
}
static const struct file_operations rt2x00debug_fop_cap_flags = {
Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error
On Sun, Aug 12, 2018 at 05:48:52PM +0530, Ajay Singh wrote: > Yes, currently only code to read and writing for "wilc_debug_level" > exists. > > The main purpose for this file was to configure(enable/disable) > specific level debug logs to print. > The kernel's dev_dbg() macros are really very clever. You can enable them for one file or for a specific line or for a function. regards, dan carpenter
[PATCH] libertas_tf: prevent underflow in process_cmdrequest()
If recvlength is less than MESSAGE_HEADER_LEN (4) we would end up
corrupting memory.
Fixes: c305a19a0d0a ("libertas_tf: usb specific functions")
Signed-off-by: Dan Carpenter
---
This is from static analysis. I'm not positive that this is a real
bug, but it's harmless to check.
diff --git a/drivers/net/wireless/marvell/libertas_tf/if_usb.c
b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
index e92fc5001171..789337ea676a 100644
--- a/drivers/net/wireless/marvell/libertas_tf/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
@@ -605,9 +605,10 @@ static inline void process_cmdrequest(int recvlength,
uint8_t *recvbuff,
{
unsigned long flags;
- if (recvlength > LBS_CMD_BUFFER_SIZE) {
+ if (recvlength < MESSAGE_HEADER_LEN ||
+ recvlength > LBS_CMD_BUFFER_SIZE) {
lbtf_deb_usbd(>udev->dev,
-"The receive buffer is too large\n");
+"The receive buffer is invalid: %d\n", recvlength);
kfree_skb(skb);
return;
}
Re: [PATCH] staging: wilc1000: fix undefined reference to `__this_module' compilation error
On Thu, Aug 09, 2018 at 02:13:24PM +0200, Greg KH wrote:
> On Thu, Aug 09, 2018 at 01:43:58PM +0300, Dan Carpenter wrote:
> > On Thu, Aug 09, 2018 at 01:08:38PM +0300, Dan Carpenter wrote:
> > > On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote:
> > > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
> > > > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) {
> > > > - info = _info[i];
> > > > - debugfs_create_file(info->name,
> > > > - info->perm,
> > > > - wilc_dir,
> > > > - >data,
> > > > - >fops);
> > > > + if (IS_ERR_OR_NULL(wilc_dir)) {
> > > > + pr_err("Error creating debugfs\n");
> > > > + return -EFAULT;
> > > > }
> > >
> > > Just check for NULL. If someone builds without debugfs enabled in their
> > > .config, that's their choice. No need to print a warning.
> > >
> >
> > Reading it again, I'm not sure my email was clear... Just do this:
> >
> > wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
> > if (!wilc_dir)) {
> > pr_err("Error creating debugfs/wilc_wifi/\n");
> > return;
> > }
> >
> > If debugfs_create_dir() returns an error pointer it means all the other
> > debugfs functions are also just no-op stub functions. Passing an error
> > pointer to them is harmless.
>
> No, please never care about the return value of a debugfs call, it
> should never cause your code flow to do anything different. THis should
> just be:
> wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
> and then keep on going. You can always pass the return value of a
> debugfs call to another one, no need to check anything.
>
> I've done a large sweep of the kernel tree for most of this pattern for
> 4.18, and will keep doing it over time, as it keeps creeping back.
Yeah, that's how I thought it was supposed be but then the cleanup
doesn't work. We end up putting all the new debugfs files in the base
debugsf directory. Doesn't it lead to a use after free if we unload the
module?
I know that you aren't supposed to unload modules in production.
regards,
dan carpenter
Re: [PATCH] staging: wilc1000: fix undefined reference to `__this_module' compilation error
On Thu, Aug 09, 2018 at 01:08:38PM +0300, Dan Carpenter wrote:
> On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote:
> > wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
> > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) {
> > - info = _info[i];
> > - debugfs_create_file(info->name,
> > - info->perm,
> > - wilc_dir,
> > - >data,
> > - >fops);
> > + if (IS_ERR_OR_NULL(wilc_dir)) {
> > + pr_err("Error creating debugfs\n");
> > + return -EFAULT;
> > }
>
> Just check for NULL. If someone builds without debugfs enabled in their
> .config, that's their choice. No need to print a warning.
>
Reading it again, I'm not sure my email was clear... Just do this:
wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
if (!wilc_dir)) {
pr_err("Error creating debugfs/wilc_wifi/\n");
return;
}
If debugfs_create_dir() returns an error pointer it means all the other
debugfs functions are also just no-op stub functions. Passing an error
pointer to them is harmless.
regards,
dan carpenter
Re: [PATCH] staging: wilc1000: fix undefined reference to `__this_module' compilation error
On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote:
> wilc_debug.o object file is included for both SDIO and SPI module. When
> anyone(either SDIO or SPI) module is compiled as loaded module and another
> as buildin module then below compilation error occurs.
>
> "drivers/staging/wilc1000/wilc_debugfs.o:(.data+0x10): undefined
> reference to `__this_module'"
>
> Moved the declaration of file_operation variable in SDIO/SPI files and
> pass this as parameter to wilc_debugfs_init().
> Refactor wilc_debugfs_init() as its not required to maintain
> 'wilc_debugfs_info_t' in debugfs_info[] array. Also modified file
> permission from 0666 to 0600 & use 'data' field as 'NULL' in
> debugfs_create_file() call.
>
> Fixes: 9abc44ba4e2f("staging: wilc1000: fix TODO to compile spi and sdio
> components in single module")
>
> Reported-by: kbuild test robot
I don't think you need to resend, but the Fixes tag format isn't
right.
1) Don't line wrap it. (Maybe checkpatch complains? Ignore checkpatch).
2) Put a space between the git hash and the '('.
3) No blank line between the Fixes and the Reported-by.
> -int wilc_debugfs_init(void)
> +int wilc_debugfs_init(const struct file_operations *fops)
You may as well make wilc_debugfs_init() void since no one checks it.
> {
> - int i;
> - struct wilc_debugfs_info_t *info;
> -
> wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
> - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) {
> - info = _info[i];
> - debugfs_create_file(info->name,
> - info->perm,
> - wilc_dir,
> - >data,
> - >fops);
> + if (IS_ERR_OR_NULL(wilc_dir)) {
> + pr_err("Error creating debugfs\n");
> + return -EFAULT;
> }
Just check for NULL. If someone builds without debugfs enabled in their
.config, that's their choice. No need to print a warning.
regards,
dan carpenter
[bug report] mt76x0: eeprom files
Hello Stanislaw Gruszka,
The patch e87b5039511a: "mt76x0: eeprom files" from Jul 31, 2018,
leads to the following static checker warning:
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c:250
mt76x0_set_lna_gain()
warn: impossible condition '(gain == 255) => ((-128)-127 == 255)'
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c
241 static void
242 mt76x0_set_lna_gain(struct mt76x0_dev *dev, u8 *eeprom)
243 {
244 s8 gain;
^^
Should this be a "u8"?
245
246 dev->ee->lna_gain_2ghz = eeprom[MT_EE_LNA_GAIN_2GHZ];
247 dev->ee->lna_gain_5ghz[0] = eeprom[MT_EE_LNA_GAIN_5GHZ_0];
248
249 gain = eeprom[MT_EE_LNA_GAIN_5GHZ_1];
250 if (gain == 0xff || gain == 0)
Impossible
251 dev->ee->lna_gain_5ghz[1] = dev->ee->lna_gain_5ghz[0];
252 else
253 dev->ee->lna_gain_5ghz[1] = gain;
254
255 gain = eeprom[MT_EE_LNA_GAIN_5GHZ_2];
256 if (gain == 0xff || gain == 0)
Same
257 dev->ee->lna_gain_5ghz[2] = dev->ee->lna_gain_5ghz[0];
258 else
259 dev->ee->lna_gain_5ghz[2] = gain;
260 }
regards,
dan carpenter
[bug report] mt76x0: mac files
Hello Stanislaw Gruszka,
The patch a77443498137: "mt76x0: mac files" from Jul 31, 2018, leads
to the following static checker warning:
drivers/net/wireless/mediatek/mt76/mt76x0/mac.c:429
mt76x0_mac_set_ampdu_factor()
info: ignoring unreachable code.
drivers/net/wireless/mediatek/mt76/mt76x0/mac.c
419 void mt76x0_mac_set_ampdu_factor(struct mt76x0_dev *dev)
420 {
421 struct ieee80211_sta *sta;
422 struct mt76_wcid *wcid;
423 void *msta;
424 u8 min_factor = 3;
425 int i;
426
427 return;
^^^
Did you mean to comment this function out?
428
429 rcu_read_lock();
430 for (i = 0; i < ARRAY_SIZE(dev->wcid); i++) {
431 wcid = rcu_dereference(dev->wcid[i]);
432 if (!wcid)
433 continue;
434
435 msta = container_of(wcid, struct mt76_sta, wcid);
436 sta = container_of(msta, struct ieee80211_sta,
drv_priv);
437
438 min_factor = min(min_factor, sta->ht_cap.ampdu_factor);
439 }
440 rcu_read_unlock();
441
442 mt76_wr(dev, MT_MAX_LEN_CFG, 0xa0fff |
443 FIELD_PREP(MT_MAX_LEN_CFG_AMPDU, min_factor));
444 }
regards,
dan carpenter
[bug report] mt76: add mt76x2_tx_common to mt76x2-common module
Hello Lorenzo Bianconi,
The patch 9367a9c7f956: "mt76: add mt76x2_tx_common to mt76x2-common
module" from Jul 31, 2018, leads to the following static checker
warning:
drivers/net/wireless/mediatek/mt76/mt76x2_tx_common.c:35 mt76x2_tx()
warn: always true condition '(wcid->hw_key_idx != -1) => (0-255 !=
(-1))'
drivers/net/wireless/mediatek/mt76/mt76x2_tx_common.c
21 void mt76x2_tx(struct ieee80211_hw *hw, struct ieee80211_tx_control
*control,
22 struct sk_buff *skb)
23 {
24 struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
25 struct mt76x2_dev *dev = hw->priv;
26 struct ieee80211_vif *vif = info->control.vif;
27 struct mt76_wcid *wcid = >global_wcid;
28
29 if (control->sta) {
30 struct mt76x2_sta *msta;
31
32 msta = (struct mt76x2_sta *)control->sta->drv_priv;
33 wcid = >wcid;
34 /* sw encrypted frames */
35 if (!info->control.hw_key && wcid->hw_key_idx != -1)
^^
This is always going to be true.
36 control->sta = NULL;
37 }
38
39 if (vif && !control->sta) {
40 struct mt76x2_vif *mvif;
41
42 mvif = (struct mt76x2_vif *)vif->drv_priv;
43 wcid = >group_wcid;
44 }
45
46 mt76_tx(>mt76, control->sta, wcid, skb);
47 }
48 EXPORT_SYMBOL_GPL(mt76x2_tx);
regards,
dan carpenter
Re: [PATCH 05/23] staging: wilc1000: rename goto to avoid leading '_' in label name
Fantastic. Thanks! regards, dan carpenter
Re: [PATCH 05/23] staging: wilc1000: rename goto to avoid leading '_' in label name
On Mon, Jul 30, 2018 at 03:40:24PM +0530, Ajay Singh wrote:
> Hi Dan,
>
> On Mon, 30 Jul 2018 11:41:13 +0300
> Dan Carpenter wrote:
>
> > On Fri, Jul 20, 2018 at 04:35:24AM +0530, Ajay Singh wrote:
> > > Hi Dan,
> > >
> > > On Thu, 19 Jul 2018 12:27:44 +0300
> > > Dan Carpenter wrote:
> > >
> > > > On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh wrote:
> > > > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c
> > > > > b/drivers/staging/wilc1000/wilc_wlan.c index 85af365..8e71c28
> > > > > 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c
> > > > > +++ b/drivers/staging/wilc1000/wilc_wlan.c
> > > > > @@ -850,13 +850,13 @@ static void
> > > > > wilc_wlan_handle_isr_ext(struct wilc *wilc, u32 int_status) if
> > > > > (wilc->rx_buffer) buffer = >rx_buffer[offset];
> > > > > else
> > > > > - goto _end_;
> > > > > + goto end;
> > > >
> > > > This isn't related to your patch but this goto doesn't appear to
> > > > make any sort of sense. I have no idea what was intended.
> > > >
> > >
> > > Thanks for pointing it out. I will include these changes in separate
> > > patchset.
> > >
> > > Yes, the position of goto label can be moved just before
> > > wilc_wlan_handle_rxq(wilc), as 'ret' will always be '0' when goto
> > > statement is executed.
> > >
> > > Actually earlier there were few more goto statement in this function
> > > and single label 'end' was used to handle for different cases. But
> > > in previous cleanup patches those cases were removed.
> > > Now this function can be further refactor by either moving
> > > goto label before wilc_wlan_handle_rxq(wilc) or avoid goto use by
> > > adding the rx_buffer validation along with size check.
> > >
> > > i.e
> > >
> > > end:
> > > wilc_wlan_handle_rxq(wilc)
> > >
> > >
> > > OR
> > >
> > > if (size > 0 && wilc->rx_buffer) {
> > >
> > >
> > > }
> > > wilc_wlan_handle_rxq(wilc)
> > >
> >
> > Actually looking at it now, you could probably just remove the if
> > statement. Hopefully wilc->rx_buffer is non-NULL at this point? Is
> > there really any need to call wilc_wlan_handle_rxq() when we haven't
> > called wilc_wlan_rxq_add()?
> >
>
> Yes, wilc->rx_buffer would be non NULL value as its only one time
> allocated buffer. wilc_wlan_handle_rxq() was called without
> wilc_wlan_rxq_add() just as a fail safe to ensure there are no pending
> packets in the queue.
The only thing is wilc->quit can be set. Otherwise if ->rx_buffer is
NULL it would just result in a NULL dereference. (We are deep into
hypotheticals here because we're discussing impossible code).
regards,
dan carpenter
Re: [PATCH 05/23] staging: wilc1000: rename goto to avoid leading '_' in label name
On Fri, Jul 20, 2018 at 04:35:24AM +0530, Ajay Singh wrote:
> Hi Dan,
>
> On Thu, 19 Jul 2018 12:27:44 +0300
> Dan Carpenter wrote:
>
> > On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh wrote:
> > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c
> > > b/drivers/staging/wilc1000/wilc_wlan.c index 85af365..8e71c28 100644
> > > --- a/drivers/staging/wilc1000/wilc_wlan.c
> > > +++ b/drivers/staging/wilc1000/wilc_wlan.c
> > > @@ -850,13 +850,13 @@ static void wilc_wlan_handle_isr_ext(struct
> > > wilc *wilc, u32 int_status) if (wilc->rx_buffer)
> > > buffer = >rx_buffer[offset];
> > > else
> > > - goto _end_;
> > > + goto end;
> >
> > This isn't related to your patch but this goto doesn't appear to make
> > any sort of sense. I have no idea what was intended.
> >
>
> Thanks for pointing it out. I will include these changes in separate
> patchset.
>
> Yes, the position of goto label can be moved just before
> wilc_wlan_handle_rxq(wilc), as 'ret' will always be '0' when goto
> statement is executed.
>
> Actually earlier there were few more goto statement in this function
> and single label 'end' was used to handle for different cases. But in
> previous cleanup patches those cases were removed.
> Now this function can be further refactor by either moving
> goto label before wilc_wlan_handle_rxq(wilc) or avoid goto use by
> adding the rx_buffer validation along with size check.
>
> i.e
>
> end:
> wilc_wlan_handle_rxq(wilc)
>
>
> OR
>
> if (size > 0 && wilc->rx_buffer) {
>
>
> }
> wilc_wlan_handle_rxq(wilc)
>
Actually looking at it now, you could probably just remove the if
statement. Hopefully wilc->rx_buffer is non-NULL at this point? Is
there really any need to call wilc_wlan_handle_rxq() when we haven't
called wilc_wlan_rxq_add()?
regards,
dan carpenter
Re: [PATCH 05/23] staging: wilc1000: rename goto to avoid leading '_' in label name
On Thu, Jul 19, 2018 at 04:15:01AM +0530, Ajay Singh wrote:
> diff --git a/drivers/staging/wilc1000/wilc_wlan.c
> b/drivers/staging/wilc1000/wilc_wlan.c
> index 85af365..8e71c28 100644
> --- a/drivers/staging/wilc1000/wilc_wlan.c
> +++ b/drivers/staging/wilc1000/wilc_wlan.c
> @@ -850,13 +850,13 @@ static void wilc_wlan_handle_isr_ext(struct wilc *wilc,
> u32 int_status)
> if (wilc->rx_buffer)
> buffer = >rx_buffer[offset];
> else
> - goto _end_;
> + goto end;
This isn't related to your patch but this goto doesn't appear to make
any sort of sense. I have no idea what was intended.
>
> wilc->hif_func->hif_clear_int_ext(wilc,
> DATA_INT_CLR | ENABLE_RX_VMM);
> ret = wilc->hif_func->hif_block_rx_ext(wilc, 0, buffer, size);
>
> -_end_:
> +end:
> if (ret) {
> offset += size;
> wilc->rx_buffer_offset = offset;
regards,
dan carpenter
Re: [PATCH] staging: wilc1000: fix static checker warning to unlock mutex in wilc_deinit()
On Thu, Jul 05, 2018 at 09:18:53AM +0530, Ajay Singh wrote:
> Fix for static checker warning inconsistent returns of
> 'hif_deinit_lock'(more details [1]).
>
> "drivers/staging/wilc1000/host_interface.c:3390 wilc_deinit()
> warn: inconsistent returns 'hif_deinit_lock'."
>
> Introduced in "ff52a57a7a42: staging: wilc1000: move the allocation of
> cmd out of wilc_enqueue_cmd()".
Can you use the Fixes tag so it's machine parseable?
Fixes: ff52a57a7a42 ("staging: wilc1000: move the allocation of cmd out of
wilc_enqueue_cmd()")
With the fixes tag, say someone pulls ff52a57a7a42 into their code, they
will know to pull this patch as well. Also scientists will be able to
measure the time between bug and fix and write a scholarly paper about
it.
regards,
dan carpenter
Re: New realtek 11ac driver
On Fri, Jun 29, 2018 at 11:38:22AM +0300, Kalle Valo wrote: > (Was "Re: [PATCH v3 00/19] rtlwifi: halmac: Add new module halmac", > changing the title to reflect what we are discussing) > > Pkshih writes: > > > On Thu, 2018-05-24 at 11:27 +0300, Kalle Valo wrote: > >> > >> You are missing my point: I don't even have time to review huge rtlwifi > >> patches when they are not even ready for upstream. I cannot start > >> working on cleaning up rtlwifi code and doing multiple iterations of > >> reviews on these kind of huge patchsets. Either you need to > >> significantly scale down the size of patchsets (especially LOC) or you > >> need to get review help from someone else. But the current way of > >> working is not doable for me. > >> > > > > Is there a proper way to look for "someone else" you mentioned? > > I don't know, I think there might a project somewhere which helps with > patch review for new people but not sure about that. Adding Dan in case > he has some ideas. It's just heart breaking to look at that driver. It's over 64k line of code. As a reviewer, it's easy to glance at the first few lines and say get rid of halmac_ret_status and use normal kernel return codes instead but just implementing that small thing would take months because there is so much code. It doesn't even look like terrible code, it's just not linux code. There is no way I'm touching these patches because it's just miles and miles of sadness. > > We plan to rewrite a new driver excluding agnostic OS layer to support > > new generation 11AC chips, because they're very different from the chips > > existed in rtlwifi and rtl8xxxu. That's good news. The OS layer was a failed experiment. It was supposed to give us one driver for everything but instead of that we have at least 7 drivers just within the Linux kernel. We abstracted the wrong thing. regards, dan carpenter
Re: [PATCH V3 2/2] brcmfmac: handle monitor mode marked msgbuf packets
Hi Rafał,
I love your patch! Perhaps something to improve:
[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on v4.18-rc1 next-20180619]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/brcmfmac-detect-firmware-support-for-monitor-interface/20180620-012610
base:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git
master
smatch warnings:
drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c:1166
brcmf_msgbuf_process_rx_complete() error: we previously assumed 'ifp' could be
null (see line 1161)
#
https://github.com/0day-ci/linux/commit/71f840a490aca9a2d4e9609641a929c6936c20e2
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 71f840a490aca9a2d4e9609641a929c6936c20e2
vim +/ifp +1166 drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1124
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1125
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1126 static void
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1127 brcmf_msgbuf_process_rx_complete(struct
brcmf_msgbuf *msgbuf, void *buf)
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1128 {
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1129 struct msgbuf_rx_complete *rx_complete;
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1130 struct sk_buff *skb;
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1131 u16 data_offset;
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1132 u16 buflen;
71f840a49 drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c Rafał
Miłecki2018-06-19 1133 u16 flags;
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1134 u32 idx;
c56caa9db drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c Franky Lin
2016-04-11 1135 struct brcmf_if *ifp;
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1136
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1137 brcmf_msgbuf_update_rxbufpost_count(msgbuf, 1);
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1138
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1139 rx_complete = (struct msgbuf_rx_complete *)buf;
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1140 data_offset =
le16_to_cpu(rx_complete->data_offset);
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1141 buflen = le16_to_cpu(rx_complete->data_len);
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1142 idx = le32_to_cpu(rx_complete->msg.request_id);
71f840a49 drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c Rafał
Miłecki2018-06-19 1143 flags = le16_to_cpu(rx_complete->flags);
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1144
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1145 skb =
brcmf_msgbuf_get_pktid(msgbuf->drvr->bus_if->dev,
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1146msgbuf->rx_pktids,
idx);
7d072b404 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Arend van
Spriel 2015-05-26 1147 if (!skb)
7d072b404 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Arend van
Spriel 2015-05-26 1148 return;
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1149
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1150 if (data_offset)
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1151 skb_pull(skb, data_offset);
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1152 else if (msgbuf->rx_dataoffset)
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman 2014-07-30 1153 skb_pull(skb, msgbuf->rx_dataoffset);
9a1bb6025 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c Hante
Meuleman
[bug report] wlcore: sdio: check for valid platform device data before suspend
Hello Eyal Reizer,
The patch 6e91d48371e7: "wlcore: sdio: check for valid platform
device data before suspend" from May 28, 2018, leads to the following
static checker warning:
drivers/net/wireless/ti/wlcore/sdio.c:404 wl1271_suspend()
warn: missing error code here? 'platform_get_drvdata()' failed. 'ret' =
'0'
drivers/net/wireless/ti/wlcore/sdio.c
391 #ifdef CONFIG_PM
392 static int wl1271_suspend(struct device *dev)
393 {
394 /* Tell MMC/SDIO core it's OK to power down the card
395 * (if it isn't already), but not to remove it completely */
396 struct sdio_func *func = dev_to_sdio_func(dev);
397 struct wl12xx_sdio_glue *glue = sdio_get_drvdata(func);
398 struct wl1271 *wl = platform_get_drvdata(glue->core);
399 mmc_pm_flag_t sdio_flags;
400 int ret = 0;
401
402 if (!wl) {
403 dev_err(dev, "no wilink module was probed\n");
404 goto out;
We should set -ENOMEM or something?
405 }
406
407 dev_dbg(dev, "wl1271 suspend. wow_enabled: %d\n",
408 wl->wow_enabled);
409
410 /* check whether sdio should keep power */
411 if (wl->wow_enabled) {
412 sdio_flags = sdio_get_host_pm_caps(func);
413
414 if (!(sdio_flags & MMC_PM_KEEP_POWER)) {
415 dev_err(dev, "can't keep power while host "
416 "is suspended\n");
417 ret = -EINVAL;
418 goto out;
419 }
420
421 /* keep power while host suspended */
422 ret = sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
423 if (ret) {
424 dev_err(dev, "error while trying to keep
power\n");
425 goto out;
426 }
427 }
428 out:
429 return ret;
430 }
regards,
dan carpenter
[PATCH] rndis_wlan: potential buffer overflow in rndis_wlan_auth_indication()
This is a static checker fix, not something I have tested. The issue
is that on the second iteration through the loop, we jump forward by
le32_to_cpu(auth_req->length) bytes. The problem is that if the length
is more than "buflen" then we end up with a negative "buflen". A
negative buflen is type promoted to a high positive value and the loop
continues but it's accessing beyond the end of the buffer.
I believe the "auth_req->length" comes from the firmware and if the
firmware is malicious or buggy, you're already toasted so the impact of
this bug is probably not very severe.
Fixes: 030645aceb3d ("rndis_wlan: handle 802.11 indications from device")
Signed-off-by: Dan Carpenter
diff --git a/drivers/net/wireless/rndis_wlan.c
b/drivers/net/wireless/rndis_wlan.c
index 9935bd09db1f..d4947e3a909e 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -2928,6 +2928,8 @@ static void rndis_wlan_auth_indication(struct usbnet
*usbdev,
while (buflen >= sizeof(*auth_req)) {
auth_req = (void *)buf;
+ if (buflen < le32_to_cpu(auth_req->length))
+ return;
type = "unknown";
flags = le32_to_cpu(auth_req->flags);
pairwise_error = false;
[bug report] mwifiex: process rxba_sync event
Hello Xinming Hu,
The patch 99ffe72cdae4: "mwifiex: process rxba_sync event" from Jul
25, 2016, leads to the following static checker warning:
drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:941
mwifiex_11n_rxba_sync_event()
warn: 'tlv_buf_left' can be negative (type promoted to high)
drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
927 void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv,
928 u8 *event_buf, u16 len)
929 {
930 struct mwifiex_ie_types_rxba_sync *tlv_rxba = (void *)event_buf;
931 u16 tlv_type, tlv_len;
932 struct mwifiex_rx_reorder_tbl *rx_reor_tbl_ptr;
933 u8 i, j;
934 u16 seq_num, tlv_seq_num, tlv_bitmap_len;
935 int tlv_buf_left = len;
936 int ret;
937 u8 *tmp;
938
939 mwifiex_dbg_dump(priv->adapter, EVT_D, "RXBA_SYNC event:",
940 event_buf, len);
941 while (tlv_buf_left >= sizeof(*tlv_rxba)) {
942 tlv_type = le16_to_cpu(tlv_rxba->header.type);
943 tlv_len = le16_to_cpu(tlv_rxba->header.len);
944 if (tlv_type != TLV_TYPE_RXBA_SYNC) {
945 mwifiex_dbg(priv->adapter, ERROR,
946 "Wrong TLV id=0x%x\n", tlv_type);
947 return;
948 }
949
950 tlv_seq_num = le16_to_cpu(tlv_rxba->seq_num);
951 tlv_bitmap_len = le16_to_cpu(tlv_rxba->bitmap_len);
952 mwifiex_dbg(priv->adapter, INFO,
953 "%pM tid=%d seq_num=%d bitmap_len=%d\n",
954 tlv_rxba->mac, tlv_rxba->tid, tlv_seq_num,
955 tlv_bitmap_len);
956
957 rx_reor_tbl_ptr =
958 mwifiex_11n_get_rx_reorder_tbl(priv,
tlv_rxba->tid,
959 tlv_rxba->mac);
960 if (!rx_reor_tbl_ptr) {
961 mwifiex_dbg(priv->adapter, ERROR,
962 "Can not find rx_reorder_tbl!");
963 return;
964 }
965
966 for (i = 0; i < tlv_bitmap_len; i++) {
967 for (j = 0 ; j < 8; j++) {
968 if (tlv_rxba->bitmap[i] & (1 << j)) {
969 seq_num = (MAX_TID_VALUE - 1) &
970 (tlv_seq_num + i * 8 +
j);
971
972 mwifiex_dbg(priv->adapter,
ERROR,
973 "drop
packet,seq=%d\n",
974 seq_num);
975
976 ret = mwifiex_11n_rx_reorder_pkt
977 (priv, seq_num, tlv_rxba->tid,
978 tlv_rxba->mac, 0, NULL);
979
980 if (ret)
981
mwifiex_dbg(priv->adapter,
982 ERROR,
983 "Fail to
drop packet");
984 }
985 }
986 }
987
988 tlv_buf_left -= (sizeof(*tlv_rxba) + tlv_len);
^^
This could theoretically underflow to negative. We should probably
check "sizeof(*tlv_rxba) + tlv_len" at the start of the loop and
tlv_bitmap_len as well to avoid a theoretical read beyond the end of
the array.
989 tmp = (u8 *)tlv_rxba + tlv_len + sizeof(*tlv_rxba);
990 tlv_rxba = (struct mwifiex_ie_types_rxba_sync *)tmp;
991 }
992 }
regards,
dan carpenter
[bug report] ath9k: Fix issue with MCS15
Hello Sujith Manoharan,
The patch 6fcbe538be43: "ath9k: Fix issue with MCS15" from Nov 14,
2013, leads to the following static checker warning:
drivers/net/wireless/ath/ath9k/ar9003_phy.c:836 ar9003_doubler_fix()
warn: 'ah->hw_version.macVersion == 448' 'false' implies
'ah->hw_version.macVersion == 448' is 'false'
drivers/net/wireless/ath/ath9k/ar9003_phy.c
834 static void ar9003_doubler_fix(struct ath_hw *ah)
835 {
836 if (AR_SREV_9300(ah) || AR_SREV_9580(ah) || AR_SREV_9550(ah)) {
The problem is that both AR_SREV_VERSION_9300 and AR_SREV_VERSION_9580
are 0x1c0 so the test for AR_SREV_9580() is duplicative.
837 REG_RMW(ah, AR_PHY_65NM_CH0_RXTX2,
838 1 << AR_PHY_65NM_CH0_RXTX2_SYNTHON_MASK_S |
839 1 << AR_PHY_65NM_CH0_RXTX2_SYNTHOVR_MASK_S, 0);
840 REG_RMW(ah, AR_PHY_65NM_CH1_RXTX2,
841 1 << AR_PHY_65NM_CH0_RXTX2_SYNTHON_MASK_S |
842 1 << AR_PHY_65NM_CH0_RXTX2_SYNTHOVR_MASK_S, 0);
843 REG_RMW(ah, AR_PHY_65NM_CH2_RXTX2,
844 1 << AR_PHY_65NM_CH0_RXTX2_SYNTHON_MASK_S |
845 1 << AR_PHY_65NM_CH0_RXTX2_SYNTHOVR_MASK_S, 0);
846
847 udelay(200);
848
regards,
dan carpenter
[bug report] mwifiex: add rx histogram statistics support
Hello Xinming Hu,
The patch cbf6e05527a7: "mwifiex: add rx histogram statistics
support" from Dec 23, 2014, leads to the following static checker
warning:
drivers/net/wireless/marvell/mwifiex/util.c:714 mwifiex_hist_data_set()
error: buffer underflow 'phist_data->snr' '(-128)-127'
drivers/net/wireless/marvell/mwifiex/util.c
706 /* function to add histogram record */
707 void mwifiex_hist_data_set(struct mwifiex_private *priv, u8 rx_rate, s8
snr,
^^
708 s8 nflr)
709 {
710 struct mwifiex_histogram_data *phist_data = priv->hist_data;
711
712 atomic_inc(_data->num_samples);
713 atomic_inc(_data->rx_rate[rx_rate]);
714 atomic_inc(_data->snr[snr]);
715 atomic_inc(_data->noise_flr[128 + nflr]);
716 atomic_inc(_data->sig_str[nflr - snr]);
Smatch complains that "snr" comes from skb->data so it's untrusted and
it can be less than zero and underflow the ->snr array.
->snr, ->noise_flr and ->sig_str all have 256 elements. Obviously it
seems like "snr" should be declared as a u8 instead of an s8. But I'm
not totally sure what to do about the ->noise_flr and ->sig_str[]
arrays.
717 }
regards,
dan carpenter
[PATCH] cfg80211: Use correct GFP_ mask in cfg80211_del_sta_sinfo()
Smatch complains that we should use the passed in "gfp" instead of hard
coding GFP_KERNEL. I looked at some of the callers and this would
probably be a bug for rtw_cfg80211_indicate_sta_disassoc() which uses
GFP_ATOMIC and a NULL "sinfo".
Fixes: 52539ca89f36 ("cfg80211: Expose TXQ stats and parameters to userspace")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index afbe5105bf7f..3eb645b81777 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -14644,7 +14644,7 @@ void cfg80211_del_sta_sinfo(struct net_device *dev,
const u8 *mac_addr,
struct station_info *empty_sinfo = NULL;
if (!sinfo) {
- empty_sinfo = kzalloc(sizeof(*empty_sinfo), GFP_KERNEL);
+ empty_sinfo = kzalloc(sizeof(*empty_sinfo), gfp);
if (!empty_sinfo)
return;
sinfo = empty_sinfo;
Re: [PATCH 24/30] staging: wilc1000: refactor del_station() to avoid parenthesis misalignment
I feel sort of bad complaining about this patchset when your co-workers
already nit picked it to death... :P
On Mon, May 07, 2018 at 02:13:28PM +0530, Ajay Singh wrote:
> Refactor the code to fix open parenthesis alignment issue reported by
> checkpatch.pl script in del_station().
I no idea what an "open parenthesis alignment issue" is. It's sort of
surprising because I deal with checkpatch patches a lot.
>
> Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> ---
> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 4600f4a..7f49d60 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -1997,6 +1997,7 @@ static int del_station(struct wiphy *wiphy, struct
> net_device *dev,
> s32 ret = 0;
> struct wilc_priv *priv;
> struct wilc_vif *vif;
> + struct sta_info *info;
>
> if (!wiphy)
> return -EFAULT;
> @@ -2004,16 +2005,17 @@ static int del_station(struct wiphy *wiphy, struct
> net_device *dev,
> priv = wiphy_priv(wiphy);
> vif = netdev_priv(dev);
>
> - if (vif->iftype == AP_MODE || vif->iftype == GO_MODE) {
> - if (!mac)
> - ret = wilc_del_allstation(vif,
> - priv->assoc_stainfo.sta_associated_bss);
> + if (!(vif->iftype == AP_MODE || vif->iftype == GO_MODE))
I feel like this is better as:
if (vif->iftype != AP_MODE && vif->iftype != GO_MODE)
> + return ret;
What is "ret" here? I haven't looked at this patch in context, but
it's probably zero. Just return the literal.
regards,
dan carpenter
Re: [PATCH 03/30] staging: wilc1000: fix line over 80 chars in handle_key()
On Thu, May 10, 2018 at 08:21:52AM +0300, Claudiu Beznea wrote: > >>> + wid.val = (s8 *)key_buf; > >> > >> Is this cast really needed? > >> > > > > This patch is only to address line over 80 chars checkpatch issue. > > It is not good to club these change with type cast related > > modification. For removing unnecessary cast we can submit > > a separate patch series. > > These are my views. What do you think? > > > > I'm ok with this. I was thinking that since you introduced this new function > you may want to also address this, if any. No. Please don't. That kind of thing is sort of an unrelated change from just moving the lines around and it messes up my review scripts. It's really really easy to review the changes when they're split into multiple chunks. regards, dan carpenter
[bug report] Add ath6kl cleaned up driver
Hello Kalle Valo,
The patch bdcd81707973: "Add ath6kl cleaned up driver" from Jul 18,
2011, leads to the following static checker warning:
drivers/net/wireless/ath/ath6kl/wmi.c:1189
ath6kl_wmi_pstream_timeout_event_rx()
error: buffer overflow 'wmi->stream_exist_for_ac' 4 <= 255
user_rl='0-255'
drivers/net/wireless/ath/ath6kl/wmi.c
1171 /* Inactivity timeout of a fatpipe(pstream) at the target */
1172 static int ath6kl_wmi_pstream_timeout_event_rx(struct wmi *wmi, u8
*datap,
1173 int len)
1174 {
1175 struct wmi_pstream_timeout_event *ev;
1176
1177 if (len < sizeof(struct wmi_pstream_timeout_event))
1178 return -EINVAL;
1179
1180 ev = (struct wmi_pstream_timeout_event *) datap;
^^^
Smatch distrusts "datap" because it comes from skb->data on the recieve
path.
1181
1182 /*
1183 * When the pstream (fat pipe == AC) timesout, it means there
were
1184 * no thinStreams within this pstream & it got implicitly
created
1185 * due to data flow on this AC. We start the inactivity timer
only
1186 * for implicitly created pstream. Just reset the host state.
1187 */
1188 spin_lock_bh(>lock);
1189 wmi->stream_exist_for_ac[ev->traffic_class] = 0;
^
How do we know it's less than 4?
1190 wmi->fat_pipe_exist &= ~(1 << ev->traffic_class);
1191 spin_unlock_bh(>lock);
1192
1193 /* Indicate inactivity to driver layer for this fatpipe
(pstream) */
1194 ath6kl_indicate_tx_activity(wmi->parent_dev, ev->traffic_class,
false);
1195
1196 return 0;
1197 }
regards,
dan carpenter
[bug report] mwifiex: do le_to_cpu conversion for Rx packet header elements
Hello Amitkumar Karwar,
The patch ed1ea6f42ece: "mwifiex: do le_to_cpu conversion for Rx
packet header elements" from Aug 3, 2012, leads to the following
static checker warning:
drivers/net/wireless/marvell/mwifiex/sta_rx.c:251
mwifiex_process_sta_rx_packet()
error: buffer overflow 'priv->rx_seq' 8 <= 255 user_rl='0-255'
drivers/net/wireless/marvell/mwifiex/sta_rx.c
187 int mwifiex_process_sta_rx_packet(struct mwifiex_private *priv,
188struct sk_buff *skb)
189 {
190 struct mwifiex_adapter *adapter = priv->adapter;
191 int ret = 0;
192 struct rxpd *local_rx_pd;
193 struct rx_packet_hdr *rx_pkt_hdr;
194 u8 ta[ETH_ALEN];
195 u16 rx_pkt_type, rx_pkt_offset, rx_pkt_length, seq_num;
196 struct mwifiex_sta_node *sta_ptr;
197
198 local_rx_pd = (struct rxpd *) (skb->data);
^
Smatch distructs skb->data like the plague.
199 rx_pkt_type = le16_to_cpu(local_rx_pd->rx_pkt_type);
200 rx_pkt_offset = le16_to_cpu(local_rx_pd->rx_pkt_offset);
201 rx_pkt_length = le16_to_cpu(local_rx_pd->rx_pkt_length);
202 seq_num = le16_to_cpu(local_rx_pd->seq_num);
203
204 rx_pkt_hdr = (void *)local_rx_pd + rx_pkt_offset;
205
206 if ((rx_pkt_offset + rx_pkt_length) > (u16) skb->len) {
207 mwifiex_dbg(adapter, ERROR,
208 "wrong rx packet: len=%d, rx_pkt_offset=%d,
rx_pkt_length=%d\n",
209 skb->len, rx_pkt_offset, rx_pkt_length);
210 priv->stats.rx_dropped++;
211 dev_kfree_skb_any(skb);
212 return ret;
213 }
214
215 if (rx_pkt_type == PKT_TYPE_MGMT) {
216 ret = mwifiex_process_mgmt_packet(priv, skb);
217 if (ret)
218 mwifiex_dbg(adapter, DATA, "Rx of mgmt packet
failed");
219 dev_kfree_skb_any(skb);
220 return ret;
221 }
222
223 /*
224 * If the packet is not an unicast packet then send the packet
225 * directly to os. Don't pass thru rx reordering
226 */
227 if ((!IS_11N_ENABLED(priv) &&
228 !(ISSUPP_TDLS_ENABLED(priv->adapter->fw_cap_info) &&
229 !(local_rx_pd->flags & MWIFIEX_RXPD_FLAGS_TDLS_PACKET)))
||
230 !ether_addr_equal_unaligned(priv->curr_addr,
rx_pkt_hdr->eth803_hdr.h_dest)) {
231 mwifiex_process_rx_packet(priv, skb);
232 return ret;
233 }
234
235 if (mwifiex_queuing_ra_based(priv) ||
236 (ISSUPP_TDLS_ENABLED(priv->adapter->fw_cap_info) &&
237 local_rx_pd->flags & MWIFIEX_RXPD_FLAGS_TDLS_PACKET)) {
238 memcpy(ta, rx_pkt_hdr->eth803_hdr.h_source, ETH_ALEN);
239 if (local_rx_pd->flags & MWIFIEX_RXPD_FLAGS_TDLS_PACKET
&&
240 local_rx_pd->priority < MAX_NUM_TID) {
^^^
We check ->priority here.
241 sta_ptr = mwifiex_get_sta_entry(priv, ta);
242 if (sta_ptr)
243 sta_ptr->rx_seq[local_rx_pd->priority] =
244
le16_to_cpu(local_rx_pd->seq_num);
245 mwifiex_auto_tdls_update_peer_signal(priv, ta,
246
local_rx_pd->snr,
247
local_rx_pd->nf);
248 }
249 } else {
250 if (rx_pkt_type != PKT_TYPE_BAR)
251 priv->rx_seq[local_rx_pd->priority] = seq_num;
^^^
But not on this path.
252 memcpy(ta,
priv->curr_bss_params.bss_descriptor.mac_address,
253 ETH_ALEN);
254 }
255
256 /* Reorder and send to OS */
257 ret = mwifiex_11n_rx_reorder_pkt(priv, seq_num,
local_rx_pd->priority,
258 ta, (u8) rx_pkt_type, skb);
259
regards,
dan carpenter
Re: [PATCH] staging: wilc1000: fix infinite loop and out-of-bounds access
We're mainly discussing readability, right? To me when people use "int" that tells me as a reader that we don't need to think about the type. It's going to be a small number. Say you have data which the user can control, then it's super important to focus on the data types. We don't focus on it enough. There is some kind of idea that good developers should just be super focused on everything all the time, but I don't think humans can do it. So to me it's useful when the author tells me, "This an int type. It's fine. This is not critical." If you make request->n_ssids a u8 or u16, that isn't going to save any memory because the struct is padded. You'd also need to audit a bunch of code to make sure that we don't overflow the u16. If you wanted to overflow the int, you'd need to allocate several gigs of memory but kmalloc() is capped at KMALLOC_MAX_SIZE (4MB) so that's not possible. How many of these structs do we allocate? Is it really worth optimizing the heck out of it? There are times where want to be very deliberate with our types because we're dealing the large numbers, or user data or fast paths. But there are other times where int is fine... regards, dan carpenter
Re: [PATCH] staging: wilc1000: fix infinite loop and out-of-bounds access
On Mon, Apr 30, 2018 at 07:59:16PM +0530, Ajay Singh wrote:
> Reviewed-by: Ajay Singh <ajay.kat...@microchip.com>
>
> On Mon, 30 Apr 2018 07:50:40 -0500
> "Gustavo A. R. Silva" <gust...@embeddedor.com> wrote:
>
> > If i < slot_id is initially true then it will remain true. Also,
> > as i is being decremented it will end up accessing memory out of
> > bounds.
> >
> > Fix this by incrementing *i* instead of decrementing it.
>
> Nice catch!
> Thanks for submitting the changes.
>
> >
> > Addresses-Coverity-ID: 1468454 ("Infinite loop")
> > Fixes: faa657641081 ("staging: wilc1000: refactor scan() to free
> > kmalloc memory on failure cases")
> > Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
> > ---
> >
> > BTW... at first sight it seems to me that variables slot_id
> > and i should be of type unsigned instead of signed.
>
> Yes, 'slot_id' & 'i' can be changed to unsigned int.
>
A lot of people think making things unsigned improves the code but I
hate "unsigned int i"... I understand that in certain cases we do loop
more than INT_MAX but those are a tiny tiny minority of loops.
Simple types like "int" are more readable than complicated types like
"unsigned int". Unsigned int just draws attention to itself and
distracts people from things that might potentially matter. We have
real life subtle loops like in xtea_encrypt() where we need to use
unsigned types.
And look at the function we're talking about here:
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
577 static inline int
578 wilc_wfi_cfg_alloc_fill_ssid(struct cfg80211_scan_request *request,
579 struct hidden_network *ntwk)
580 {
581 int i;
582 int slot_id = 0;
583
584 ntwk->net_info = kcalloc(request->n_ssids,
585 sizeof(struct hidden_network),
GFP_KERNEL);
586 if (!ntwk->net_info)
587 goto out;
588
589 ntwk->n_ssids = request->n_ssids;
590
591 for (i = 0; i < request->n_ssids; i++) {
request->n_ssids is an int. It can't possibly be INT_MAX because the
kcalloc() will fail. Ideally the static analysis tool should be able to
tell you that if you seed it with the knowledge of how much memory it's
possible to kmalloc(). So it's just laziness here is why the static
checker complains, it should see there is no issue. Smatch fails here
as well but I'll see if I can fix it.
Anyway let's say it could be negative then 0 is greater than negative
values so the loop would be a no-op. I've seen code where the user
could set the loop bounds to s32min-4 but because it was "int i" instead
of "unsigned int i" then it meant the loop was a no-op instead of being
a security problem. In other words, unsigned can be less secure.
I honestly have never seen a bug in the kernel where we intended to loop
more than INT_MAX times, but there was a signedness bug preventing it.
That's the only reason I can see to change the signedness. Am I missing
something?
592 if (request->ssids[i].ssid_len > 0) {
593 struct hidden_net_info *info =
>net_info[slot_id];
594
595 info->ssid = kmemdup(request->ssids[i].ssid,
596 request->ssids[i].ssid_len,
597 GFP_KERNEL);
598 if (!info->ssid)
599 goto out_free;
600
601 info->ssid_len = request->ssids[i].ssid_len;
602 slot_id++;
603 } else {
604 ntwk->n_ssids -= 1;
605 }
606 }
607 return 0;
608
609 out_free:
610
611 for (i = 0; i < slot_id ; i--)
612 kfree(ntwk->net_info[i].ssid);
613
614 kfree(ntwk->net_info);
615 out:
616
617 return -ENOMEM;
618 }
regards,
dan carpenter
Re: [PATCH] rsi: fix a bug in rsi_hal_key_config()
On Fri, Apr 27, 2018 at 02:32:20PM +0300, Kalle Valo wrote: > > Gustavo submitted an identical patch also for this one :) > > https://patchwork.kernel.org/patch/10365997/ > Hey Gustavo, We keep on sending duplicate patches. Most of the static checker people CC kernel-janit...@vger.kernel.org so we can see what's already been sent. regards, dan carpenter
[PATCH] rsi: fix a bug in rsi_hal_key_config()
Smatch complains that the end of this function is dead code. I'm pretty
sure that this return needs to be changed to only return on error.
Fixes: 4fd6c4762f37 ("rsi: roaming enhancements")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
index 766d874cc6e2..80e7f4f4f188 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -911,14 +911,14 @@ static int rsi_hal_key_config(struct ieee80211_hw *hw,
}
}
- return rsi_hal_load_key(adapter->priv,
- key->key,
- key->keylen,
- key_type,
- key->keyidx,
- key->cipher,
- sta_id,
- vif);
+ status = rsi_hal_load_key(adapter->priv,
+ key->key,
+ key->keylen,
+ key_type,
+ key->keyidx,
+ key->cipher,
+ sta_id,
+ vif);
if (status)
return status;
[PATCH] rsi: Uninitialized return value in rsi_reset_card()
If rsi_usb_master_reg_write() fails then "ret" hasn't been initialized.
Fixes: 16d3bb7b2f37 ("rsi: disable fw watchdog timer during reset")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c
b/drivers/net/wireless/rsi/rsi_91x_usb.c
index b065438f51b2..6ce6b754df12 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -687,9 +687,10 @@ static int rsi_reset_card(struct rsi_hw *adapter)
*/
msleep(100);
- if (rsi_usb_master_reg_write(adapter, SWBL_REGOUT,
-RSI_FW_WDT_DISABLE_REQ,
-RSI_COMMON_REG_SIZE) < 0) {
+ ret = rsi_usb_master_reg_write(adapter, SWBL_REGOUT,
+ RSI_FW_WDT_DISABLE_REQ,
+ RSI_COMMON_REG_SIZE);
+ if (ret < 0) {
rsi_dbg(ERR_ZONE, "Disabling firmware watchdog timer failed\n");
goto fail;
}
[PATCH v2] rsi: remove unecessary PTR_ALIGN()s
The issue here is that we allocate "data" and then set
"data = PTR_ALIGN(data, 8);" and then we free the aligned pointer
instead of the original pointer.
kmalloc() pointers are already ARCH_SLAB_MINALIGN aligned which is 8 or
more on everything except certain Xtensa variants. We decided that if
the Xtensa people ever notice a bug here then we'll tell them the bug is
on their side. ;)
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
v2: Instead of saving the original pointer, just remove the ALIGN()s
diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c
b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index d76e69c0beaa..8ef00582c6ea 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -660,8 +660,6 @@ static int rsi_sdio_master_reg_read(struct rsi_hw *adapter,
u32 addr,
if (!data)
return -ENOMEM;
- data = PTR_ALIGN(data, 8);
-
ms_addr = (addr >> 16);
status = rsi_sdio_master_access_msword(adapter, ms_addr);
if (status < 0) {
@@ -724,8 +722,6 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
if (!data_aligned)
return -ENOMEM;
- data_aligned = PTR_ALIGN(data_aligned, 8);
-
if (size == 2) {
*data_aligned = ((data << 16) | (data & 0x));
} else if (size == 1) {
Re: [PATCH] rsi: Free the unaligned pointer
On Thu, Apr 05, 2018 at 02:39:31PM +0300, Dan Carpenter wrote: > On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote: > > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote: > > > The problem here is that we allocate "data". Then we do > > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and > > > not the one we allocated. > > > > That seems pretty pointless, since kmalloc guarantees such alignment for > > sure. Better to just remove PTR_ALIGN()? > > Yeah. You're probably right. I was thinking that maybe > ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I > think it's always 8 or more. > Perhaps on certain xtensa variants? arch/xtensa/include/asm/processor.h:#define ARCH_SLAB_MINALIGN XCHAL_DATA_WIDTH arch/xtensa/variants/fsf/include/variant/core.h:#define XCHAL_DATA_WIDTH 4 /* data width in bytes */ regards, dan carpenter
Re: [PATCH] rsi: Free the unaligned pointer
On Thu, Apr 05, 2018 at 01:30:35PM +0200, Johannes Berg wrote: > On Thu, 2018-04-05 at 14:23 +0300, Dan Carpenter wrote: > > The problem here is that we allocate "data". Then we do > > "data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and > > not the one we allocated. > > That seems pretty pointless, since kmalloc guarantees such alignment for > sure. Better to just remove PTR_ALIGN()? Yeah. You're probably right. I was thinking that maybe ARCH_SLAB_MINALIGN was smaller than 8 somewhere but look it it now, I think it's always 8 or more. Let me resend with the ALIGN() removed. regards, dan carpenter
[PATCH] rsi: Free the unaligned pointer
The problem here is that we allocate "data". Then we do
"data = PTR_ALIGN(data, 8);" and then we free the aligned pointer and
not the one we allocated.
I don't know if it causes an issue in real life, but it seems like a
reasonable thing to free the same pointer that we allocated.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c
b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index d76e69c0beaa..ca4e698ab69b 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -652,11 +652,11 @@ static int rsi_sdio_load_data_master_write(struct rsi_hw
*adapter,
static int rsi_sdio_master_reg_read(struct rsi_hw *adapter, u32 addr,
u32 *read_buf, u16 size)
{
- u32 addr_on_bus, *data;
+ u32 addr_on_bus, *data, *data_orig;
u16 ms_addr;
int status;
- data = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
+ data = data_orig = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
if (!data)
return -ENOMEM;
@@ -709,7 +709,7 @@ static int rsi_sdio_master_reg_read(struct rsi_hw *adapter,
u32 addr,
}
err:
- kfree(data);
+ kfree(data_orig);
return status;
}
@@ -717,10 +717,10 @@ static int rsi_sdio_master_reg_write(struct rsi_hw
*adapter,
unsigned long addr,
unsigned long data, u16 size)
{
- unsigned long *data_aligned;
+ unsigned long *data_aligned, *data_orig;
int status;
- data_aligned = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
+ data_aligned = data_orig = kzalloc(RSI_MASTER_REG_BUF_SIZE, GFP_KERNEL);
if (!data_aligned)
return -ENOMEM;
@@ -743,7 +743,7 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
rsi_dbg(ERR_ZONE,
"%s: Unable to set ms word to common reg\n",
__func__);
- kfree(data_aligned);
+ kfree(data_orig);
return -EIO;
}
addr = addr & 0x;
@@ -757,7 +757,7 @@ static int rsi_sdio_master_reg_write(struct rsi_hw *adapter,
rsi_dbg(ERR_ZONE,
"%s: Unable to do AHB reg write\n", __func__);
- kfree(data_aligned);
+ kfree(data_orig);
return status;
}
[PATCH] mwifiex: pcie: tighten a check in mwifiex_pcie_process_event_ready()
If "evt_len" is 1 then we try to memcpy() negative 3 bytes and it would
cause memory corruption.
Fixes: d930faee141b ("mwifiex: add support for Marvell pcie8766 chipset")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 97a6199692ab..7538543d46fa 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -1881,7 +1881,8 @@ static int mwifiex_pcie_process_event_ready(struct
mwifiex_adapter *adapter)
mwifiex_dbg(adapter, EVENT,
"info: Event length: %d\n", evt_len);
- if ((evt_len > 0) && (evt_len < MAX_EVENT_SIZE))
+ if (evt_len > MWIFIEX_EVENT_HEADER_LEN &&
+ evt_len < MAX_EVENT_SIZE)
memcpy(adapter->event_body, skb_cmd->data +
MWIFIEX_EVENT_HEADER_LEN, evt_len -
MWIFIEX_EVENT_HEADER_LEN);
Re: [PATCH 9/9] staging: wilc1000: free memory allocated for general info message from firmware
On Mon, Mar 26, 2018 at 05:01:50PM +0530, Ajay Singh wrote: > On Mon, 26 Mar 2018 11:32:41 +0300 > Dan Carpenter <dan.carpen...@oracle.com> wrote: > > > What happened to patch 8/9? Anyway, I can't apply this patch and it > > could be my fault or it could be the missing patch. I don't know... > > I rechecked by applying the patches in order and didn't face any conflict. > I am going to send the v2 for this patch series by including the review > comments. The problem was on my end. Sorry. Gmail's spam filtering messed up. I should have checked better. regards, dan carpenter
Re: [PATCH 9/9] staging: wilc1000: free memory allocated for general info message from firmware
What happened to patch 8/9? Anyway, I can't apply this patch and it could be my fault or it could be the missing patch. I don't know... Anwyway, seems like a nice patchset. regards, dan carpenter
Re: [PATCH 4/9] staging: wilc1000: free memory allocated in add wep key functions
On Fri, Mar 23, 2018 at 08:38:53PM +0530, Ajay Singh wrote:
> Free memory allocated for wep key when command enqueue is failed.
>
> Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> ---
> drivers/staging/wilc1000/host_interface.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/host_interface.c
> b/drivers/staging/wilc1000/host_interface.c
> index 1cc4c08..c958dd3 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -2727,8 +2727,10 @@ int wilc_add_wep_key_bss_sta(struct wilc_vif *vif,
> const u8 *key, u8 len,
> msg.body.key_info.attr.wep.index = index;
>
> result = wilc_enqueue_cmd();
> - if (result)
> + if (result) {
> netdev_err(vif->ndev, "STA - WEP Key\n");
> + kfree(msg.body.key_info.attr.wep.key);
We should "return result;" here otherwise we'll hang when we
wait_for_completion(). This is the sort of bug why I always encourage
people to keep the error path and success path separate (unless they
both have to unlock or free the same resources).
> + }
> wait_for_completion(_drv->comp_test_key_block);
>
> return result;
That way this becomes a "return 0;" instead of a "return result;".
> @@ -2763,10 +2765,12 @@ int wilc_add_wep_key_bss_ap(struct wilc_vif *vif,
> const u8 *key, u8 len,
>
> result = wilc_enqueue_cmd();
>
> - if (result)
> + if (result) {
> netdev_err(vif->ndev, "AP - WEP Key\n");
> - else
> + kfree(msg.body.key_info.attr.wep.key);
> + } else {
> wait_for_completion(_drv->comp_test_key_block);
> + }
>
> return result;
> }
This code works, but it would look cleaner with "return result;".
result = wilc_enqueue_cmd();
if (result) {
netdev_err(vif->ndev, "AP - WEP Key\n");
kfree(msg.body.key_info.attr.wep.key);
return result;
}
wait_for_completion(_drv->comp_test_key_block);
return 0;
I removed a blank line between the wilc_enqueue_cmd() and the error
handling because they're very connected. All the success path is at
indent level one so you can just glance at the function and see what
it's supposed to do in the normal case. The error handling is self
contained at indent level two.
regards,
dan carpenter
Re: [PATCH 00/11] staging: wilc1000: fix for checkpatch and handled malloc memory properly
These look good. I've reviewed them all. Reviewed-by: Dan Carpenter <dan.carpen...@oracle.com> I had some small process complaints but it doesn't make life easier for me if you resend them and I have to review everything twice :P regards, dan carpenter
Re: [PATCH 09/11] staging: wilc1000: remove line over 80 char in cfg_connect_result()
On Tue, Mar 20, 2018 at 10:25:42PM +0530, Ajay Singh wrote:
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 1b6fe64..af1b8aa 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -460,6 +460,17 @@ static void cfg_scan_result(enum scan_event scan_event,
> }
> }
>
> +static inline bool wilc_wfi_cfg_scan_time_expired(int i)
"i" is the wrong parameter to pass. The name is not useful. Probably
the right parameter is either _scanned_shadow[i] or
last_scanned_shadow[i].time_scan_cached.
> +{
> + unsigned long now = jiffies;
> +
> + if (time_after(now, last_scanned_shadow[i].time_scan_cached +
> +(unsigned long)(nl80211_SCAN_RESULT_EXPIRE - (1 * HZ
> + return true;
> + else
> + return false;
Also I think it you apply this patch and then run checkpatch.pl --strict
against the file it will complain that it should be:
if (time_after(now, last_scanned_shadow[i].time_scan_cached +
(unsigned long)(nl80211_SCAN_RESULT_EXPIRE - (1 * HZ
return true;
return false;
> +}
> +
> int wilc_connecting;
>
> static void cfg_connect_result(enum conn_event conn_disconn_evt,
> @@ -505,17 +516,14 @@ static void cfg_connect_result(enum conn_event
> conn_disconn_evt,
> bool scan_refresh = false;
> u32 i;
>
> - memcpy(priv->associated_bss, conn_info->bssid,
> ETH_ALEN);
> + memcpy(priv->associated_bss, conn_info->bssid,
> +ETH_ALEN);
>
It basically always helps me if the "new function" changes are in a
patch by themselves. These lines are a pure white space changes and I
have a script that reviews those for me instantly, but when it's mixed
with this patch I have to do it by hand.
regards,
dan carpenter
Re: [PATCH 06/11] staging: wilc1000: refactor mgmt_tx to fix line over 80 chars
This would have been easier for me if it were split up slightly
different again.
This patch is fine. I have a couple comments for future patches which
you are free to ignore if you want because it's mostly just my personal
taste.
On Tue, Mar 20, 2018 at 10:25:39PM +0530, Ajay Singh wrote:
> + if (subtype != P2P_INV_REQ && subtype != P2P_INV_RSP) {
> + int vendor_spec_len = sizeof(p2p_vendor_spec);
I'm not a huge fan of making shorter names for sizeof()s just to get
around the 80 character rule. The 80 character rule is ultimately
supposed to make the code more readable, and this is making less
readable so it's maybe better to just ignore the rule.
> +
> + memcpy(_tx->buff[len], p2p_vendor_spec,
> +vendor_spec_len);
> + mgmt_tx->buff[len + vendor_spec_len] = p2p_local_random;
> + mgmt_tx->size = buf_len;
> + }
> + }
> +}
> +
> static int mgmt_tx(struct wiphy *wiphy,
> struct wireless_dev *wdev,
> struct cfg80211_mgmt_tx_params *params,
> @@ -1568,9 +1620,9 @@ static int mgmt_tx(struct wiphy *wiphy,
> struct p2p_mgmt_data *mgmt_tx;
> struct wilc_priv *priv;
> struct host_if_drv *wfi_drv;
> - u32 i;
> struct wilc_vif *vif;
> u32 buf_len = len + sizeof(p2p_vendor_spec) + sizeof(p2p_local_random);
> + int ret = 0;
>
> vif = netdev_priv(wdev->netdev);
> priv = wiphy_priv(wiphy);
> @@ -1580,92 +1632,75 @@ static int mgmt_tx(struct wiphy *wiphy,
> priv->tx_cookie = *cookie;
> mgmt = (const struct ieee80211_mgmt *)buf;
>
> - if (ieee80211_is_mgmt(mgmt->frame_control)) {
> - mgmt_tx = kmalloc(sizeof(struct p2p_mgmt_data), GFP_KERNEL);
> - if (!mgmt_tx)
> - return -EFAULT;
> + if (!ieee80211_is_mgmt(mgmt->frame_control))
> + goto out;
I hate this "goto out;". My first question when I see it is "what does
goto out; do?" It's a kind of vague label name. So I have to scroll
down to the bottom of the function. And then I'm like "Oh, it doesn't
do anything. Well that was a waste of time." And then it occurs to me,
"Huh, what is 'ret' set to?" So now I have to scroll all the way to the
very start of the function... All this scrolling could be avoided if we
just did a direct "return 0;"
regards,
dan carpenter
Re: [PATCH 04/11] staging: wilc1000: refactor WILC_WFI_p2p_rx() to avoid line over 80 char
This one would have been easier for me to review if it were broken up slightly differently. I have a script to review when people split functions up, but there were a bunch of other stuff so my script gets confused. Anyway, looks good. regards, dan carpenter
Re: [PATCH 01/11] staging: wilc1000: refactor scan() to free kmalloc memory on failure cases
On Tue, Mar 20, 2018 at 10:25:34PM +0530, Ajay Singh wrote:
> Added changes to free the allocated memory in scan() for error condition.
> Also added 'NULL' check validation before accessing allocated memory.
>
> Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> ---
> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 62
> +--
> 1 file changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 9d8d5d0..b784e15 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -582,6 +582,49 @@ static int set_channel(struct wiphy *wiphy,
> return result;
> }
>
> +static inline bool
This shouldn't be a bool function. It should return 0 and -ENOMEM.
Bool functions should kind of have the return values built into the name
like access_ok() or IS_ERR().
> +wilc_wfi_cfg_alloc_fill_ssid(struct cfg80211_scan_request *request,
> + struct hidden_network *ntwk)
> +{
> + int i = 0;
No need to initialize "i".
> +
> + ntwk->net_info = kcalloc(request->n_ssids,
> + sizeof(struct hidden_network), GFP_KERNEL);
> +
> + if (!ntwk->net_info)
> + goto out;
Don't put a blank line between the alloc and the check. They're as
connected as can be. I hate "goto out;" but that is a personal
preference which I would never push on to other developers...
> +
> + ntwk->n_ssids = request->n_ssids;
> +
> + for (i = 0; i < request->n_ssids; i++) {
> + if (request->ssids[i].ssid_len > 0) {
> + struct hidden_net_info *info = >net_info[i];
> +
> + info->ssid = kmemdup(request->ssids[i].ssid,
> + request->ssids[i].ssid_len,
> + GFP_KERNEL);
> +
> + if (!info->ssid)
> + goto out_free;
> +
> + info->ssid_len = request->ssids[i].ssid_len;
> + } else {
> + ntwk->n_ssids -= 1;
> + }
You didn't introduce the problem, but this loop seems kind of buggy. We
should have two iterators, one for request->ssids[i] and one for
ntwk->net_info[i]. Otherwise we're copying the array information but
we're leaving holes in the destination array. Which would be fine
except we're not saving the totaly number of elements in the destination
array, we're saving the number of elements with stuff in them.
So imagine that we have a request->n_ssids == 10 but only the last
three elements have request->ssids[i].ssid_len > 0. Then we record that
ntwk->n_ssids is 3 but wthose elements are all holes. So that can't
work. See handle_scan():
for (i = 0; i < hidden_net->n_ssids; i++)
valuesize += ((hidden_net->net_info[i].ssid_len) + 1);
"valuesize" is wrong because it's looking at holes.
> + }
> + return true;
> +
> +out_free:
> +
> + for (; i >= 0 ; i--)
> + kfree(ntwk->net_info[i].ssid);
The first kfree(ntwk->net_info[i].ssid); is a no-op. You could write
this like:
while (--i >= 0)
kfree(ntwk->net_info[i].ssid);
Or:
while (i--)
kfree(ntwk->net_info[i].ssid);
Or you could do:
for (i--; i >= 0 ; i--)
kfree(ntwk->net_info[i].ssid);
I don't care...
regards,
dan carpenter
Re: [PATCH 5/7] staging: wilc1000: replace switch statement by simple if condition
On Tue, Mar 20, 2018 at 11:42:27AM +0530, wrote: > On Mon, Mar 19, 2018 at 07:45:46PM +0100, Greg KH wrote: > > On Wed, Mar 14, 2018 at 06:15:03PM +0530, hariprasath.ela...@gmail.com > > wrote: > > > From: HariPrasath Elango <hariprasath.ela...@gmail.com> > > > > > > In this case,there is only a single switch case statement.So replacing > > > by a simple if condition. > > > > > > Signed-off-by: HariPrasath Elango <hariprasath.ela...@gmail.com> > > > --- > > > drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 +--- > > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > Does not apply to my tree :( > > Hi Greg, > > Sorry about that. Probably there were other patches on the list that were applied first. It's likely not your fault, but just part of the process. > Shall I sent a v2 after rebasing my repo ? Will that > be fine ? Yes. regards, dan carpenter
Re: [PATCH 8/8] staging: wilc1000: fix open parenthesis mismatch issue in wilc_wlan_cfg_set()
On Tue, Feb 27, 2018 at 06:53:40PM +0530, Ajay Singh wrote: > Hi Dan, > > On Tue, 27 Feb 2018 12:41:40 +0300 > Dan Carpenter <dan.carpen...@oracle.com> wrote: > > > The first 5 patches are good, but the last 3 are not OK. > > Thanks for your review comments. > I will resubmit the patch series by only including first 5 patches. I > will recheck the last 3 patches and submit them separately. > Greg can probably just apply the first 5 as-is. No need to resend. regards, dan carpenter
Re: [PATCH 7/8] staging: wilc1000: fix line over 80 char in wilc_wlan_cfg_set()
On Mon, Feb 26, 2018 at 10:02:01PM +0530, Ajay Singh wrote:
> Fix 'line over 80 character' issue found by checkpatch.pl script.
>
> Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> ---
> drivers/staging/wilc1000/wilc_wlan.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wlan.c
> b/drivers/staging/wilc1000/wilc_wlan.c
> index 223bf8b..acf7591 100644
> --- a/drivers/staging/wilc1000/wilc_wlan.c
> +++ b/drivers/staging/wilc1000/wilc_wlan.c
> @@ -1230,6 +1230,8 @@ int wilc_wlan_cfg_set(struct wilc_vif *vif, int start,
> u16 wid, u8 *buffer,
> wilc->cfg_frame_offset = offset;
>
> if (commit) {
> + unsigned long tmp = msecs_to_jiffies(CFG_PKTS_TIMEOUT);
> +
> netdev_dbg(vif->ndev,
> "[WILC]PACKET Commit with sequence number %d\n",
> wilc->cfg_seq_no);
> @@ -1239,8 +1241,7 @@ int wilc_wlan_cfg_set(struct wilc_vif *vif, int start,
> u16 wid, u8 *buffer,
> if (wilc_wlan_cfg_commit(vif, WILC_CFG_SET, drv_handler))
> ret_size = 0;
>
> - if (!wait_for_completion_timeout(>cfg_event,
> -
> msecs_to_jiffies(CFG_PKTS_TIMEOUT))) {
> + if (!wait_for_completion_timeout(>cfg_event, tmp)) {
Also, it's not just the variable name I have an issue with. I like that
I can see CFG_PKTS_TIMEOUT directly instead of having to look for it a
few lines back. Don't add unecessary indirection.
So just leave this one as-is. Or flip the "if (commit) " condition
around and do:
if (!commit)
return ret_size;
regards,
dan carpenter
Re: [PATCH 8/8] staging: wilc1000: fix open parenthesis mismatch issue in wilc_wlan_cfg_set()
The first 5 patches are good, but the last 3 are not OK. Normally "tmp" is used as an iterator pointer or something along those lines. For example, here is a good use of "tmp". tmp = left; left = right; right = tmp; In this example, you want to store a pointer temporarily, so what else are you going to call it besides "tmp"? The name "tmp" doesn't mean "I want a short name and I'm too lazy to think of one". regards, dan carpenter
Re: [PATCH 7/8] staging: wilc1000: fix line over 80 char in wilc_wlan_cfg_set()
On Mon, Feb 26, 2018 at 10:02:01PM +0530, Ajay Singh wrote:
> Fix 'line over 80 character' issue found by checkpatch.pl script.
>
> Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> ---
> drivers/staging/wilc1000/wilc_wlan.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wlan.c
> b/drivers/staging/wilc1000/wilc_wlan.c
> index 223bf8b..acf7591 100644
> --- a/drivers/staging/wilc1000/wilc_wlan.c
> +++ b/drivers/staging/wilc1000/wilc_wlan.c
> @@ -1230,6 +1230,8 @@ int wilc_wlan_cfg_set(struct wilc_vif *vif, int start,
> u16 wid, u8 *buffer,
> wilc->cfg_frame_offset = offset;
>
> if (commit) {
> + unsigned long tmp = msecs_to_jiffies(CFG_PKTS_TIMEOUT);
Again. This "tmp" is a bad name here. "tmp" can be a good name, and
I use it all the time, but *here* it is a bad name.
regards,
dan carpenter
Re: [PATCH 6/8] staging: wilc1000: fix line over 80 char in wilc_wlan_handle_rxq()
On Mon, Feb 26, 2018 at 10:02:00PM +0530, Ajay Singh wrote: > Fix 'line over 80 character' issue found by checkpatch.pl script. > Refactor wilc_wlan_handle_rxq() code to remove the checkpatch.pl > warnings. > > Signed-off-by: Ajay Singh <ajay.kat...@microchip.com> > --- > drivers/staging/wilc1000/wilc_wlan.c | 46 > > 1 file changed, 25 insertions(+), 21 deletions(-) > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c > b/drivers/staging/wilc1000/wilc_wlan.c > index 74b80ad..223bf8b 100644 > --- a/drivers/staging/wilc1000/wilc_wlan.c > +++ b/drivers/staging/wilc1000/wilc_wlan.c > @@ -798,6 +798,7 @@ static void wilc_wlan_handle_rxq(struct wilc *wilc) > u32 header; > u32 pkt_len, pkt_offset, tp_len; > int is_cfg_packet; > + int tmp; > Heh. Nope. Don't do this. You've just create a "tmp" variable to hold many different types of random long values... It makes the code less readable. Just break it up into separate functions. regards, dan carpenter
Re: [PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()
On Thu, Feb 22, 2018 at 02:09:01PM +0530, Ajay Singh wrote:
> Hi Dan,
>
> On Thu, 22 Feb 2018 10:20:58 +0300
> Dan Carpenter <dan.carpen...@oracle.com> wrote:
>
> > On Wed, Feb 21, 2018 at 09:42:09PM +0530, Ajay Singh wrote:
> > > Modified wilc_spi_read_int() by removing unnecessary use of "happened"
> > > variable.
> > >
> > > Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> > > ---
> > > drivers/staging/wilc1000/wilc_spi.c | 8 +++-
> > > 1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/wilc1000/wilc_spi.c
> > > b/drivers/staging/wilc1000/wilc_spi.c
> > > index 6b392c9..131d2b7 100644
> > > --- a/drivers/staging/wilc1000/wilc_spi.c
> > > +++ b/drivers/staging/wilc1000/wilc_spi.c
> > > @@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32
> > > *int_status)
> > > int ret;
> > > u32 tmp;
> > > u32 byte_cnt;
> > > - int happened, j;
> > > + int j;
> > > u32 unknown_mask;
> > > u32 irq_flags;
> > > int k = IRG_FLAGS_OFFSET + 5;
> > > @@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32
> > > *int_status)
> > >
> > > j = 0;
> > > do {
> > > - happened = 0;
> > > -
> > > wilc_spi_read_reg(wilc, 0x1a90, _flags);
> > > tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
> > >
> > > @@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32
> > > *int_status)
> > > dev_err(>dev,
> > > "Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
> > > j, tmp, unknown_mask);
> > > - happened = 1;
> > > + break;
> >
> > This is flipped around. happened means don't break, but you've changed
> > it to be the opposite.
>
> You are right. Thanks for pointing it out. It's was a mistake. I will
> change 'break' to 'continue' and while(1) to while(0) and resubmit the
> patch.
>
Don't be in a hurry to resend. I always wait over night before
resending so that I'm not stressed when I review it. What you are
proposing still sounds wrong because the j++ is essential. Anyway, I
can't really review your v2 patch until you send it.
regards,
dan carpenter
Re: [PATCH 3/4] staging: wilc1000: refactor wilc_spi_clear_int_ext() by using GENMASK macro
Please check all these again, right? I've glanced at this and it seems wrong, but I'm too stupid to sure immediately and too lazy to be thourough. regards, dan caprenter
Re: [PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro
On Wed, Feb 21, 2018 at 09:42:10PM +0530, Ajay Singh wrote:
> Use existing macro GENMASK to get the bitmask value. Moved the code to
> get the bitmask value outside the loop, as its only required one time.
>
> Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> ---
> drivers/staging/wilc1000/wilc_spi.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_spi.c
> b/drivers/staging/wilc1000/wilc_spi.c
> index 131d2b7..c63f534 100644
> --- a/drivers/staging/wilc1000/wilc_spi.c
> +++ b/drivers/staging/wilc1000/wilc_spi.c
> @@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32
> *int_status)
> tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
>
> j = 0;
> + unknown_mask = GENMASK(g_spi.nint - 1, 0);
> do {
> wilc_spi_read_reg(wilc, 0x1a90, _flags);
> tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
> @@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32
> *int_status)
> tmp |= (((irq_flags >> 0) & 0x7) << k);
> }
>
> - unknown_mask = ~((1ul << g_spi.nint) - 1);
> -
This isn't right at all... Say g_spi.nint is zero, then we're doing
GENMASK(-1, 0) which seems like it should be undefined. If g_spi.nint
is 1 then "GENMASK(1 - 1, 0)" is 0x1 but "~((1 < 1) - 1)" is ~0x1.
I'm done reviewing this patch series... You need to be more careful.
Create a small test program to test your patches. As a reviewer,
creating test programs is how I review your patches.
#include
#include
#include
#include
#include
#include
#include
#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
#include "kernel.h"
#include
#include
#include
#include
#include
#include
#define BITS_PER_LONG 64
#define GENMASK(h, l) \
(((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h
int main(void)
{
int i;
u32 mask1, mask2;
for (i = 0; i < 32; i++) {
mask1 = ~((1ul << i) - 1);
mask2 = GENMASK(i - 1, 0);
if (mask1 == mask2)
continue;
printf("ONE 0x%x %d\n", mask1, i);
printf("TWO 0x%x\n", mask2);
}
return 0;
}
regards,
dan carpenter
Re: [PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()
On Wed, Feb 21, 2018 at 09:42:09PM +0530, Ajay Singh wrote:
> Modified wilc_spi_read_int() by removing unnecessary use of "happened"
> variable.
>
> Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> ---
> drivers/staging/wilc1000/wilc_spi.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_spi.c
> b/drivers/staging/wilc1000/wilc_spi.c
> index 6b392c9..131d2b7 100644
> --- a/drivers/staging/wilc1000/wilc_spi.c
> +++ b/drivers/staging/wilc1000/wilc_spi.c
> @@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32
> *int_status)
> int ret;
> u32 tmp;
> u32 byte_cnt;
> - int happened, j;
> + int j;
> u32 unknown_mask;
> u32 irq_flags;
> int k = IRG_FLAGS_OFFSET + 5;
> @@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32
> *int_status)
>
> j = 0;
> do {
> - happened = 0;
> -
> wilc_spi_read_reg(wilc, 0x1a90, _flags);
> tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
>
> @@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32
> *int_status)
> dev_err(>dev,
> "Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
> j, tmp, unknown_mask);
> - happened = 1;
> + break;
This is flipped around. happened means don't break, but you've changed
it to be the opposite.
regards,
dan carpenter
> }
>
> j++;
> - } while (happened);
> + } while (1);
>
Re: [PATCH 04/12] staging: wilc1000: fix open parenthesis alignment mismatch in wilc_parse_network_info()
On Fri, Feb 16, 2018 at 08:41:41PM +0530, Ajay Singh wrote: > Fix "Alignment should match open parenthesis" issue found by > checkpatch.pl script. > > Signed-off-by: Ajay Singh <ajay.kat...@microchip.com> > --- > drivers/staging/wilc1000/coreconfigurator.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/wilc1000/coreconfigurator.c > b/drivers/staging/wilc1000/coreconfigurator.c > index e98fc8e..2e2187b 100644 > --- a/drivers/staging/wilc1000/coreconfigurator.c > +++ b/drivers/staging/wilc1000/coreconfigurator.c > @@ -320,8 +320,8 @@ s32 wilc_parse_network_info(u8 *msg_buffer, > get_ssid(msa, network_info->ssid, _info->ssid_len); > get_BSSID(msa, network_info->bssid); > > - network_info->ch = get_current_channel_802_11n(msa, > - rx_len + FCS_LEN); > + network_info->ch = get_current_channel_802_11n(msa, rx_len > ++ FCS_LEN); Greg has already applied this which is fine, but I probably would have left this as is. Normally a rule is that the '+' character should be on the first line, and also it's slightly easier to read when the argument is one one line instead of split across two. But it doesn't matter much either way. We'll probably end up re-writing great swaths of this code. regards, dan carpenter
[bug report] NFC: nci: Add HCI over NCI protocol support
Hello Christophe Ricard,
The patch 11f54f228643: "NFC: nci: Add HCI over NCI protocol support"
from Feb 1, 2015, leads to the following static checker warning:
net/nfc/nci/hci.c:297 nci_hci_cmd_received()
error: buffer overflow 'ndev->hci_dev->pipes' 127 <= 127
net/nfc/nci/hci.c
294 static void nci_hci_cmd_received(struct nci_dev *ndev, u8 pipe,
295 u8 cmd, struct sk_buff *skb)
296 {
297 u8 gate = ndev->hci_dev->pipes[pipe].gate;
->pipes[] has 127 elements and "pipe" can go up to 127 so this might be
reading one element beyond the end of the array.
298 u8 status = NCI_HCI_ANY_OK | ~NCI_HCI_FRAGMENT;
299 u8 dest_gate, new_pipe;
300 struct nci_hci_create_pipe_resp *create_info;
301 struct nci_hci_delete_pipe_noti *delete_info;
302 struct nci_hci_all_pipe_cleared_noti *cleared_info;
303
304 pr_debug("from gate %x pipe %x cmd %x\n", gate, pipe, cmd);
305
"pipe" can come from two places but their both essentially the same:
net/nfc/nci/hci.c
413 static void nci_hci_msg_rx_work(struct work_struct *work)
414 {
415 struct nci_hci_dev *hdev =
416 container_of(work, struct nci_hci_dev, msg_rx_work);
417 struct sk_buff *skb;
418 struct nci_hcp_message *message;
419 u8 pipe, type, instruction;
420
421 while ((skb = skb_dequeue(>msg_rx_queue)) != NULL) {
422 pipe = NCI_HCP_MSG_GET_PIPE(skb->data[0]);
^^
The NCI_HCP_MSG_GET_PIPE() macro looks like this:
#define NCI_HCP_MSG_GET_PIPE(header) (header & 0x7f)
423 skb_pull(skb, NCI_HCI_HCP_PACKET_HEADER_LEN);
424 message = (struct nci_hcp_message *)skb->data;
425 type = NCI_HCP_MSG_GET_TYPE(message->header);
426 instruction = NCI_HCP_MSG_GET_CMD(message->header);
427 skb_pull(skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
428
429 nci_hci_hcp_message_rx(hdev->ndev, pipe,
430 type, instruction, skb);
431 }
432 }
regards,
dan carpenter
Re: [PATCH] wireless: zd1211rw: remove redundant assignment of pointer 'q'
On Wed, Jan 31, 2018 at 02:58:57PM +0200, Andy Shevchenko wrote: > On Tue, Jan 30, 2018 at 8:25 PM, Colin King <colin.k...@canonical.com> wrote: > > From: Colin Ian King <colin.k...@canonical.com> > > > > Pointer q is initialized and then almost immediately afterwards being > > re-assigned the same value. Remove the second redundant assignment. > > > > Don't you see strange that in the same context of the patch two users > of q are present? > > How did you test this? > The patch is obviously correct, I don't understand the question. regards, dan carpenter
Re: [PATCH v2 13/14] staging: wilc1000: rename Handle_Connect() to avoid camelCase
On Tue, Jan 30, 2018 at 07:29:49PM +0530, Ajay Singh wrote: > On Tue, 30 Jan 2018 02:13:53 +0800 > kbuild test robot <l...@intel.com> wrote: > > > > The patch only change variable names to avoid the camelCase, didn't modify > any extra code to dereference memory. You are responding to a robot and I think we all understood that this warning was there before you renamed the variables. > I think, with the use of shorter variable name now memcpy() is taking 1 line > instead of 3 lines. So, now line 937 has different code line(as code is > swifted up by few lines).So because of that new potential NULL dereference > error is popped up for same existing code. > The code to validate dynamically allocated memory before access, will be > include in separate patch to keep it segregated from variable names changes. > I will rework on this patch and resend again. There is no need to re-work the patch. You are right that the NULL check should be added as a separate check. regards, dan carpenter
Re: [PATCH v2 13/14] staging: wilc1000: rename Handle_Connect() to avoid camelCase
On Tue, Jan 30, 2018 at 02:13:53AM +0800, kbuild test robot wrote: > Hi Ajay, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on staging/staging-testing] > [cannot apply to v4.15 next-20180126] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Ajay-Singh/fix-to-remove-unnecessary-parenthesis-typedef-and-avoid-camelCase/20180123-115314 > > New smatch warnings: > drivers/staging/wilc1000/host_interface.c:937 handle_connect() error: > potential null dereference 'hif_drv->usr_conn_req.ssid'. (kmalloc returns > null) > It's not really a new warning, it's just that we renamed variables. But that's fine because it's a real bug and we should fix it. What I don't understand is that this warning was introduced in patch 9 so I don't know why the script is responding to patch 13. I would have expect it to reply to patch 9 (because that's where the warning is introduced) or patch 0 or patch 14 (if it's testing the whole patchset instead of individual patches). regards, dan carpenter
Re: [bug report] mt76: fix transmission of encrypted management frames
On Tue, Jan 30, 2018 at 03:39:56PM +0300, Dan Carpenter wrote:
> Hello Felix Fietkau,
>
> The patch 23405236460b: "mt76: fix transmission of encrypted
> management frames" from Jan 18, 2018, leads to the following static
> checker warning:
>
> drivers/net/wireless/mediatek/mt76/mt76x2_tx.c:41 mt76x2_tx()
> warn: always true condition '(wcid->hw_key_idx != -1) => (0-255 !=
> (-1))'
>
> drivers/net/wireless/mediatek/mt76/mt76x2_tx.c
> 26 void mt76x2_tx(struct ieee80211_hw *hw, struct ieee80211_tx_control
> *control,
> 27 struct sk_buff *skb)
> 28 {
> 29 struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> 30 struct mt76x2_dev *dev = hw->priv;
> 31 struct ieee80211_vif *vif = info->control.vif;
> 32 struct mt76_wcid *wcid = >global_wcid;
> 33
> 34 if (control->sta) {
> 35 struct mt76x2_sta *msta;
> 36
> 37 msta = (struct mt76x2_sta *) control->sta->drv_priv;
> 38 wcid = >wcid;
> 39 }
> 40
> 41 if (vif || (!info->control.hw_key && wcid->hw_key_idx != -1))
> {
> ^^
> We set ->hw_key_idx to -1 but it's a u8 so it gets truncated to 0xFF.
> This should probably be a define anyway.
>
> 42 struct mt76x2_vif *mvif;
> 43
> 44 mvif = (struct mt76x2_vif *) vif->drv_priv;
^
Oh, there is another static checker warning because of the "vif" check:
drivers/net/wireless/mediatek/mt76/mt76x2_tx.c:44 mt76x2_tx()
error: we previously assumed 'vif' could be null (see line 41)
Assume "vif" is NULL, and info->control.hw_key is zero and ->hw_key_idx
is not -1.
regards,
dan carpenter
[bug report] mt76: fix transmission of encrypted management frames
Hello Felix Fietkau,
The patch 23405236460b: "mt76: fix transmission of encrypted
management frames" from Jan 18, 2018, leads to the following static
checker warning:
drivers/net/wireless/mediatek/mt76/mt76x2_tx.c:41 mt76x2_tx()
warn: always true condition '(wcid->hw_key_idx != -1) => (0-255 !=
(-1))'
drivers/net/wireless/mediatek/mt76/mt76x2_tx.c
26 void mt76x2_tx(struct ieee80211_hw *hw, struct ieee80211_tx_control
*control,
27 struct sk_buff *skb)
28 {
29 struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
30 struct mt76x2_dev *dev = hw->priv;
31 struct ieee80211_vif *vif = info->control.vif;
32 struct mt76_wcid *wcid = >global_wcid;
33
34 if (control->sta) {
35 struct mt76x2_sta *msta;
36
37 msta = (struct mt76x2_sta *) control->sta->drv_priv;
38 wcid = >wcid;
39 }
40
41 if (vif || (!info->control.hw_key && wcid->hw_key_idx != -1)) {
^^
We set ->hw_key_idx to -1 but it's a u8 so it gets truncated to 0xFF.
This should probably be a define anyway.
42 struct mt76x2_vif *mvif;
43
44 mvif = (struct mt76x2_vif *) vif->drv_priv;
45 wcid = >group_wcid;
46 }
47
48 mt76_tx(>mt76, control->sta, wcid, skb);
49 }
regards,
dan carpenter
Re: [PATCH v2 02/14] staging: wilc1000: removed unnecessary defined enums typedef
On Mon, Jan 22, 2018 at 01:06:59PM +0200, Claudiu Beznea wrote: > > > On 22.01.2018 12:22, Ajay Singh wrote: > > Fix the "do not add new typedefs" issue found by checkpatch.pl > > script > > > >From > >https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format > the the message body line is wrapped at 75 columns. > I normally consider 75 characters a maximum. I wrap at 72 characters like a normal email. If we get too hung up on procedures for their own sake then it makes sending patches a real pain. There are some people I just don't send patches to because they are too much headache to deal with. regards, dan carpenter
Re: [PATCH 04/14] staging: wilc1000: rename host_int_ParseJoinBssParam() and it's variable using camelCase
On Fri, Jan 19, 2018 at 01:20:00PM +0200, Claudiu Beznea wrote:
> It is hard to review this. Anyway, Reviewed-by: Claudiu Beznea
> <claudiu.bez...@microchip.com>
>
I have a script I use. It's sort of hacky, but it's a time saver. I've
attached it.
To review this one, I do `cat | rename_rev.pl -a`
regards,
dan carpenter
#!/usr/bin/perl
# This is a tool to help review variable rename patches. The goal is
# to strip out the automatic sed renames and the white space changes
# and leaves the interesting code changes.
#
# Example 1: A patch renames openInfo to open_info:
# cat diff | rename_review.pl openInfo open_info
#
# Example 2: A patch swaps the first two arguments to some_func():
# cat diff | rename_review.pl \
#-e 's/some_func\((.*?),(.*?),/some_func\($2, $1,/'
#
# Example 3: A patch removes the xkcd_ prefix from some but not all the
# variables. Instead of trying to figure out which variables were renamed
# just remove the prefix from them all:
# cat diff | rename_review.pl -ea 's/xkcd_//g'
#
# Example 4: A patch renames 20 CamelCase variables. To review this let's
# just ignore all case changes and all '_' chars.
# cat diff | rename_review -ea 'tr/[A-Z]/[a-z]/' -ea 's/_//g'
#
# The other arguments are:
# -nc removes comments
# -ns removes '\' chars if they are at the end of the line.
use strict;
use File::Temp qw/ :mktemp /;
sub usage() {
print "usage: cat diff | $0 old new old new old new...\n";
print " or: cat diff | $0 -e 's/old/new/g'\n";
print " -a : auto";
print " -e : execute on old lines\n";
print " -ea: execute on all lines\n";
print " -nc: no comments\n";
print " -nb: no unneeded braces\n";
print " -ns: no slashes at the end of a line\n";
print " -pull: for function pull. deletes context.\n";
print " -r : NULL, bool";
exit(1);
}
my @subs;
my @strict_subs;
my @cmds;
my $strip_comments;
my $strip_braces;
my $strip_slashes;
my $pull_context;
my $auto;
sub filter($) {
my $line = shift();
my $old = 0;
if ($line =~ /^-/) {
$old = 1;
}
# remove the first char
$line =~ s/^[ +-]//;
if ($strip_comments) {
$line =~ s/\/\*.*?\*\///g;
$line =~ s/\/\/.*//;
}
foreach my $cmd (@cmds) {
if ($old || $cmd->[0] =~ /^-ea$/) {
eval "\$line =~ $cmd->[1]";
}
}
foreach my $sub (@subs) {
if ($old) {
$line =~ s/$sub->[0]/$sub->[1]/g;
}
}
foreach my $sub (@strict_subs) {
if ($old) {
$line =~ s/\b$sub->[0]\b/$sub->[1]/g;
}
}
# remove the newline so we can move curly braces here if we want.
$line =~ s/\n//;
return $line;
}
while (my $param1 = shift()) {
if ($param1 =~ /^-a$/) {
$auto = 1;
next;
}
if ($param1 =~ /^-nc$/) {
$strip_comments = 1;
next;
}
if ($param1 =~ /^-nb$/) {
$strip_braces = 1;
next;
}
if ($param1 =~ /^-ns$/) {
$strip_slashes = 1;
next;
}
if ($param1 =~ /^-pull$/) {
$pull_context = 1;
next;
}
my $param2 = shift();
if ($param2 =~ /^$/) {
usage();
}
if ($param1 =~ /^-e(a|)$/) {
push @cmds, [$param1, $param2];
next;
}
if ($param1 =~ /^-r$/) {
if ($param2 =~ /bool/) {
push @cmds, ["-e", "s/== true//"];
push @cmds, ["-e", "s/true ==//"];
push @cmds, ["-e", "s/([a-zA-Z\-\>\._]+) == false/!\$1/"];
next;
} elsif ($param2 =~ /NULL/) {
push @cmds, ["-e", "s/ != NULL//"];
push @cmds, ["-e", "s/([a-zA-Z\-\>\._0-9]+) == NULL/!\$1/"];
next;
} elsif ($param2 =~ /BIT/) {
push @cmds, ["-e", 's/1[uU]* *<< *(\d+)/BIT($1)/'];
push @cmds, ["-e", 's/\(1 *<< *(\w+)\)/BIT($1)/'];
push @cmds, ["-e", 's/\(BIT\((.*?)\)\)/BIT($1)/'];
next;
}
usage();
}
push @subs, [$param1, $param2];
}
my ($oldfh, $oldfile) = mkstemp("/tmp/oldX");
my ($newfh, $newfile) = mkstemp("/tmp/newX");
my @input = ;
# auto works on the observation that the - line comes before the + line when we
# rename variables. Take the first - line. Find the first + line. Find the
# one word difference. Test that the old word never occurs in the new text.
if ($auto) {
my %c_keywords = ( auto => 1,
break => 1,
case => 1,
char => 1,
const => 1,
continue => 1,
[bug report] mt76: add driver code for MT76x2e
Hello Felix Fietkau, This is a semi-automatic email about new static checker warnings. The patch 7bc04215a66b: "mt76: add driver code for MT76x2e" from Nov 21, 2017, leads to the following Smatch complaint: drivers/net/wireless/mediatek/mt76/mt76x2_main.c:457 mt76x2_ampdu_action() warn: variable dereferenced before check 'txq' (see line 453) drivers/net/wireless/mediatek/mt76/mt76x2_main.c 452 struct ieee80211_txq *txq = sta->txq[params->tid]; 453 struct mt76_txq *mtxq = (struct mt76_txq *) txq->drv_priv; ^ Unchecked dereference. Also the cast is has a checkpatch a space issue. No space after the cast, because casting is a high precedence operation. 454 u16 tid = params->tid; 455 u16 *ssn = >ssn; 456 457 if (!txq) ^^^ Check is too late. 458 return -EINVAL; 459 regards, dan carpenter
[bug report] mt76: add driver code for MT76x2e
Hello Felix Fietkau,
This is a semi-automatic email about new static checker warnings.
The patch 7bc04215a66b: "mt76: add driver code for MT76x2e" from Nov
21, 2017, leads to the following Smatch complaint:
drivers/net/wireless/mediatek/mt76/mt76x2_mac.c:189 mt76x2_mac_write_txwi()
error: we previously assumed 'wcid' could be null (see line 180)
drivers/net/wireless/mediatek/mt76/mt76x2_mac.c
179
180 if (wcid)
Check for NULL
181 txwi->wcid = wcid->idx;
182 else
183 txwi->wcid = 0xff;
184
185 txwi->pktid = 1;
186
187 spin_lock_bh(>mt76.lock);
188 if (rate->idx < 0 || !rate->count) {
189 txwi->rate = wcid->tx_rate;
^
Unchecked dereference
190 max_txpwr_adj = wcid->max_txpwr_adj;
191 nss = wcid->tx_rate_nss;
regards,
dan carpenter
[PATCH] ath9k_htc: Add a sanity check in ath9k_htc_ampdu_action()
Smatch generates a warning here:
drivers/net/wireless/ath/ath9k/htc_drv_main.c:1688 ath9k_htc_ampdu_action()
error: buffer overflow 'ista->tid_state' 8 <= 15
I don't know if it's a real bug or not but the other paths through this
function all ensure that "tid" is less than ATH9K_HTC_MAX_TID (8) so
checking here makes things more consistent.
Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
This might be fine. I see lots of other drivers don't check tid.
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index f808e5833d7e..a82ad739ab80 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -1683,6 +1683,10 @@ static int ath9k_htc_ampdu_action(struct ieee80211_hw
*hw,
ieee80211_stop_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_OPERATIONAL:
+ if (tid >= ATH9K_HTC_MAX_TID) {
+ ret = -EINVAL;
+ break;
+ }
ista = (struct ath9k_htc_sta *) sta->drv_priv;
spin_lock_bh(>tx.tx_lock);
ista->tid_state[tid] = AGGR_OPERATIONAL;
[bug report] mac80211: fix VLAN handling with TXQs
[ I fixed a Smatch bug and now it started warning about this code -dan ]
Hello Johannes Berg,
This is a semi-automatic email about new static checker warnings.
The patch 531682159092: "mac80211: fix VLAN handling with TXQs" from
Jun 22, 2017, leads to the following Smatch complaint:
net/mac80211/tx.c:3529 ieee80211_tx_dequeue()
error: we previously assumed 'skb' could be null (see line 3511)
net/mac80211/tx.c
3500 tx.key, skb);
3501 } else {
3502 if (invoke_tx_handlers_late())
3503 goto begin;
3504
3505 skb = __skb_dequeue();
^
The NULL skb would have to come from here.
3506
3507 if (!skb_queue_empty())
3508 skb_queue_splice_tail(, >frags);
3509 }
3510
3511 if (skb && skb_has_frag_list(skb) &&
^^^
Old code checks
3512 !ieee80211_hw_check(>hw, TX_FRAG_LIST)) {
3513 if (skb_linearize(skb)) {
3514 ieee80211_free_txskb(>hw, skb);
3515 goto begin;
3516 }
3517 }
3518
3519 switch (tx.sdata->vif.type) {
3520 case NL80211_IFTYPE_MONITOR:
3521 if (tx.sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE) {
3522 vif = >vif;
3523 break;
3524 }
3525 tx.sdata = rcu_dereference(local->monitor_sdata);
3526 if (tx.sdata) {
3527 vif = >vif;
3528 info->hw_queue =
3529
vif->hw_queue[skb_get_queue_mapping(skb)];
^^^
Patch adds unchecked dereference (might be a false positive).
3530 } else if (ieee80211_hw_check(>hw,
QUEUE_CONTROL)) {
3531 ieee80211_free_txskb(>hw, skb);
regards,
dan carpenter
Re: [PATCH] staging: wilc1000: Fix bssid buffer offset in Txq
This driver really needs proper structs and datatypes so this doesn't happen again and so that you and Colin stop reverting each other's *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF); assignments: regards, dan carpenter
Re: [PATCH] staging: wilc1000: replace redundant computations with 0
On Tue, Oct 10, 2017 at 03:05:48PM +0100, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> Shifting and masking strHostIfSetMulti->enabled is redundant since
> enabled is a bool and so all the shifted and masked values will be
> zero. Replace them with zero to simplify the code.
>
> Detected by CoverityScan, CID#1339458 ("Bad shift operation") and
> CID#1339506 ("Operands don't affect result").
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
> drivers/staging/wilc1000/host_interface.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/host_interface.c
> b/drivers/staging/wilc1000/host_interface.c
> index 7b620658ec38..94477dd08c85 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -2417,9 +2417,9 @@ static void Handle_SetMulticastFilter(struct wilc_vif
> *vif,
>
> pu8CurrByte = wid.val;
> *pu8CurrByte++ = (strHostIfSetMulti->enabled & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 16) & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 24) & 0xFF);
> + *pu8CurrByte++ = 0;
> + *pu8CurrByte++ = 0;
> + *pu8CurrByte++ = 0;
This is harder to understand now. I would be better to solve this by
declaring a struct with the right format and using cpu_to_be32().
regards,
dan carpenter
Re: Two rtlwifi drivers?
On Wed, Oct 11, 2017 at 03:13:10PM +0200, Greg Kroah-Hartman wrote: > And it seems like the company isn't willing to do the real work, so > dumping this in staging is the best we can do at the moment. I'm more optimistic. There are a lot of @realtek.com addresses in the CC list and that's a new thing. regards, dan carpenter
[PATCH 2/2] rtlwifi: silence underflow warning
My static checker complains that we have an upper bound but no lower
bound. I suspect neither are really required but it doesn't hurt to add
a check for negatives.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c
b/drivers/net/wireless/realtek/rtlwifi/core.c
index c53cbf3d52bd..294a6b43d1bc 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.c
+++ b/drivers/net/wireless/realtek/rtlwifi/core.c
@@ -453,7 +453,8 @@ static void _rtl_add_wowlan_patterns(struct ieee80211_hw
*hw,
for (i = 0; i < wow->n_patterns; i++) {
memset(_pattern, 0, sizeof(struct rtl_wow_pattern));
memset(mask, 0, MAX_WOL_BIT_MASK_SIZE);
- if (patterns[i].pattern_len > MAX_WOL_PATTERN_SIZE) {
+ if (patterns[i].pattern_len < 0 ||
+ patterns[i].pattern_len > MAX_WOL_PATTERN_SIZE) {
RT_TRACE(rtlpriv, COMP_POWER, DBG_WARNING,
"Pattern[%d] is too long\n", i);
continue;
[bug report] NFC: st21nfcb: Add support for secure element
Hello Christophe Ricard,
The patch 8ae01f796771: "NFC: st21nfcb: Add support for secure
element" from Feb 1, 2015, leads to the following static checker
warning:
drivers/nfc/st-nci/se.c:373 st_nci_hci_event_received()
warn: constraint overflow 'ndev->hci_dev->pipes' 127 <= 0-127
drivers/nfc/st-nci/se.c
370 void st_nci_hci_event_received(struct nci_dev *ndev, u8 pipe,
371 u8 event, struct sk_buff *skb)
372 {
373 u8 gate = ndev->hci_dev->pipes[pipe].gate;
374 u8 host = ndev->hci_dev->pipes[pipe].host;
375
376 switch (gate) {
377 case NCI_HCI_ADMIN_GATE:
378 st_nci_hci_admin_event_received(ndev, event, skb);
379 break;
380 case ST_NCI_APDU_READER_GATE:
381 st_nci_hci_apdu_reader_event_received(ndev, event, skb);
382 break;
383 case ST_NCI_CONNECTIVITY_GATE:
384 st_nci_hci_connectivity_event_received(ndev, host,
event, skb);
385 break;
386 }
387 }
388 EXPORT_SYMBOL_GPL(st_nci_hci_event_received);
pipe comes from skb->data[]. The call tree is:
nci_hci_msg_rx_work <-- gets pipe from skb->data[0]
-> nci_hci_hcp_message_rx
--> nci_hci_event_received
--> st_nci_hci_event_received <-- off by one
regards,
dan carpenter
[bug report] iwlwifi: mvm: add station before allocating a queue
Hello Shaul Triebitz,
The patch 732d06e9d9cf: "iwlwifi: mvm: add station before allocating
a queue" from Jul 10, 2017, leads to the following static checker
warning:
drivers/net/wireless/intel/iwlwifi/mvm/sta.c:1312
iwl_mvm_add_int_sta_common()
error: uninitialized symbol 'status'.
drivers/net/wireless/intel/iwlwifi/mvm/sta.c
1281 static int iwl_mvm_add_int_sta_common(struct iwl_mvm *mvm,
1282struct iwl_mvm_int_sta *sta,
1283const u8 *addr,
1284u16 mac_id, u16 color)
1285 {
1286 struct iwl_mvm_add_sta_cmd cmd;
1287 int ret;
1288 u32 status;
^^
1289
1290 lockdep_assert_held(>mutex);
1291
1292 memset(, 0, sizeof(cmd));
1293 cmd.sta_id = sta->sta_id;
1294 cmd.mac_id_n_color = cpu_to_le32(FW_CMD_ID_AND_COLOR(mac_id,
1295 color));
1296 if (fw_has_api(>fw->ucode_capa,
IWL_UCODE_TLV_API_STA_TYPE))
1297 cmd.station_type = sta->type;
1298
1299 if (!iwl_mvm_has_new_tx_api(mvm))
1300 cmd.tfd_queue_msk = cpu_to_le32(sta->tfd_queue_msk);
1301 cmd.tid_disable_tx = cpu_to_le16(0x);
1302
1303 if (addr)
1304 memcpy(cmd.addr, addr, ETH_ALEN);
1305
1306 ret = iwl_mvm_send_cmd_pdu_status(mvm, ADD_STA,
1307iwl_mvm_add_sta_cmd_size(mvm),
1308, );
^^
1309 if (ret)
1310 return ret;
1311
1312 switch (status & IWL_ADD_STA_STATUS_MASK) {
1313 case ADD_STA_SUCCESS:
1314 IWL_DEBUG_INFO(mvm, "Internal station added.\n");
1315 return 0;
1316 default:
1317 ret = -EIO;
1318 IWL_ERR(mvm, "Add internal station failed,
status=0x%x\n",
1319 status);
1320 break;
1321 }
1322 return ret;
1323 }
The problem here is this code from iwl_mvm_send_cmd_status()
drivers/net/wireless/intel/iwlwifi/mvm/utils.c
157 cmd->flags |= CMD_WANT_SKB;
158
159 ret = iwl_trans_send_cmd(mvm->trans, cmd);
160 if (ret == -ERFKILL) {
161 /*
162 * The command failed because of RFKILL, don't update
163 * the status, leave it as success and return 0.
164 */
165 return 0;
We return zero without setting "status". Shouldn't we set *status = 0;?
166 } else if (ret) {
167 return ret;
168 }
169
170 pkt = cmd->resp_pkt;
regards,
dan carpenter
[PATCH 1/2] rsi: missing unlocks on error paths
There is a missing unlock if rsi_find_sta() fails in
rsi_mac80211_ampdu_action() or if we hit the -EINVAL path in
rsi_mac80211_sta_add().
Fixes: 3528608f3a79 ("rsi: handle station connection in AP mode")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
index 8b983d03f2da..25331aa16e8e 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -906,7 +906,8 @@ static int rsi_mac80211_ampdu_action(struct ieee80211_hw
*hw,
rsta = rsi_find_sta(common, sta->addr);
if (!rsta) {
rsi_dbg(ERR_ZONE, "No station mapped\n");
- return 0;
+ status = 0;
+ goto unlock;
}
sta_id = rsta->sta_id;
}
@@ -974,6 +975,7 @@ static int rsi_mac80211_ampdu_action(struct ieee80211_hw
*hw,
break;
}
+unlock:
mutex_unlock(>mutex);
return status;
}
@@ -1202,6 +1204,7 @@ static int rsi_mac80211_sta_add(struct ieee80211_hw *hw,
struct rsi_common *common = adapter->priv;
bool sta_exist = false;
struct rsi_sta *rsta;
+ int status = 0;
rsi_dbg(INFO_ZONE, "Station Add: %pM\n", sta->addr);
@@ -1215,8 +1218,8 @@ static int rsi_mac80211_sta_add(struct ieee80211_hw *hw,
/* Check if max stations reached */
if (common->num_stations >= common->max_stations) {
rsi_dbg(ERR_ZONE, "Reject: Max Stations exists\n");
- mutex_unlock(>mutex);
- return -EOPNOTSUPP;
+ status = -EOPNOTSUPP;
+ goto unlock;
}
for (cnt = 0; cnt < common->max_stations; cnt++) {
rsta = >stations[cnt];
@@ -1241,7 +1244,8 @@ static int rsi_mac80211_sta_add(struct ieee80211_hw *hw,
rsi_dbg(ERR_ZONE,
"%s: Some problem reaching here...\n",
__func__);
- return -EINVAL;
+ status = -EINVAL;
+ goto unlock;
}
rsta = >stations[sta_idx];
rsta->sta = sta;
@@ -1289,9 +1293,10 @@ static int rsi_mac80211_sta_add(struct ieee80211_hw *hw,
}
}
+unlock:
mutex_unlock(>mutex);
- return 0;
+ return status;
}
/**
[PATCH 2/2] rsi: update some comments
These functions don't return -1 on failure.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
index 25331aa16e8e..e78e87e99804 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -754,7 +754,7 @@ static int rsi_mac80211_conf_tx(struct ieee80211_hw *hw,
* @vif: Pointer to the ieee80211_vif structure.
* @key: Pointer to the ieee80211_key_conf structure.
*
- * Return: status: 0 on success, -1 on failure.
+ * Return: status: 0 on success, negative error codes on failure.
*/
static int rsi_hal_key_config(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
@@ -1194,7 +1194,7 @@ static void rsi_set_min_rate(struct ieee80211_hw *hw,
* @vif: Pointer to the ieee80211_vif structure.
* @sta: Pointer to the ieee80211_sta structure.
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, negative error codes on failure.
*/
static int rsi_mac80211_sta_add(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
@@ -1306,7 +1306,7 @@ static int rsi_mac80211_sta_add(struct ieee80211_hw *hw,
* @vif: Pointer to the ieee80211_vif structure.
* @sta: Pointer to the ieee80211_sta structure.
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, negative error codes on failure.
*/
static int rsi_mac80211_sta_remove(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
@@ -1426,7 +1426,7 @@ static int rsi_mac80211_set_antenna(struct ieee80211_hw
*hw,
* @tx_ant: Bitmap for tx antenna
* @rx_ant: Bitmap for rx antenna
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, negative error codes on failure.
*/
static int rsi_mac80211_get_antenna(struct ieee80211_hw *hw,
u32 *tx_ant, u32 *rx_ant)
@@ -1533,7 +1533,7 @@ static struct ieee80211_ops mac80211_ops = {
* rsi_mac80211_attach() - This function is used to initialize Mac80211 stack.
* @common: Pointer to the driver private structure.
*
- * Return: 0 on success, -1 on failure.
+ * Return: 0 on success, negative error codes on failure.
*/
int rsi_mac80211_attach(struct rsi_common *common)
{
[PATCH] staging: rtlwifi: check for array overflow
Smatch is distrustful of the "capab" value and marks it as user
controlled. I think it actually comes from the firmware? Anyway, I
looked at other drivers and they added a bounds check and it seems like
a harmless thing to have so I have added it here as well.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
index f7f207cbaee3..a30b928d5ee1 100644
--- a/drivers/staging/rtlwifi/base.c
+++ b/drivers/staging/rtlwifi/base.c
@@ -1414,6 +1414,10 @@ bool rtl_action_proc(struct ieee80211_hw *hw, struct
sk_buff *skb, u8 is_tx)
le16_to_cpu(mgmt->u.action.u.addba_req.capab);
tid = (capab &
IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
+ if (tid >= MAX_TID_COUNT) {
+ rcu_read_unlock();
+ return true;
+ }
tid_data = _entry->tids[tid];
if (tid_data->agg.rx_agg_state ==
RTL_RX_AGG_START)
[bug report] rsi: data and managemet path changes for AP mode
Hello Prameela Rani Garnepudi,
The patch 19844c0a9a19: "rsi: data and managemet path changes for AP
mode" from Aug 16, 2017, leads to the following static checker
warning:
drivers/net/wireless/rsi/rsi_91x_core.c:397 rsi_core_xmit()
error: buffer overflow 'rsta->start_tx_aggr' 16 <= 16
drivers/net/wireless/rsi/rsi_91x_core.c
369 if ((ieee80211_is_mgmt(wh->frame_control)) ||
370 (ieee80211_is_ctl(wh->frame_control)) ||
371 (ieee80211_is_qos_nullfunc(wh->frame_control))) {
372 q_num = MGMT_SOFT_Q;
373 skb->priority = q_num;
374 } else {
375 if (ieee80211_is_data_qos(wh->frame_control)) {
376 tid = (skb->data[24] & IEEE80211_QOS_TID);
^^^
tid is capped at 15 here so that's fine.
377 skb->priority = TID_TO_WME_AC(tid);
378 } else {
379 tid = IEEE80211_NONQOS_TID;
^^
but here it's set to 16.
380 skb->priority = BE_Q;
381 }
382
383 q_num = skb->priority;
384 tx_params->tid = tid;
385
386 if ((vif->type == NL80211_IFTYPE_AP) &&
387 (!is_broadcast_ether_addr(wh->addr1)) &&
388 (!is_multicast_ether_addr(wh->addr1))) {
389 rsta = rsi_find_sta(common, wh->addr1);
390 if (!rsta)
391 goto xmit_fail;
392 tx_params->sta_id = rsta->sta_id;
393 }
394
395 if (rsta) {
396 /* Start aggregation if not done for this tid */
397 if (!rsta->start_tx_aggr[tid]) {
Smatch complains that this might be out of bounds.
398 rsta->start_tx_aggr[tid] = true;
399 ieee80211_start_tx_ba_session(rsta->sta,
400tid, 0);
401 }
402 }
403 }
regards,
dan carpenter
[PATCH] rtlwifi: make a couple arrays larger
This is a static checker fix. "cal_num" is 10. We're declaring the tx_dt[] and rx_td[] arrays as 3 element arrays. The static checker complains that we do: tx_dt[cal] = (vdf_y[1]>>20)-(vdf_y[0]>>20); "cal" is the iterator and it is in the 0-9 range so it looks like we could corrupt memory. Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com> --- I'm pretty sure this patch is correct and absolutely harmless. But the code is pretty involved and I didn't test it. So you may want to review this one carefully. diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c index aa3ccc740521..176deb2b5386 100644 --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c @@ -3773,10 +3773,11 @@ static void _rtl8821ae_iqk_tx(struct ieee80211_hw *hw, enum radio_path path) u32 tx_fail, rx_fail, delay_count, iqk_ready, cal_retry, cal = 0, temp_reg65; int tx_x = 0, tx_y = 0, rx_x = 0, rx_y = 0, tx_average = 0, rx_average = 0; int tx_x0[cal_num], tx_y0[cal_num], tx_x0_rxk[cal_num], - tx_y0_rxk[cal_num], rx_x0[cal_num], rx_y0[cal_num]; + tx_y0_rxk[cal_num], rx_x0[cal_num], rx_y0[cal_num], + tx_dt[cal_num], rx_dt[cal_num]; booltx0iqkok = false, rx0iqkok = false; boolvdf_enable = false; - int i, k, vdf_y[3], vdf_x[3], tx_dt[3], rx_dt[3], + int i, k, vdf_y[3], vdf_x[3], ii, dx = 0, dy = 0, tx_finish = 0, rx_finish = 0; RT_TRACE(rtlpriv, COMP_IQK, DBG_LOUD,
[bug report] rsi: add support for U-APSD power save
Hello Karun Eagalapati,
The patch db07971d085f: "rsi: add support for U-APSD power save" from
Aug 3, 2017, leads to the following static checker warning:
drivers/net/wireless/rsi/rsi_91x_mac80211.c:575
rsi_mac80211_bss_info_changed()
warn: inconsistent indenting
drivers/net/wireless/rsi/rsi_91x_mac80211.c
555 mutex_lock(>mutex);
556 if (changed & BSS_CHANGED_ASSOC) {
557 rsi_dbg(INFO_ZONE, "%s: Changed Association status:
%d\n",
558 __func__, bss_conf->assoc);
559 if (bss_conf->assoc) {
560 /* Send the RX filter frame */
561 rx_filter_word = (ALLOW_DATA_ASSOC_PEER |
562ALLOW_CTRL_ASSOC_PEER |
563ALLOW_MGMT_ASSOC_PEER);
564 rsi_send_rx_filter_frame(common,
rx_filter_word);
565 }
566 rsi_inform_bss_status(common,
567bss_conf->assoc,
568bss_conf->bssid,
569bss_conf->qos,
570bss_conf->aid);
571 adapter->ps_info.dtim_interval_duration =
bss->dtim_period;
572 adapter->ps_info.listen_interval =
conf->listen_interval;
573
574 /* If U-APSD is updated, send ps parameters to firmware */
575 if (bss->assoc) {
Is this supposed to be inside the if BSS_CHANGED_ASSOC if statement or
after?
576 if (common->uapsd_bitmap) {
577 rsi_dbg(INFO_ZONE, "Configuring UAPSD\n");
578 rsi_conf_uapsd(adapter);
579 }
580 } else {
581 common->uapsd_bitmap = 0;
582 }
583 }
584
585 if (changed & BSS_CHANGED_CQM) {
regards,
dan carpenter
[bug report] rsi: immediate wakeup bit and priority for TX command packets
Hello Prameela Rani Garnepudi,
The patch 9a629fafe7d8: "rsi: immediate wakeup bit and priority for
TX command packets" from Jul 10, 2017, leads to the following static
checker warning:
drivers/net/wireless/rsi/rsi_91x_mgmt.c:277
rsi_send_internal_mgmt_frame()
error: potentially dereferencing uninitialized 'desc'.
drivers/net/wireless/rsi/rsi_91x_mgmt.c
267 static int rsi_send_internal_mgmt_frame(struct rsi_common *common,
268 struct sk_buff *skb)
269 {
270 struct skb_info *tx_params;
271 struct rsi_cmd_desc *desc;
272
273 if (skb == NULL) {
274 rsi_dbg(ERR_ZONE, "%s: Unable to allocate skb\n",
__func__);
275 return -ENOMEM;
276 }
277 desc->desc_dword0.len_qno |= cpu_to_le16(DESC_IMMEDIATE_WAKEUP);
^^
278 skb->priority = MGMT_SOFT_Q;
279 tx_params = (struct skb_info
*)_SKB_CB(skb)->driver_data;
280 tx_params->flags |= INTERNAL_MGMT_PKT;
281 skb_queue_tail(>tx_queue[MGMT_SOFT_Q], skb);
282 rsi_set_event(>tx_thread.event);
283 return 0;
284 }
regards,
dan carpenter
Re: [PATCH] iwlwifi: pcie: Fix error code in iwl_trans_pcie_alloc()
Oops, heh, sorry. I switched computers so I first one wasn't in my outbox. regards, dan carpenter
Re: [PATCH] rtlwifi: rtl8821ae: Fix HW_VAR_NAV_UPPER operation
On Fri, Jul 14, 2017 at 02:09:46PM -0500, Larry Finger wrote:
> On 07/13/2017 02:43 AM, Dan Carpenter wrote:
> > The cast here is wrong. We want to cast the pointer but we accidentally
> > do a no-op cast of the value. We normally want to set us_nav_upper to
> > WIFI_NAV_UPPER_US (3) but because of this bug we instead set it to
> > 184 on little endian systems and 0 on big endian ones.
> >
> > Fixes: 3c05bedb5fef ("Staging: rtl8812ae: Add Realtek 8821 PCI WIFI driver")
> > Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
>
> Acked-by: Larry Finger <larry.fin...@lwfinger.net>
>
> Thanks for catching this error. I suspect a typo, but your fix is obviously
> correct.
>
> Does the "Fixes" notation imply a Cc to stable?
No. Fixes doesn't necessarily imply a Cc to stable.
regards,
dan carpenter
[PATCH] rtlwifi: rtl8821ae: Fix HW_VAR_NAV_UPPER operation
The cast here is wrong. We want to cast the pointer but we accidentally
do a no-op cast of the value. We normally want to set us_nav_upper to
WIFI_NAV_UPPER_US (3) but because of this bug we instead set it to
184 on little endian systems and 0 on big endian ones.
Fixes: 3c05bedb5fef ("Staging: rtl8812ae: Add Realtek 8821 PCI WIFI driver")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 2bc6bace069c..230f92a48f3c 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -779,7 +779,7 @@ void rtl8821ae_set_hw_reg(struct ieee80211_hw *hw, u8
variable, u8 *val)
_rtl8821ae_resume_tx_beacon(hw);
break; }
case HW_VAR_NAV_UPPER: {
- u32 us_nav_upper = ((u32)*val);
+ u32 us_nav_upper = *(u32 *)val;
if (us_nav_upper > HAL_92C_NAV_UPPER_UNIT * 0xFF) {
RT_TRACE(rtlpriv, COMP_INIT , DBG_WARNING,
