Re: [PATCH] net: bonding: fix error return code of bond_neigh_init()
On 2021/3/10 17:24, Roi Dayan wrote:
On 2021-03-08 5:11 AM, Jia-Ju Bai wrote:
When slave is NULL or slave_ops->ndo_neigh_setup is NULL, no error
return code of bond_neigh_init() is assigned.
To fix this bug, ret is assigned with -EINVAL in these cases.
Fixes: 9e99bfefdbce ("bonding: fix bond_neigh_init()")
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/bonding/bond_main.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c
b/drivers/net/bonding/bond_main.c
index 74cbbb22470b..456315bef3a8 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3978,11 +3978,15 @@ static int bond_neigh_init(struct neighbour *n)
rcu_read_lock();
slave = bond_first_slave_rcu(bond);
- if (!slave)
+ if (!slave) {
+ ret = -EINVAL;
goto out;
+ }
slave_ops = slave->dev->netdev_ops;
- if (!slave_ops->ndo_neigh_setup)
+ if (!slave_ops->ndo_neigh_setup) {
+ ret = -EINVAL;
goto out;
+ }
/* TODO: find another way [1] to implement this.
* Passing a zeroed structure is fragile,
Hi,
This breaks basic functionally that always worked. A slave doesn't need
to exists nor to implement ndo_neigh_setup.
Now trying to add a neigh entry because of that fails.
This commit needs to be reverted.
Okay, thanks for the explanation, and I am sorry for this false report...
Best wishes,
Jia-Ju Bai
Re: [PATCH] net: bridge: fix error return code of do_update_counters()
On 2021/3/9 19:01, Florian Westphal wrote: Jia-Ju Bai wrote: When find_table_lock() returns NULL to t, no error return code of do_update_counters() is assigned. Its -ENOENT. t = find_table_lock(net, name, &ret, &ebt_mutex); ^ ret is passed to find_table_lock, which passes it to find_inlist_lock_noload() which will set *ret = -ENOENT for NULL case. Thanks for the reply! I did not notice "&ret" in find_table_lock()... I am sorry for the false positive. Best wishes, Jia-Ju Bai
Re: [PATCH] net: netlink: fix error return code of netlink_proto_init()
On 2021/3/9 16:47, Heiner Kallweit wrote: On 09.03.2021 09:33, Jia-Ju Bai wrote: When kcalloc() returns NULL to nl_table, no error return code of netlink_proto_init() is assigned. To fix this bug, err is assigned with -ENOMEM in this case. Didn't we talk enough about your incorrect patches yesterday? This one is incorrect again. panic() never returns. Stop sending patches until you understand the code you're changing! Ah, sorry, I was too confident about this bug report... Thanks for your reply. Following your advice, now I am sending the patches only for the bug reports that I am confident about after careful code review. Some of the patches have been applied, but some of them are still wrong, like this patch... I am sorry for the false positives... Best wishes, Jia-Ju Bai
[PATCH] net: netlink: fix error return code of netlink_proto_init()
When kcalloc() returns NULL to nl_table, no error return code of
netlink_proto_init() is assigned.
To fix this bug, err is assigned with -ENOMEM in this case.
Fixes: fab2caf62ed0 ("[NETLINK]: Call panic if nl_table allocation fails")
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
net/netlink/af_netlink.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index dd488938447f..9ab66cfb1037 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2880,8 +2880,10 @@ static int __init netlink_proto_init(void)
BUILD_BUG_ON(sizeof(struct netlink_skb_parms) > sizeof_field(struct
sk_buff, cb));
nl_table = kcalloc(MAX_LINKS, sizeof(*nl_table), GFP_KERNEL);
- if (!nl_table)
+ if (!nl_table) {
+ err = -ENOMEM;
goto panic;
+ }
for (i = 0; i < MAX_LINKS; i++) {
if (rhashtable_init(&nl_table[i].hash,
--
2.17.1
Re: [PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower()
On 2021/3/9 16:24, Roi Dayan wrote:
On 2021-03-09 10:20 AM, Roi Dayan wrote:
On 2021-03-06 3:47 PM, Jia-Ju Bai wrote:
When mlx5e_tc_get_counter() returns NULL to counter or
mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return
code of mlx5e_stats_flower() is assigned.
To fix this bug, err is assigned with -EINVAL in these cases.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 0da69b98f38f..1f2c9da7bd35 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device
*dev, struct mlx5e_priv *priv,
if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) {
counter = mlx5e_tc_get_counter(flow);
- if (!counter)
+ if (!counter) {
+ err = -EINVAL;
goto errout;
+ }
mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse);
}
@@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device
*dev, struct mlx5e_priv *priv,
* un-offloaded while the other rule is offloaded.
*/
peer_esw = mlx5_devcom_get_peer_data(devcom,
MLX5_DEVCOM_ESW_OFFLOADS);
- if (!peer_esw)
+ if (!peer_esw) {
+ err = -EINVAL;
note here it's not an error. it could be there is no peer esw
so just continue with the stats update.
goto out;
+ }
if (flow_flag_test(flow, DUP) &&
flow_flag_test(flow->peer_flow, OFFLOADED)) {
@@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device
*dev, struct mlx5e_priv *priv,
u64 lastuse2;
counter = mlx5e_tc_get_counter(flow->peer_flow);
- if (!counter)
+ if (!counter) {
+ err = -EINVAL;
this change is problematic. the current goto is to do stats update with
the first counter stats we got but if you now want to return an error
then you probably should not do any update at all.
Thanks for your reply :)
I am not sure whether an error code should be returned here?
If so, flow_stats_update(...) should not be called here?
Best wishes,
Jia-Ju Bai
goto no_peer_counter;
+ }
mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2);
bytes += bytes2;
[PATCH] net: bridge: fix error return code of do_update_counters()
When find_table_lock() returns NULL to t, no error return code of
do_update_counters() is assigned.
To fix this bug, ret is assigned with -ENOENT in this case.
Fixes: 49facff9f925 ("netfilter: ebtables: split update_counters into two
functions")
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
net/bridge/netfilter/ebtables.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index ebe33b60efd6..66c9e4077985 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1256,8 +1256,10 @@ static int do_update_counters(struct net *net, const
char *name,
return -ENOMEM;
t = find_table_lock(net, name, &ret, &ebt_mutex);
- if (!t)
+ if (!t) {
+ ret = -ENOENT;
goto free_tmp;
+ }
if (num_counters != t->private->nentries) {
ret = -EINVAL;
--
2.17.1
Re: [PATCH] net: ieee802154: fix error return code of dgram_sendmsg()
On 2021/3/8 21:33, Heiner Kallweit wrote: On 08.03.2021 13:18, Jia-Ju Bai wrote: On 2021/3/8 18:19, Heiner Kallweit wrote: On 08.03.2021 10:31, Jia-Ju Bai wrote: When sock_alloc_send_skb() returns NULL to skb, no error return code of dgram_sendmsg() is assigned. To fix this bug, err is assigned with -ENOMEM in this case. Please stop sending such nonsense. Basically all such patches you sent so far are false positives. You have to start thinking, don't blindly trust your robot. In the case here the err variable is populated by sock_alloc_send_skb(). Ah, sorry, it is my fault :( I did not notice that the err variable is populated by sock_alloc_send_skb(). I will think more carefully before sending patches. By the way, I wonder how to report and discuss possible bugs that I am not quite sure of? Some people told me that sending patches is better than reporting bugs via Bugzilla, so I write the patches of these possible bugs... Do you have any advice? If you're quite sure that something is a bug then sending a patch is fine. Your submissions more or less all being false positives shows that this takes more than just forwarding bot findings, especially if you have no idea yet regarding the quality of the bot. Alternatively you can contact the maintainer and respective mailing list. But again, maintainers typically are very busy and you should have done all you can to analyze the suspected bug. What I'd do being in your shoes: Take the first 10 findings of a new bot and analyze in detail whether findings are correct or false positives. Of course this means you need to get familiar with the affected code in the respective driver. If false positive ratio is > 5% I wouldn't send out patches w/o more detailed analysis per finding. Worst case a maintainer is busy and can't review your submission in time, and the incorrect fix is applied and breaks the driver. Typically this shouldn't happen however because Dave/Jakub won't apply a patch w/o Ack from the respective maintainer. Disclaimer: I can only speak for myself. Other maintainers may see this differently. Okay, thanks a lot for the very helpful advice :) I will carefully check the bug report and try my best to write correct patches. Best wishes, Jia-Ju Bai
Re: [PATCH] net: ieee802154: fix error return code of dgram_sendmsg()
On 2021/3/8 18:19, Heiner Kallweit wrote:
On 08.03.2021 10:31, Jia-Ju Bai wrote:
When sock_alloc_send_skb() returns NULL to skb, no error return code of
dgram_sendmsg() is assigned.
To fix this bug, err is assigned with -ENOMEM in this case.
Please stop sending such nonsense. Basically all such patches you
sent so far are false positives. You have to start thinking,
don't blindly trust your robot.
In the case here the err variable is populated by sock_alloc_send_skb().
Ah, sorry, it is my fault :(
I did not notice that the err variable is populated by
sock_alloc_send_skb().
I will think more carefully before sending patches.
By the way, I wonder how to report and discuss possible bugs that I am
not quite sure of?
Some people told me that sending patches is better than reporting bugs
via Bugzilla, so I write the patches of these possible bugs...
Do you have any advice?
Thanks a lot!
Best wishes,
Jia-Ju Bai
Fixes: 78f821b64826 ("ieee802154: socket: put handling into one file")
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
net/ieee802154/socket.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index a45a0401adc5..a750b37c7e73 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -642,8 +642,10 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr
*msg, size_t size)
skb = sock_alloc_send_skb(sk, hlen + tlen + size,
msg->msg_flags & MSG_DONTWAIT,
&err);
- if (!skb)
+ if (!skb) {
+ err = -ENOMEM;
goto out_dev;
+ }
skb_reserve(skb, hlen);
[PATCH] net: ieee802154: fix error return code of dgram_sendmsg()
When sock_alloc_send_skb() returns NULL to skb, no error return code of
dgram_sendmsg() is assigned.
To fix this bug, err is assigned with -ENOMEM in this case.
Fixes: 78f821b64826 ("ieee802154: socket: put handling into one file")
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
net/ieee802154/socket.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index a45a0401adc5..a750b37c7e73 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -642,8 +642,10 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr
*msg, size_t size)
skb = sock_alloc_send_skb(sk, hlen + tlen + size,
msg->msg_flags & MSG_DONTWAIT,
&err);
- if (!skb)
+ if (!skb) {
+ err = -ENOMEM;
goto out_dev;
+ }
skb_reserve(skb, hlen);
--
2.17.1
[PATCH] net: ieee802154: fix error return code of raw_sendmsg()
When sock_alloc_send_skb() returns NULL to skb, no error return code of
raw_sendmsg() is assigned.
To fix this bug, err is assigned with -ENOMEM in this case.
Fixes: 78f821b64826 ("ieee802154: socket: put handling into one file")
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
net/ieee802154/socket.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index a45a0401adc5..3d76b207385e 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -277,8 +277,10 @@ static int raw_sendmsg(struct sock *sk, struct msghdr
*msg, size_t size)
tlen = dev->needed_tailroom;
skb = sock_alloc_send_skb(sk, hlen + tlen + size,
msg->msg_flags & MSG_DONTWAIT, &err);
- if (!skb)
+ if (!skb) {
+ err = -ENOMEM;
goto out_dev;
+ }
skb_reserve(skb, hlen);
--
2.17.1
[PATCH] net: qrtr: fix error return code of qrtr_sendmsg()
When sock_alloc_send_skb() returns NULL to skb, no error return code of
qrtr_sendmsg() is assigned.
To fix this bug, rc is assigned with -ENOMEM in this case.
Fixes: 194ccc88297a ("net: qrtr: Support decoding incoming v2 packets")
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
net/qrtr/qrtr.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index b34358282f37..ac2a4a7711da 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -958,8 +958,10 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr
*msg, size_t len)
plen = (len + 3) & ~3;
skb = sock_alloc_send_skb(sk, plen + QRTR_HDR_MAX_SIZE,
msg->msg_flags & MSG_DONTWAIT, &rc);
- if (!skb)
+ if (!skb) {
+ rc = -ENOMEM;
goto out_node;
+ }
skb_reserve(skb, QRTR_HDR_MAX_SIZE);
--
2.17.1
[PATCH] net: bonding: fix error return code of bond_neigh_init()
When slave is NULL or slave_ops->ndo_neigh_setup is NULL, no error
return code of bond_neigh_init() is assigned.
To fix this bug, ret is assigned with -EINVAL in these cases.
Fixes: 9e99bfefdbce ("bonding: fix bond_neigh_init()")
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/bonding/bond_main.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 74cbbb22470b..456315bef3a8 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3978,11 +3978,15 @@ static int bond_neigh_init(struct neighbour *n)
rcu_read_lock();
slave = bond_first_slave_rcu(bond);
- if (!slave)
+ if (!slave) {
+ ret = -EINVAL;
goto out;
+ }
slave_ops = slave->dev->netdev_ops;
- if (!slave_ops->ndo_neigh_setup)
+ if (!slave_ops->ndo_neigh_setup) {
+ ret = -EINVAL;
goto out;
+ }
/* TODO: find another way [1] to implement this.
* Passing a zeroed structure is fragile,
--
2.17.1
Re: [PATCH] ath: ath6kl: fix error return code of ath6kl_htc_rx_bundle()
Hi Leon,
I am quite sorry for my incorrect patches...
My static analysis tool reports some possible bugs about error handling
code, and thus I write some patches for the bugs that seem to be true in
my opinion.
Because I am not familiar with many device drivers, some of my reported
bugs can be false positives...
Best wishes,
Jia-Ju Bai
On 2021/3/7 17:18, Leon Romanovsky wrote:
On Sun, Mar 07, 2021 at 01:07:57AM -0800, Jia-Ju Bai wrote:
When hif_scatter_req_get() returns NULL to scat_req, no error return
code of ath6kl_htc_rx_bundle() is assigned.
To fix this bug, status is assigned with -EINVAL in this case.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/ath/ath6kl/htc_mbox.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
index 998947ef63b6..3f8857d19a0c 100644
--- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
+++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
@@ -1944,8 +1944,10 @@ static int ath6kl_htc_rx_bundle(struct htc_target
*target,
scat_req = hif_scatter_req_get(target->dev->ar);
- if (scat_req == NULL)
+ if (scat_req == NULL) {
+ status = -EINVAL;
I'm not sure about it.
David. Jakub,
Please be warned that patches from this guy are not so great.
I looked on 4 patches and 3 of them were wrong (2 in RDMA and 1 for mlx5)
plus this patch most likely is incorrect too.
[PATCH] net: wan: fix error return code of uhdlc_init()
When priv->rx_skbuff or priv->tx_skbuff is NULL, no error return code of
uhdlc_init() is assigned.
To fix this bug, ret is assigned with -ENOMEM in these cases.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/wan/fsl_ucc_hdlc.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index dca97cd7c4e7..7eac6a3e1cde 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -204,14 +204,18 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
priv->rx_skbuff = kcalloc(priv->rx_ring_size,
sizeof(*priv->rx_skbuff),
GFP_KERNEL);
- if (!priv->rx_skbuff)
+ if (!priv->rx_skbuff) {
+ ret = -ENOMEM;
goto free_ucc_pram;
+ }
priv->tx_skbuff = kcalloc(priv->tx_ring_size,
sizeof(*priv->tx_skbuff),
GFP_KERNEL);
- if (!priv->tx_skbuff)
+ if (!priv->tx_skbuff) {
+ ret = -ENOMEM;
goto free_rx_skbuff;
+ }
priv->skb_curtx = 0;
priv->skb_dirtytx = 0;
--
2.17.1
[PATCH] ath: ath6kl: fix error return code of ath6kl_htc_rx_bundle()
When hif_scatter_req_get() returns NULL to scat_req, no error return
code of ath6kl_htc_rx_bundle() is assigned.
To fix this bug, status is assigned with -EINVAL in this case.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/ath/ath6kl/htc_mbox.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
index 998947ef63b6..3f8857d19a0c 100644
--- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
+++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
@@ -1944,8 +1944,10 @@ static int ath6kl_htc_rx_bundle(struct htc_target
*target,
scat_req = hif_scatter_req_get(target->dev->ar);
- if (scat_req == NULL)
+ if (scat_req == NULL) {
+ status = -EINVAL;
goto fail_rx_pkt;
+ }
for (i = 0; i < n_scat_pkt; i++) {
int pad_len;
--
2.17.1
[PATCH] net: hisilicon: hns: fix error return code of hns_nic_clear_all_rx_fetch()
When hns_assemble_skb() returns NULL to skb, no error return code of
hns_nic_clear_all_rx_fetch() is assigned.
To fix this bug, ret is assigned with -ENOMEM in this case.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index 5d7824d2b4d4..c66a7a51198e 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -1663,8 +1663,10 @@ static int hns_nic_clear_all_rx_fetch(struct net_device
*ndev)
for (j = 0; j < fetch_num; j++) {
/* alloc one skb and init */
skb = hns_assemble_skb(ndev);
- if (!skb)
+ if (!skb) {
+ ret = -ENOMEM;
goto out;
+ }
rd = &tx_ring_data(priv, skb->queue_mapping);
hns_nic_net_xmit_hw(ndev, skb, rd);
--
2.17.1
[PATCH] rsi: fix error return code of rsi_load_9116_firmware()
When kmemdup() returns NULL to ta_firmware, no error return code of
rsi_load_9116_firmware() is assigned.
To fix this bug, status is assigned with -ENOMEM in this case.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/rsi/rsi_91x_hal.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c
b/drivers/net/wireless/rsi/rsi_91x_hal.c
index ce9892152f4d..32ecb8b3d6c5 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -1038,8 +1038,10 @@ static int rsi_load_9116_firmware(struct rsi_hw *adapter)
}
ta_firmware = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
- if (!ta_firmware)
+ if (!ta_firmware) {
+ status = -ENOMEM;
goto fail_release_fw;
+ }
fw_p = ta_firmware;
instructions_sz = fw_entry->size;
rsi_dbg(INFO_ZONE, "FW Length = %d bytes\n", instructions_sz);
--
2.17.1
[PATCH] ti: wlcore: fix error return code of wl1271_cmd_build_ps_poll()
When ieee80211_pspoll_get() returns NULL to skb, no error return code of
wl1271_cmd_build_ps_poll() is assigned.
To fix this bug, ret is assigned with -ENOMEM in this case.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/ti/wlcore/cmd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ti/wlcore/cmd.c
b/drivers/net/wireless/ti/wlcore/cmd.c
index 32a2e27cc561..7bf8b8201fdd 100644
--- a/drivers/net/wireless/ti/wlcore/cmd.c
+++ b/drivers/net/wireless/ti/wlcore/cmd.c
@@ -1120,8 +1120,10 @@ int wl1271_cmd_build_ps_poll(struct wl1271 *wl, struct
wl12xx_vif *wlvif,
int ret = 0;
skb = ieee80211_pspoll_get(wl->hw, vif);
- if (!skb)
+ if (!skb) {
+ ret = -ENOMEM;
goto out;
+ }
ret = wl1271_cmd_template_set(wl, wlvif->role_id,
CMD_TEMPL_PS_POLL, skb->data,
--
2.17.1
[PATCH] ti: wlcore: fix error return code of wl1271_suspend()
When wl is NULL, no error return code of wl1271_suspend() is assigned.
To fix this bug, ret is assigned with -EINVAL in this case.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/ti/wlcore/sdio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/ti/wlcore/sdio.c
b/drivers/net/wireless/ti/wlcore/sdio.c
index 9fd8cf2d270c..a040d595a43a 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -390,6 +390,7 @@ static int wl1271_suspend(struct device *dev)
if (!wl) {
dev_err(dev, "no wilink module was probed\n");
+ ret = -EINVAL;
goto out;
}
--
2.17.1
[PATCH] net: mellanox: mlxsw: fix error return code of mlxsw_sp_router_nve_promote_decap()
When fib_entry is NULL, no error return code of
mlxsw_sp_router_nve_promote_decap() is assigned.
To fix this bug, err is assigned with -EINVAL in this case.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 9ce90841f92d..7b260e25df1b 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -1981,8 +1981,10 @@ int mlxsw_sp_router_nve_promote_decap(struct mlxsw_sp
*mlxsw_sp, u32 ul_tb_id,
fib_entry = mlxsw_sp_router_ip2me_fib_entry_find(mlxsw_sp, ul_tb_id,
ul_proto, ul_sip,
type);
- if (!fib_entry)
+ if (!fib_entry) {
+ err = -EINVAL;
goto out;
+ }
fib_entry->decap.tunnel_index = tunnel_index;
fib_entry->type = MLXSW_SP_FIB_ENTRY_TYPE_NVE_DECAP;
--
2.17.1
[PATCH] net: mellanox: mlx5: fix error return code of mlx5e_stats_flower()
When mlx5e_tc_get_counter() returns NULL to counter or
mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return
code of mlx5e_stats_flower() is assigned.
To fix this bug, err is assigned with -EINVAL in these cases.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 0da69b98f38f..1f2c9da7bd35 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct
mlx5e_priv *priv,
if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) {
counter = mlx5e_tc_get_counter(flow);
- if (!counter)
+ if (!counter) {
+ err = -EINVAL;
goto errout;
+ }
mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse);
}
@@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct
mlx5e_priv *priv,
* un-offloaded while the other rule is offloaded.
*/
peer_esw = mlx5_devcom_get_peer_data(devcom, MLX5_DEVCOM_ESW_OFFLOADS);
- if (!peer_esw)
+ if (!peer_esw) {
+ err = -EINVAL;
goto out;
+ }
if (flow_flag_test(flow, DUP) &&
flow_flag_test(flow->peer_flow, OFFLOADED)) {
@@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct
mlx5e_priv *priv,
u64 lastuse2;
counter = mlx5e_tc_get_counter(flow->peer_flow);
- if (!counter)
+ if (!counter) {
+ err = -EINVAL;
goto no_peer_counter;
+ }
mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2);
bytes += bytes2;
--
2.17.1
[PATCH] net: smc: fix error return code of smc_diag_dump_proto()
When the list of head is empty, no error return code of
smc_diag_dump_proto() is assigned.
To fix this bug, rc is assigned with -ENOENT as error return code.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
net/smc/smc_diag.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index c952986a6aca..a90889482842 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -201,8 +201,10 @@ static int smc_diag_dump_proto(struct proto *prot, struct
sk_buff *skb,
read_lock(&prot->h.smc_hash->lock);
head = &prot->h.smc_hash->ht;
- if (hlist_empty(head))
+ if (hlist_empty(head)) {
+ rc = -ENOENT;
goto out;
+ }
sk_for_each(sk, head) {
if (!net_eq(sock_net(sk), net))
--
2.17.1
[PATCH] net: xdp: fix error return code of xsk_generic_xmit()
When err is zero but xskq_prod_reserve() fails, no error return code of
xsk_generic_xmit() is assigned.
To fix this bug, err is assigned with the return value of
xskq_prod_reserve(), and then err is checked.
The spinlock is only used to protect the call to xskq_prod_reserve().
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
net/xdp/xsk.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 4faabd1ecfd1..f1c1db07dd07 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -484,8 +484,14 @@ static int xsk_generic_xmit(struct sock *sk)
* if there is space in it. This avoids having to implement
* any buffering in the Tx path.
*/
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ goto out;
+ }
+
spin_lock_irqsave(&xs->pool->cq_lock, flags);
- if (unlikely(err) || xskq_prod_reserve(xs->pool->cq)) {
+ err = xskq_prod_reserve(xs->pool->cq);
+ if (unlikely(err)) {
spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
kfree_skb(skb);
goto out;
--
2.17.1
[PATCH] marvell: libertas_tf: fix error return code of if_usb_prog_firmware()
When check_fwfile_format() fails, no error return code of
if_usb_prog_firmware() is assigned.
To fix this bug, ret is assigned with -EINVAL as error return code.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/marvell/libertas_tf/if_usb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/libertas_tf/if_usb.c
b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
index a92916dc81a9..ceca22da5a29 100644
--- a/drivers/net/wireless/marvell/libertas_tf/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
@@ -825,8 +825,10 @@ static int if_usb_prog_firmware(struct lbtf_private *priv)
}
kernel_param_unlock(THIS_MODULE);
- if (check_fwfile_format(cardp->fw->data, cardp->fw->size))
+ if (check_fwfile_format(cardp->fw->data, cardp->fw->size)) {
+ ret = -EINVAL;
goto release_fw;
+ }
restart:
if (if_usb_submit_rx_urb_fwload(cardp) < 0) {
--
2.17.1
[PATCH] net: intel: iavf: fix error return code of iavf_init_get_resources()
When iavf_process_config() fails, no error return code of iavf_init_get_resources() is assigned. To fix this bug, err is assigned with the return value of iavf_process_config(), and then err is checked. Reported-by: TOTE Robot Signed-off-by: Jia-Ju Bai --- drivers/net/ethernet/intel/iavf/iavf_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c index 0a867d64d467..dc5b3c06d1e0 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_main.c +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c @@ -1776,7 +1776,8 @@ static int iavf_init_get_resources(struct iavf_adapter *adapter) goto err_alloc; } - if (iavf_process_config(adapter)) + err = iavf_process_config(adapter); + if (err) goto err_alloc; adapter->current_op = VIRTCHNL_OP_UNKNOWN; -- 2.17.1
[PATCH] net: tehuti: fix error return code in bdx_probe()
When bdx_read_mac() fails, no error return code of bdx_probe()
is assigned.
To fix this bug, err is assigned with -EFAULT as error return code.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/tehuti/tehuti.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/tehuti/tehuti.c
b/drivers/net/ethernet/tehuti/tehuti.c
index b8f4f419173f..d054c6e83b1c 100644
--- a/drivers/net/ethernet/tehuti/tehuti.c
+++ b/drivers/net/ethernet/tehuti/tehuti.c
@@ -2044,6 +2044,7 @@ bdx_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
/*bdx_hw_reset(priv); */
if (bdx_read_mac(priv)) {
pr_err("load MAC address failed\n");
+ err = -EFAULT;
goto err_out_iomap;
}
SET_NETDEV_DEV(ndev, &pdev->dev);
--
2.17.1
[PATCH] net: mellanox: mlx5: fix error return code in mlx5_fpga_device_start()
When mlx5_is_fpga_lookaside() returns a non-zero value, no error
return code is assigned.
To fix this bug, err is assigned with -EINVAL as error return code.
Reported-by: TOTE Robot
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c
b/drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c
index 2ce4241459ce..c9e6da97126f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/core.c
@@ -198,8 +198,10 @@ int mlx5_fpga_device_start(struct mlx5_core_dev *mdev)
mlx5_fpga_info(fdev, "FPGA card %s:%u\n", mlx5_fpga_name(fpga_id),
fpga_id);
/* No QPs if FPGA does not participate in net processing */
- if (mlx5_is_fpga_lookaside(fpga_id))
+ if (mlx5_is_fpga_lookaside(fpga_id)) {
+ err = -EINVAL;
goto out;
+ }
mlx5_fpga_info(fdev, "%s(%d): image, version %u; SBU %06x:%04x version
%d\n",
mlx5_fpga_image_name(fdev->last_oper_image),
--
2.17.1
Re: [PATCH v2 1/4] rtlwifi: rtl8188ee: avoid accessing the data mapped to streaming DMA
Thanks for the advice. I have added the description of the changes and resent the patches. Best wishes, Jia-Ju Bai On 2020/11/19 1:20, Larry Finger wrote: On 11/17/20 7:53 PM, Jia-Ju Bai wrote: In rtl88ee_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on line 677: dma_addr_t mapping = dma_map_single(..., skb->data, ...); On line 680, skb->data is assigned to hdr after cast: struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data); Then hdr->frame_control is accessed on line 681: __le16 fc = hdr->frame_control; This DMA access may cause data inconsistency between CPU and hardwre. To fix this bug, hdr->frame_control is accessed before the DMA mapping. Signed-off-by: Jia-Ju Bai --- drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) What changed between v1 and v2? As outlined in Documentation/process/submitting-patches.rst, you should add a '---' marker and descrive what was changed. I usually summarize the changes, but it is also possible to provide a diffstat of the changes as the above file shows.
[PATCH v2 3/4 resend] rtlwifi: rtl8192de: avoid accessing the data mapped to streaming DMA
In rtl92de_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 667:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 669, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 670:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
v2:
* Use "rtlwifi" as subject prefix and have all rtlwifi patches in the
same pathset.
Thank Ping and Larry for good advice.
---
drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
index 8944712274b5..c02813fba934 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
@@ -664,12 +664,14 @@ void rtl92de_tx_fill_cmddesc(struct ieee80211_hw *hw,
struct rtl_ps_ctl *ppsc = rtl_psc(rtlpriv);
struct rtl_hal *rtlhal = rtl_hal(rtlpriv);
u8 fw_queue = QSLT_BEACON;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
+
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
__le32 *pdesc = (__le32 *)pdesc8;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH v2 2/4 resend] rtlwifi: rtl8192ce: avoid accessing the data mapped to streaming DMA
In rtl92ce_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 530:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 533, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 534:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
v2:
* Use "rtlwifi" as subject prefix and have all rtlwifi patches in the
same pathset.
Thank Ping and Larry for good advice.
---
drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
index c0635309a92d..4165175cf5c0 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
@@ -527,12 +527,12 @@ void rtl92ce_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH v2 4/4 resend] rtlwifi: rtl8723ae: avoid accessing the data mapped to streaming DMA
In rtl8723e_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 531:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 534, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 535:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
v2:
* Use "rtlwifi" as subject prefix and have all rtlwifi patches in the
same pathset.
Thank Ping and Larry for good advice.
---
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
index e3ee91b7ea8d..340b3d68a54e 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
@@ -528,12 +528,12 @@ void rtl8723e_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH v2 1/4 resend] rtlwifi: rtl8188ee: avoid accessing the data mapped to streaming DMA
In rtl88ee_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 677:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 680, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 681:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
v2:
* Use "rtlwifi" as subject prefix and have all rtlwifi patches in the
same pathset.
Thank Ping and Larry for good advice.
---
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
index b9775eec4c54..c948dafa0c80 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
@@ -674,12 +674,12 @@ void rtl88ee_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH v2 4/4] rtlwifi: rtl8723ae: avoid accessing the data mapped to streaming DMA
In rtl8723e_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 531:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 534, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 535:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
index e3ee91b7ea8d..340b3d68a54e 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
@@ -528,12 +528,12 @@ void rtl8723e_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
Re: [PATCH] rtl8192ce: avoid accessing the data mapped to streaming DMA
On 2020/11/7 19:44, Kalle Valo wrote: Jia-Ju Bai wrote: In rtl92ce_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on line 530: dma_addr_t mapping = dma_map_single(..., skb->data, ...); On line 533, skb->data is assigned to hdr after cast: struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data); Then hdr->frame_control is accessed on line 534: __le16 fc = hdr->frame_control; This DMA access may cause data inconsistency between CPU and hardwre. To fix this bug, hdr->frame_control is accessed before the DMA mapping. Signed-off-by: Jia-Ju Bai Like Ping said, use "rtlwifi:" prefix and have all rtlwifi patches in the same patchset. 4 patches set to Changes Requested. 11843533 rtl8192ce: avoid accessing the data mapped to streaming DMA 11843541 rtl8192de: avoid accessing the data mapped to streaming DMA 11843553 rtl8723ae: avoid accessing the data mapped to streaming DMA 11843557 rtl8188ee: avoid accessing the data mapped to streaming DMA Okay, I have sent v2 patches just now. Please have a look, thank :) Best wishes, Jia-Ju Bai
[PATCH v2 3/4] rtlwifi: rtl8192de: avoid accessing the data mapped to streaming DMA
In rtl92de_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 667:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 669, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 670:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
index 8944712274b5..c02813fba934 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
@@ -664,12 +664,14 @@ void rtl92de_tx_fill_cmddesc(struct ieee80211_hw *hw,
struct rtl_ps_ctl *ppsc = rtl_psc(rtlpriv);
struct rtl_hal *rtlhal = rtl_hal(rtlpriv);
u8 fw_queue = QSLT_BEACON;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
+
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
__le32 *pdesc = (__le32 *)pdesc8;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH v2 2/4] rtlwifi: rtl8192ce: avoid accessing the data mapped to streaming DMA
In rtl92ce_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 530:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 533, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 534:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
index c0635309a92d..4165175cf5c0 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
@@ -527,12 +527,12 @@ void rtl92ce_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH v2 1/4] rtlwifi: rtl8188ee: avoid accessing the data mapped to streaming DMA
In rtl88ee_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 677:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 680, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 681:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
index b9775eec4c54..c948dafa0c80 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
@@ -674,12 +674,12 @@ void rtl88ee_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH] rtl8188ee: avoid accessing the data mapped to streaming DMA
In rtl88ee_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 677:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 680, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 681:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
index b9775eec4c54..c948dafa0c80 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
@@ -674,12 +674,12 @@ void rtl88ee_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH] rtl8723ae: avoid accessing the data mapped to streaming DMA
In rtl8723e_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 531:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 534, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 535:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
index e3ee91b7ea8d..340b3d68a54e 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/trx.c
@@ -528,12 +528,12 @@ void rtl8723e_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH] rtl8192de: avoid accessing the data mapped to streaming DMA
In rtl92de_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 667:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 669, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 670:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
index 8944712274b5..c02813fba934 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/trx.c
@@ -664,12 +664,14 @@ void rtl92de_tx_fill_cmddesc(struct ieee80211_hw *hw,
struct rtl_ps_ctl *ppsc = rtl_psc(rtlpriv);
struct rtl_hal *rtlhal = rtl_hal(rtlpriv);
u8 fw_queue = QSLT_BEACON;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
+
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
__le32 *pdesc = (__le32 *)pdesc8;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH] rtl8192ce: avoid accessing the data mapped to streaming DMA
In rtl92ce_tx_fill_cmddesc(), skb->data is mapped to streaming DMA on
line 530:
dma_addr_t mapping = dma_map_single(..., skb->data, ...);
On line 533, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
Then hdr->frame_control is accessed on line 534:
__le16 fc = hdr->frame_control;
This DMA access may cause data inconsistency between CPU and hardwre.
To fix this bug, hdr->frame_control is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
index c0635309a92d..4165175cf5c0 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ce/trx.c
@@ -527,12 +527,12 @@ void rtl92ce_tx_fill_cmddesc(struct ieee80211_hw *hw,
u8 fw_queue = QSLT_BEACON;
__le32 *pdesc = (__le32 *)pdesc8;
- dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
-
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)(skb->data);
__le16 fc = hdr->frame_control;
+ dma_addr_t mapping = dma_map_single(&rtlpci->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+
if (dma_mapping_error(&rtlpci->pdev->dev, mapping)) {
rtl_dbg(rtlpriv, COMP_SEND, DBG_TRACE,
"DMA mapping error\n");
--
2.17.1
[PATCH] rtl8180: avoid accessing the data mapped to streaming DMA
In rtl8180_tx(), skb->data is mapped to streaming DMA on line 476:
mapping = dma_map_single(..., skb->data, ...);
On line 459, skb->data is assigned to hdr after cast:
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
Then hdr->seq_ctrl is accessed on lines 540 and 541:
hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
hdr->seq_ctrl |= cpu_to_le16(priv->seqno);
These DMA accesses may cause data inconsistency between CPU and hardwre.
To fix this problem, hdr->seq_ctrl is accessed before the DMA mapping.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
index 2477e18c7cae..cc73014aa5f3 100644
--- a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
@@ -473,6 +473,13 @@ static void rtl8180_tx(struct ieee80211_hw *dev,
prio = skb_get_queue_mapping(skb);
ring = &priv->tx_ring[prio];
+ if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
+ if (info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT)
+ priv->seqno += 0x10;
+ hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
+ hdr->seq_ctrl |= cpu_to_le16(priv->seqno);
+ }
+
mapping = dma_map_single(&priv->pdev->dev, skb->data, skb->len,
DMA_TO_DEVICE);
@@ -534,13 +541,6 @@ static void rtl8180_tx(struct ieee80211_hw *dev,
spin_lock_irqsave(&priv->lock, flags);
- if (info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ) {
- if (info->flags & IEEE80211_TX_CTL_FIRST_FRAGMENT)
- priv->seqno += 0x10;
- hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG);
- hdr->seq_ctrl |= cpu_to_le16(priv->seqno);
- }
-
idx = (ring->idx + skb_queue_len(&ring->queue)) % ring->entries;
entry = &ring->desc[idx];
--
2.17.1
Re: [PATCH] net: vmxnet3: avoid accessing the data mapped to streaming DMA
On 2020/8/4 6:59, David Miller wrote: From: Jia-Ju Bai Date: Sun, 2 Aug 2020 21:11:07 +0800 In vmxnet3_probe_device(), "adapter" is mapped to streaming DMA: adapter->adapter_pa = dma_map_single(..., adapter, ...); Then "adapter" is accessed at many places in this function. Theses accesses may cause data inconsistency between CPU cache and hardware. To fix this problem, dma_map_single() is called after these accesses. Signed-off-by: Jia-Ju Bai 'adapter' is accessed everywhere, in the entire driver, not just here in the probe function. Okay, replacing dma_map_single() with dma_alloc_coherent() may be better. If you think this solution is okay, I can submit a new patch. Best wishes, Jia-Ju Bai
[PATCH] net: sfc: fix possible buffer overflow caused by bad DMA value in efx_siena_sriov_vfdi()
In efx_siena_sriov_vfdi():
req = vf->buf.addr;
Because "vf->buf.addr" is mapped to coherent DMA (allocated in
efx_nic_alloc_buffer()), "req" is also mapped to DMA.
Then "req->op" is accessed in this function:
if (req->op < VFDI_OP_LIMIT && vfdi_ops[req->op] != NULL) {
rc = vfdi_ops[req->op](vf);
Because "req" is mapped to DMA, its data can be modified at any time by
malicious or malfunctioning hardware. In this case, the check
"if (req->op < VFDI_OP_LIMIT)" can be passed, and then "req->op" can be
modified to cause buffer overflow when the driver accesses
"vfdi_ops[req->op]".
To fix this problem, "req->op" is assigned to a local variable, and then
the driver accesses this variable instead of "req->op".
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/sfc/siena_sriov.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/sfc/siena_sriov.c
b/drivers/net/ethernet/sfc/siena_sriov.c
index 83dcfcae3d4b..21a8482cbb3b 100644
--- a/drivers/net/ethernet/sfc/siena_sriov.c
+++ b/drivers/net/ethernet/sfc/siena_sriov.c
@@ -875,6 +875,7 @@ static void efx_siena_sriov_vfdi(struct work_struct *work)
struct vfdi_req *req = vf->buf.addr;
struct efx_memcpy_req copy[2];
int rc;
+ u32 op = req->op;
/* Copy this page into the local address space */
memset(copy, '\0', sizeof(copy));
@@ -894,17 +895,17 @@ static void efx_siena_sriov_vfdi(struct work_struct *work)
return;
}
- if (req->op < VFDI_OP_LIMIT && vfdi_ops[req->op] != NULL) {
- rc = vfdi_ops[req->op](vf);
+ if (op < VFDI_OP_LIMIT && vfdi_ops[op] != NULL) {
+ rc = vfdi_ops[op](vf);
if (rc == 0) {
netif_dbg(efx, hw, efx->net_dev,
"vfdi request %d from %s ok\n",
- req->op, vf->pci_name);
+ op, vf->pci_name);
}
} else {
netif_dbg(efx, hw, efx->net_dev,
"ERROR: Unrecognised request %d from VF %s addr "
- "%llx\n", req->op, vf->pci_name,
+ "%llx\n", op, vf->pci_name,
(unsigned long long)vf->req_addr);
rc = VFDI_RC_EOPNOTSUPP;
}
--
2.17.1
[BUG] net: rocker: accessing the data mapped to streaming DMA
In rocker_dma_test_offset(), "buf" is mapped to streaming DMA on line 203: dma_handle = pci_map_single(..., buf, ...); Then, "buf" is accessed on line 229: expect[i] = ~buf[i]; This access may cause data inconsistency between CPU cache and hardware. I am not sure how to properly fix this problem, and thus I only report it. Best wishes, Jia-Ju Bai
[PATCH] p54: avoid accessing the data mapped to streaming DMA
In p54p_tx(), skb->data is mapped to streaming DMA on line 337: mapping = pci_map_single(..., skb->data, ...); Then skb->data is accessed on line 349: desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; This access may cause data inconsistency between CPU cache and hardware. To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in a local variable before DMA mapping, and then the driver accesses this local variable instead of skb->data. Signed-off-by: Jia-Ju Bai --- drivers/net/wireless/intersil/p54/p54pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/wireless/intersil/p54/p54pci.c b/drivers/net/wireless/intersil/p54/p54pci.c index 80ad0b7eaef4..f8c6027cab6b 100644 --- a/drivers/net/wireless/intersil/p54/p54pci.c +++ b/drivers/net/wireless/intersil/p54/p54pci.c @@ -329,10 +329,12 @@ static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb) struct p54p_desc *desc; dma_addr_t mapping; u32 idx, i; + __le32 device_addr; spin_lock_irqsave(&priv->lock, flags); idx = le32_to_cpu(ring_control->host_idx[1]); i = idx % ARRAY_SIZE(ring_control->tx_data); + device_addr = ((struct p54_hdr *)skb->data)->req_id; mapping = pci_map_single(priv->pdev, skb->data, skb->len, PCI_DMA_TODEVICE); @@ -346,7 +348,7 @@ static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb) desc = &ring_control->tx_data[i]; desc->host_addr = cpu_to_le32(mapping); - desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; + desc->device_addr = device_addr; desc->len = cpu_to_le16(skb->len); desc->flags = 0; -- 2.17.1
[PATCH] net: vmxnet3: avoid accessing the data mapped to streaming DMA
In vmxnet3_probe_device(), "adapter" is mapped to streaming DMA:
adapter->adapter_pa = dma_map_single(..., adapter, ...);
Then "adapter" is accessed at many places in this function.
Theses accesses may cause data inconsistency between CPU cache and
hardware.
To fix this problem, dma_map_single() is called after these accesses.
Signed-off-by: Jia-Ju Bai
---
drivers/net/vmxnet3/vmxnet3_drv.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c
b/drivers/net/vmxnet3/vmxnet3_drv.c
index ca395f9679d0..96a4c28ba429 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -3445,14 +3445,6 @@ vmxnet3_probe_device(struct pci_dev *pdev,
}
spin_lock_init(&adapter->cmd_lock);
- adapter->adapter_pa = dma_map_single(&adapter->pdev->dev, adapter,
-sizeof(struct vmxnet3_adapter),
-PCI_DMA_TODEVICE);
- if (dma_mapping_error(&adapter->pdev->dev, adapter->adapter_pa)) {
- dev_err(&pdev->dev, "Failed to map dma\n");
- err = -EFAULT;
- goto err_set_mask;
- }
adapter->shared = dma_alloc_coherent(
&adapter->pdev->dev,
sizeof(struct Vmxnet3_DriverShared),
@@ -3628,6 +3620,16 @@ vmxnet3_probe_device(struct pci_dev *pdev,
}
vmxnet3_check_link(adapter, false);
+
+ adapter->adapter_pa = dma_map_single(&adapter->pdev->dev, adapter,
+sizeof(struct vmxnet3_adapter),
+PCI_DMA_TODEVICE);
+ if (dma_mapping_error(&adapter->pdev->dev, adapter->adapter_pa)) {
+ dev_err(&pdev->dev, "Failed to map dma\n");
+ err = -EFAULT;
+ goto err_register;
+ }
+
return 0;
err_register:
@@ -3655,8 +3657,6 @@ vmxnet3_probe_device(struct pci_dev *pdev,
sizeof(struct Vmxnet3_DriverShared),
adapter->shared, adapter->shared_pa);
err_alloc_shared:
- dma_unmap_single(&adapter->pdev->dev, adapter->adapter_pa,
-sizeof(struct vmxnet3_adapter), PCI_DMA_TODEVICE);
err_set_mask:
free_netdev(netdev);
return err;
--
2.17.1
[PATCH] atm: idt77252: avoid accessing the data mapped to streaming DMA
In queue_skb(), skb->data is mapped to streaming DMA on line 850:
dma_map_single(..., skb->data, ...);
Then skb->data is accessed on lines 862 and 863:
tbd->word_4 = (skb->data[0] << 24) | (skb->data[1] << 16) |
(skb->data[2] << 8) | (skb->data[3] << 0);
and on lines 893 and 894:
tbd->word_4 = (skb->data[0] << 24) | (skb->data[1] << 16) |
(skb->data[2] << 8) | (skb->data[3] << 0);
These accesses may cause data inconsistency between CPU cache and
hardware.
To fix this problem, the calculation result of skb->data is stored in a
local variable before DMA mapping, and then the driver accesses this
local variable instead of skb->data.
Signed-off-by: Jia-Ju Bai
---
drivers/atm/idt77252.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/atm/idt77252.c b/drivers/atm/idt77252.c
index df51680e8931..65a3886f68c9 100644
--- a/drivers/atm/idt77252.c
+++ b/drivers/atm/idt77252.c
@@ -835,6 +835,7 @@ queue_skb(struct idt77252_dev *card, struct vc_map *vc,
unsigned long flags;
int error;
int aal;
+ u32 word4;
if (skb->len == 0) {
printk("%s: invalid skb->len (%d)\n", card->name, skb->len);
@@ -846,6 +847,8 @@ queue_skb(struct idt77252_dev *card, struct vc_map *vc,
tbd = &IDT77252_PRV_TBD(skb);
vcc = ATM_SKB(skb)->vcc;
+ word4 = (skb->data[0] << 24) | (skb->data[1] << 16) |
+ (skb->data[2] << 8) | (skb->data[3] << 0);
IDT77252_PRV_PADDR(skb) = dma_map_single(&card->pcidev->dev, skb->data,
skb->len, DMA_TO_DEVICE);
@@ -859,8 +862,7 @@ queue_skb(struct idt77252_dev *card, struct vc_map *vc,
tbd->word_1 = SAR_TBD_OAM | ATM_CELL_PAYLOAD | SAR_TBD_EPDU;
tbd->word_2 = IDT77252_PRV_PADDR(skb) + 4;
tbd->word_3 = 0x;
- tbd->word_4 = (skb->data[0] << 24) | (skb->data[1] << 16) |
- (skb->data[2] << 8) | (skb->data[3] << 0);
+ tbd->word_4 = word4;
if (test_bit(VCF_RSV, &vc->flags))
vc = card->vcs[0];
@@ -890,8 +892,7 @@ queue_skb(struct idt77252_dev *card, struct vc_map *vc,
tbd->word_2 = IDT77252_PRV_PADDR(skb) + 4;
tbd->word_3 = 0x;
- tbd->word_4 = (skb->data[0] << 24) | (skb->data[1] << 16) |
- (skb->data[2] << 8) | (skb->data[3] << 0);
+ tbd->word_4 = word4;
break;
case ATM_AAL5:
--
2.17.1
[PATCH] atm: eni: avoid accessing the data mapped to streaming DMA
In do_tx(), skb->data is mapped to streaming DMA on line :
paddr = dma_map_single(...,skb->data,DMA_TO_DEVICE);
Then skb->data is accessed on line 1153:
(skb->data[3] & 0xf)
This access may cause data inconsistency between CPU cache and hardware.
To fix this problem, skb->data[3] is assigned to a local variable before
DMA mapping, and then the driver accesses this local variable instead of
skb->data[3].
Signed-off-by: Jia-Ju Bai
---
drivers/atm/eni.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
index 17d47ad03ab7..09f4e2f41363 100644
--- a/drivers/atm/eni.c
+++ b/drivers/atm/eni.c
@@ -1034,6 +1034,7 @@ static enum enq_res do_tx(struct sk_buff *skb)
u32 dma_rd,dma_wr;
u32 size; /* in words */
int aal5,dma_size,i,j;
+ unsigned char skb_data3;
DPRINTK(">do_tx\n");
NULLCHECK(skb);
@@ -1108,6 +1109,7 @@ DPRINTK("iovcnt = %d\n",skb_shinfo(skb)->nr_frags);
vcc->dev->number);
return enq_jam;
}
+ skb_data3 = skb->data[3];
paddr = dma_map_single(&eni_dev->pci_dev->dev,skb->data,skb->len,
DMA_TO_DEVICE);
ENI_PRV_PADDR(skb) = paddr;
@@ -1150,7 +1152,7 @@ DPRINTK("doing direct send\n"); /* @@@ well, this doesn't
work anyway */
(size/(ATM_CELL_PAYLOAD/4)),tx->send+tx->tx_pos*4);
/*printk("dsc = 0x%08lx\n",(unsigned long) readl(tx->send+tx->tx_pos*4));*/
writel((vcc->vci << MID_SEG_VCI_SHIFT) |
-(aal5 ? 0 : (skb->data[3] & 0xf)) |
+(aal5 ? 0 : (skb_data3 & 0xf)) |
(ATM_SKB(skb)->atm_options & ATM_ATMOPT_CLP ? MID_SEG_CLP : 0),
tx->send+((tx->tx_pos+1) & (tx->words-1))*4);
DPRINTK("size: %d, len:%d\n",size,skb->len);
--
2.17.1
Rule about streaming DMA mapping
Hello,
From the book "Linux device drivers" (3rd edition), I find an
interesting rule for streaming DMA mapping:
Once a buffer has been mapped, it belongs to the device, not the
processor. Until
the buffer has been unmapped, the driver should not touch its contents
in any
way. Only after dma_unmap_single has been called is it safe for the
driver to
access the contents of the buffer (with one exception that we see shortly).
Among other things, this rule implies that a buffer being written to a
device cannot
be mapped until it contains all the data to write.
I find some violations about this rule, and there are two examples in
Linux-5.6:
=== EXAMPLE 1 ===
In vmxnet3_probe_device() in drivers/net/vmxnet3/vmxnet3_drv.c:
adapter->adapter_pa = dma_map_single(&adapter->pdev->dev, adapter,
sizeof(struct vmxnet3_adapter),
PCI_DMA_TODEVICE);
if (dma_mapping_error(&adapter->pdev->dev, adapter->adapter_pa)) {
dev_err(&pdev->dev, "Failed to map dma\n");
err = -EFAULT;
goto err_set_mask;
}
adapter->shared = dma_alloc_coherent(
&adapter->pdev->dev,
sizeof(struct Vmxnet3_DriverShared),
&adapter->shared_pa, GFP_KERNEL);
if (!adapter->shared) {
dev_err(&pdev->dev, "Failed to allocate memory\n");
err = -ENOMEM;
goto err_alloc_shared;
}
adapter->num_rx_queues = num_rx_queues;
adapter->num_tx_queues = num_tx_queues;
adapter->rx_buf_per_pkt = 1;
The variable "adapter" is mapped to streaming DMA, but its fields are
used before this variable is unmapped.
=== EXAMPLE 2 ===
In queue_skb() in drivers/atm/idt77252.c:
IDT77252_PRV_PADDR(skb) = dma_map_single(&card->pcidev->dev, skb->data,
skb->len, DMA_TO_DEVICE);
error = -EINVAL;
if (oam) {
if (skb->len != 52)
goto errout;
tbd->word_1 = SAR_TBD_OAM | ATM_CELL_PAYLOAD | SAR_TBD_EPDU;
tbd->word_2 = IDT77252_PRV_PADDR(skb) + 4;
tbd->word_3 = 0x;
tbd->word_4 = (skb->data[0] << 24) | (skb->data[1] << 16) |
(skb->data[2] << 8) | (skb->data[3] << 0);
The array "skb->data" is mapped to streaming DMA, but its elements are
used before this array is unmapped.
Because I am not familiar with streaming DMA mapping, I wonder whether
these violations are real?
If they are real, what problems can they cause?
Thanks a lot :)
Best wishes,
Jia-Ju Bai
[PATCH] net: vmxnet3: fix possible buffer overflow caused by bad DMA value in vmxnet3_get_rss()
The value adapter->rss_conf is stored in DMA memory, and it is assigned to rssConf, so rssConf->indTableSize can be modified at anytime by malicious hardware. Because rssConf->indTableSize is assigned to n, buffer overflow may occur when the code "rssConf->indTable[n]" is executed. To fix this possible bug, n is checked after being used. Signed-off-by: Jia-Ju Bai --- drivers/net/vmxnet3/vmxnet3_ethtool.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c index 6528940ce5f3..b53bb8bcd47f 100644 --- a/drivers/net/vmxnet3/vmxnet3_ethtool.c +++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c @@ -700,6 +700,8 @@ vmxnet3_get_rss(struct net_device *netdev, u32 *p, u8 *key, u8 *hfunc) *hfunc = ETH_RSS_HASH_TOP; if (!p) return 0; + if (n > UPT1_RSS_MAX_IND_TABLE_SIZE) + return 0; while (n--) p[n] = rssConf->indTable[n]; return 0; -- 2.17.1
[BUG] net: chelsio: Possible buffer overflow caused by DMA failures/attacks
In alloc_rx_resources(): sge->respQ.entries = pci_alloc_consistent(pdev, size, &sge->respQ.dma_addr); Thus, "sge->respQ.entries" is a DMA value, and it is assigned to "e" in process_pure_responses(): struct sge *sge = adapter->sge; struct respQ *q = &sge->respQ; struct respQ_e *e = &q->entries[q->cidx]; When DMA failures or attacks occur, the data stored in "e" can be changed at any time. In this case, the value of "e->FreelistQid" can be a large number to cause buffer overflow when the following code is executed: const struct freelQ *fl = &sge->freelQ[e->FreelistQid]; Similarly, "sge->respQ.entries" is also assigned to "e" in process_responses(): struct sge *sge = adapter->sge; struct respQ *q = &sge->respQ; struct respQ_e *e = &q->entries[q->cidx]; When DMA failures or attacks occur, the data stored in "e" can be changed at any time. In this case, the value of "e->FreelistQid" can be a large number to cause buffer overflow when the following code is executed: struct freelQ *fl = &sge->freelQ[e->FreelistQid]; Considering that DMA can fail or be attacked, I think that it is dangerous to use a DMA value (or any value tainted by it) as an array index or a control-flow condition. However, I have found many such dangerous cases in Linux device drivers through my static-analysis tool and code review. I am not sure whether my opinion is correct, so I want to listen to your points of view. Thanks in advance :) Best wishes, Jia-Ju Bai
Re: [BUG] rtlwifi: a crash in error handling code of rtl_pci_probe()
On 2019/5/15 9:08, Larry Finger wrote: On 5/14/19 8:07 AM, Jia-Ju Bai wrote: In rtl_pci_probe(), when request_irq() in rtl_pci_intr_mode_legacy() in rtl_pci_intr_mode_decide() fails, a crash occurs. The crash information is as follows: [ 108.271155] kasan: CONFIG_KASAN_INLINE enabled [ 108.271163] kasan: GPF could be caused by NULL-ptr deref or user memory access .. [ 108.271193] RIP: 0010:cfg80211_get_drvinfo+0xce/0x3b0 [cfg80211] .. [ 108.271235] Call Trace: [ 108.271245] ethtool_get_drvinfo+0x110/0x640 [ 108.271255] ? cfg80211_get_chan_state+0x7e0/0x7e0 [cfg80211] [ 108.271261] ? ethtool_get_settings+0x340/0x340 [ 108.271268] ? __read_once_size_nocheck.constprop.7+0x20/0x20 [ 108.271279] ? kasan_check_write+0x14/0x20 [ 108.271284] dev_ethtool+0x272d/0x4c20 [ 108.271290] ? unwind_get_return_address+0x66/0xb0 [ 108.271299] ? __save_stack_trace+0x92/0x100 [ 108.271307] ? ethtool_get_rxnfc+0x3f0/0x3f0 [ 108.271316] ? save_stack+0xa3/0xd0 [ 108.271323] ? save_stack+0x43/0xd0 [ 108.271331] ? ftrace_graph_ret_addr+0x2d/0x170 [ 108.271338] ? ftrace_graph_ret_addr+0x2d/0x170 [ 108.271346] ? ftrace_graph_ret_addr+0x2d/0x170 [ 108.271354] ? update_stack_state+0x3b2/0x670 [ 108.271361] ? update_stack_state+0x3b2/0x670 [ 108.271370] ? __read_once_size_nocheck.constprop.7+0x20/0x20 [ 108.271379] ? unwind_next_frame.part.5+0x19f/0xa60 [ 108.271388] ? bpf_prog_kallsyms_find+0x3e/0x270 [ 108.271396] ? is_bpf_text_address+0x1a/0x30 [ 108.271408] ? kernel_text_address+0x11d/0x130 [ 108.271416] ? __kernel_text_address+0x12/0x40 [ 108.271423] ? unwind_get_return_address+0x66/0xb0 [ 108.271431] ? __save_stack_trace+0x92/0x100 [ 108.271440] ? save_stack+0xa3/0xd0 [ 108.271448] ? udp_ioctl+0x35/0xe0 [ 108.271457] ? inet_ioctl+0x100/0x320 [ 108.271466] ? inet_stream_connect+0xb0/0xb0 [ 108.271475] ? alloc_file+0x60/0x480 [ 108.271483] ? alloc_file_pseudo+0x19d/0x270 [ 108.271495] ? sock_alloc_file+0x51/0x170 [ 108.271502] ? __sys_socket+0x12c/0x1f0 [ 108.271510] ? __x64_sys_socket+0x78/0xb0 [ 108.271520] ? do_syscall_64+0xb1/0x2e0 [ 108.271529] ? entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 108.271538] ? kasan_check_read+0x11/0x20 [ 108.271548] ? mutex_lock+0x8f/0xe0 [ 108.271557] ? __mutex_lock_slowpath+0x20/0x20 [ 108.271568] dev_ioctl+0x1fb/0xae0 [ 108.271576] ? dev_ioctl+0x1fb/0xae0 [ 108.271586] ? _copy_from_user+0x71/0xd0 [ 108.271594] sock_do_ioctl+0x1e2/0x2f0 [ 108.271602] ? kmem_cache_alloc+0xf9/0x250 [ 108.271611] ? ___sys_recvmsg+0x5a0/0x5a0 [ 108.271621] ? apparmor_file_alloc_security+0x128/0x7e0 [ 108.271630] ? kasan_unpoison_shadow+0x35/0x50 [ 108.271638] ? kasan_kmalloc+0xad/0xe0 [ 108.271652] ? apparmor_file_alloc_security+0x128/0x7e0 [ 108.271662] ? apparmor_file_alloc_security+0x269/0x7e0 [ 108.271670] sock_ioctl+0x361/0x590 [ 108.271678] ? sock_ioctl+0x361/0x590 [ 108.271686] ? routing_ioctl+0x470/0x470 [ 108.271695] ? kasan_check_write+0x14/0x20 [ 108.271703] ? __mutex_init+0xba/0x130 [ 108.271713] ? percpu_counter_add_batch+0xc7/0x120 [ 108.271722] ? alloc_empty_file+0xae/0x150 [ 108.271729] ? routing_ioctl+0x470/0x470 [ 108.271738] do_vfs_ioctl+0x1ae/0xfe0 [ 108.271745] ? do_vfs_ioctl+0x1ae/0xfe0 [ 108.271754] ? alloc_file_pseudo+0x1ad/0x270 [ 108.271762] ? ioctl_preallocate+0x1e0/0x1e0 [ 108.271770] ? alloc_file+0x480/0x480 [ 108.271778] ? kasan_check_read+0x11/0x20 [ 108.271786] ? __fget+0x24d/0x320 [ 108.271794] ? iterate_fd+0x180/0x180 [ 108.271802] ? fd_install+0x52/0x60 [ 108.271812] ? security_file_ioctl+0x8c/0xb0 [ 108.271820] ksys_ioctl+0x99/0xb0 [ 108.271829] __x64_sys_ioctl+0x78/0xb0 [ 108.271839] do_syscall_64+0xb1/0x2e0 [ 108.271857] ? prepare_exit_to_usermode+0xc8/0x160 [ 108.271871] entry_SYSCALL_64_after_hwframe+0x44/0xa9 .. I checked the driver source code, but cannot find the reason, so I only report the crash... Can somebody give an explanation about this crash? This crash is triggered by a runtime fuzzing tool named FIZZER written by us. Your backtrace does not include any references to rtlwifi routines, and I have no idea what FIZZER does, thus it is not possible for me to debug this. If the error situation that you state happens, the code should end up at label "fail3" in routine rtl_pci_probe(). Insert printk statements after every line of the following, and report the last good point before the error. It is certainly possible that something is being torn down that was never erected. The likelihood of failure of both MSI and legacy interrupts is not very likely, and we probably have never hit those conditions. fail3: pci_set_drvdata(pdev, NULL); rtl_deinit_core(hw); fail2: if (rtlpriv->io.pci_mem_start != 0) pci_iounmap(pdev, (void __iomem *)rtlpriv->io.pci_mem_start); pci_release_regions(pdev); complete(&rtlpriv->fi
[BUG] rtlwifi: a crash in error handling code of rtl_pci_probe()
In rtl_pci_probe(), when request_irq() in rtl_pci_intr_mode_legacy() in rtl_pci_intr_mode_decide() fails, a crash occurs. The crash information is as follows: [ 108.271155] kasan: CONFIG_KASAN_INLINE enabled [ 108.271163] kasan: GPF could be caused by NULL-ptr deref or user memory access .. [ 108.271193] RIP: 0010:cfg80211_get_drvinfo+0xce/0x3b0 [cfg80211] .. [ 108.271235] Call Trace: [ 108.271245] ethtool_get_drvinfo+0x110/0x640 [ 108.271255] ? cfg80211_get_chan_state+0x7e0/0x7e0 [cfg80211] [ 108.271261] ? ethtool_get_settings+0x340/0x340 [ 108.271268] ? __read_once_size_nocheck.constprop.7+0x20/0x20 [ 108.271279] ? kasan_check_write+0x14/0x20 [ 108.271284] dev_ethtool+0x272d/0x4c20 [ 108.271290] ? unwind_get_return_address+0x66/0xb0 [ 108.271299] ? __save_stack_trace+0x92/0x100 [ 108.271307] ? ethtool_get_rxnfc+0x3f0/0x3f0 [ 108.271316] ? save_stack+0xa3/0xd0 [ 108.271323] ? save_stack+0x43/0xd0 [ 108.271331] ? ftrace_graph_ret_addr+0x2d/0x170 [ 108.271338] ? ftrace_graph_ret_addr+0x2d/0x170 [ 108.271346] ? ftrace_graph_ret_addr+0x2d/0x170 [ 108.271354] ? update_stack_state+0x3b2/0x670 [ 108.271361] ? update_stack_state+0x3b2/0x670 [ 108.271370] ? __read_once_size_nocheck.constprop.7+0x20/0x20 [ 108.271379] ? unwind_next_frame.part.5+0x19f/0xa60 [ 108.271388] ? bpf_prog_kallsyms_find+0x3e/0x270 [ 108.271396] ? is_bpf_text_address+0x1a/0x30 [ 108.271408] ? kernel_text_address+0x11d/0x130 [ 108.271416] ? __kernel_text_address+0x12/0x40 [ 108.271423] ? unwind_get_return_address+0x66/0xb0 [ 108.271431] ? __save_stack_trace+0x92/0x100 [ 108.271440] ? save_stack+0xa3/0xd0 [ 108.271448] ? udp_ioctl+0x35/0xe0 [ 108.271457] ? inet_ioctl+0x100/0x320 [ 108.271466] ? inet_stream_connect+0xb0/0xb0 [ 108.271475] ? alloc_file+0x60/0x480 [ 108.271483] ? alloc_file_pseudo+0x19d/0x270 [ 108.271495] ? sock_alloc_file+0x51/0x170 [ 108.271502] ? __sys_socket+0x12c/0x1f0 [ 108.271510] ? __x64_sys_socket+0x78/0xb0 [ 108.271520] ? do_syscall_64+0xb1/0x2e0 [ 108.271529] ? entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 108.271538] ? kasan_check_read+0x11/0x20 [ 108.271548] ? mutex_lock+0x8f/0xe0 [ 108.271557] ? __mutex_lock_slowpath+0x20/0x20 [ 108.271568] dev_ioctl+0x1fb/0xae0 [ 108.271576] ? dev_ioctl+0x1fb/0xae0 [ 108.271586] ? _copy_from_user+0x71/0xd0 [ 108.271594] sock_do_ioctl+0x1e2/0x2f0 [ 108.271602] ? kmem_cache_alloc+0xf9/0x250 [ 108.271611] ? ___sys_recvmsg+0x5a0/0x5a0 [ 108.271621] ? apparmor_file_alloc_security+0x128/0x7e0 [ 108.271630] ? kasan_unpoison_shadow+0x35/0x50 [ 108.271638] ? kasan_kmalloc+0xad/0xe0 [ 108.271652] ? apparmor_file_alloc_security+0x128/0x7e0 [ 108.271662] ? apparmor_file_alloc_security+0x269/0x7e0 [ 108.271670] sock_ioctl+0x361/0x590 [ 108.271678] ? sock_ioctl+0x361/0x590 [ 108.271686] ? routing_ioctl+0x470/0x470 [ 108.271695] ? kasan_check_write+0x14/0x20 [ 108.271703] ? __mutex_init+0xba/0x130 [ 108.271713] ? percpu_counter_add_batch+0xc7/0x120 [ 108.271722] ? alloc_empty_file+0xae/0x150 [ 108.271729] ? routing_ioctl+0x470/0x470 [ 108.271738] do_vfs_ioctl+0x1ae/0xfe0 [ 108.271745] ? do_vfs_ioctl+0x1ae/0xfe0 [ 108.271754] ? alloc_file_pseudo+0x1ad/0x270 [ 108.271762] ? ioctl_preallocate+0x1e0/0x1e0 [ 108.271770] ? alloc_file+0x480/0x480 [ 108.271778] ? kasan_check_read+0x11/0x20 [ 108.271786] ? __fget+0x24d/0x320 [ 108.271794] ? iterate_fd+0x180/0x180 [ 108.271802] ? fd_install+0x52/0x60 [ 108.271812] ? security_file_ioctl+0x8c/0xb0 [ 108.271820] ksys_ioctl+0x99/0xb0 [ 108.271829] __x64_sys_ioctl+0x78/0xb0 [ 108.271839] do_syscall_64+0xb1/0x2e0 [ 108.271857] ? prepare_exit_to_usermode+0xc8/0x160 [ 108.271871] entry_SYSCALL_64_after_hwframe+0x44/0xa9 .. I checked the driver source code, but cannot find the reason, so I only report the crash... Can somebody give an explanation about this crash? This crash is triggered by a runtime fuzzing tool named FIZZER written by us. Best wishes, Jia-Ju Bai
[BUG] rtlwifi: Resource leaks in error handling code of rtl_pci_probe()
In rtl_pci_probe(), rtl_pci_init() allocates some resources, such as: _rtl_pci_init_trx_ring _rtl_pci_init_rx_ring _rtl_pci_init_rx_ring pci_zalloc_consistent() -- resource _rtl_pci_init_one_rxdesc dev_alloc_skb() -- resource _rtl_pci_init_trx_ring _rtl_pci_init_tx_ring pci_zalloc_consistent() -- resource When ieee80211_register_hw() or rtl_pci_intr_mode_decide() fails, these resources are not released in error handling code. A possible fix is to call rtl_pci_deinit() in error handling code, but I am not sure whether this is correct. Thus, I only report the bugs. These bugs are found by a runtime fuzzing tool named FIZZER written by us. Best wishes, Jia-Ju Bai
[PATCH] rtlwifi: Fix null-pointer dereferences in error handling code of rtl_pci_probe()
*BUG 1:
In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails,
rtl_deinit_core() in the error handling code is executed.
rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses
rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been
initialized. Thus a null-pointer dereference occurs.
The reason is that rtlpriv->scan_list.list is initialized by
INIT_LIST_HEAD() in rtl_init_core(), which has not been called.
To fix this bug, rtl_deinit_core() should not be called when
rtlpriv->cfg->ops->init_sw_vars() fails.
*BUG 2:
In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in
this function fails, and rtlpriv->scan_list.list has not been
initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error
handling code of rtl_pci_probe() is executed. Finally, a null-pointer
dereference occurs due to the same reason of the above bug.
To fix this bug, the initialization of lists in rtl_init_core() are
performed before the call to rtl_regd_init().
These bugs are found by a runtime fuzzing tool named FIZZER written by
us.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/realtek/rtlwifi/base.c | 15 ---
drivers/net/wireless/realtek/rtlwifi/pci.c | 4 ++--
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c
b/drivers/net/wireless/realtek/rtlwifi/base.c
index 217d2a7a43c7..b3f341ec3710 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.c
+++ b/drivers/net/wireless/realtek/rtlwifi/base.c
@@ -526,8 +526,14 @@ int rtl_init_core(struct ieee80211_hw *hw)
/* <2> rate control register */
hw->rate_control_algorithm = "rtl_rc";
+ /* <3> init list */
+ INIT_LIST_HEAD(&rtlpriv->entry_list);
+ INIT_LIST_HEAD(&rtlpriv->scan_list.list);
+ skb_queue_head_init(&rtlpriv->tx_report.queue);
+ skb_queue_head_init(&rtlpriv->c2hcmd_queue);
+
/*
-* <3> init CRDA must come after init
+* <4> init CRDA must come after init
* mac80211 hw in _rtl_init_mac80211.
*/
if (rtl_regd_init(hw, rtl_reg_notifier)) {
@@ -535,7 +541,7 @@ int rtl_init_core(struct ieee80211_hw *hw)
return 1;
}
- /* <4> locks */
+ /* <5> locks */
mutex_init(&rtlpriv->locks.conf_mutex);
mutex_init(&rtlpriv->locks.ips_mutex);
mutex_init(&rtlpriv->locks.lps_mutex);
@@ -550,11 +556,6 @@ int rtl_init_core(struct ieee80211_hw *hw)
spin_lock_init(&rtlpriv->locks.cck_and_rw_pagea_lock);
spin_lock_init(&rtlpriv->locks.fw_ps_lock);
spin_lock_init(&rtlpriv->locks.iqk_lock);
- /* <5> init list */
- INIT_LIST_HEAD(&rtlpriv->entry_list);
- INIT_LIST_HEAD(&rtlpriv->scan_list.list);
- skb_queue_head_init(&rtlpriv->tx_report.queue);
- skb_queue_head_init(&rtlpriv->c2hcmd_queue);
rtlmac->link_state = MAC80211_NOLINK;
diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c
b/drivers/net/wireless/realtek/rtlwifi/pci.c
index 48ca52102cef..864cb76230c4 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -2267,7 +2267,7 @@ int rtl_pci_probe(struct pci_dev *pdev,
if (rtlpriv->cfg->ops->init_sw_vars(hw)) {
pr_err("Can't init_sw_vars\n");
err = -ENODEV;
- goto fail3;
+ goto fail2;
}
rtlpriv->cfg->ops->init_sw_leds(hw);
--
2.17.0
[BUG] net: huawei: hinic: a possible sleep-in-atomic-context bug in msg_to_mgmt_async
The driver may sleep in an interrupt handler. The function call path (from bottom to top) in the directory "drivers/net/ethernet/huawei/hinic/" in Linux-4.17 is: [FUNC] down hinic_hw_mgmt.c, 324: down in msg_to_mgmt_async hinic_hw_mgmt.c, 408: msg_to_mgmt_async in mgmt_recv_msg_handler hinic_hw_mgmt.c, 464:mgmt_recv_msg_handler in recv_mgmt_msg_handler hinic_hw_mgmt.c, 484: recv_mgmt_msg_handler in mgmt_msg_aeqe_handler hinic_hw_eqs.c, 264: [FUNC_PTR]mgmt_msg_aeqe_handler in aeq_irq_handler hinic_hw_eqs.c, 355: aeq_irq_handler in eq_irq_handler hinic_hw_eqs.c, 383: eq_irq_handler in ceq_tasklet Note that [FUNC_PTR] means a function pointer call. This bug is found by my static analysis tool (DSAC-2) and checked by my manual code review. I do not know how to correctly fix this bug, so I just report it. A possible way may be to replace up() and down() with spin_lock() and spin_unlock(). Best wishes, Jia-Ju Bai
[BUG] net: huawei: hinic: a possible sleep-in-atomic-context bug in hinic_get_stats64
The driver may sleep while holding a RCU lock. The function call path (from bottom to top) in Linux-4.17 is: [FUNC] down drivers/net/.../hinic/hinic_main.c, 775: down in hinic_get_stats64 net/core/dev.c, 8278: [FUNC_PTR]hinic_get_stats64 in dev_get_stats net/core/net-sysfs.c, 568: dev_get_stats in netstat_show net/core/net-sysfs.c, 565: _raw_read_lock in netstat_show Note that [FUNC_PTR] means a function pointer call. This bug is found by my static analysis tool (DSAC-2) and checked by my manual code review. I do not know how to correctly fix this bug, so I just report it. A possible way may be to replace up() and down() with spin_lock() and spin_unlock(). Best wishes, Jia-Ju Bai
[BUG] net: huawei: hinic: a possible sleep-in-atomic-context bug in hinic_get_stats64
The driver may sleep while holding a RCU lock. The function call path (from bottom to top) in Linux-4.17 is: [FUNC] down drivers/net/.../hinic/hinic_main.c, 775: down in hinic_get_stats64 net/core/dev.c, 8278: [FUNC_PTR]hinic_get_stats64 in dev_get_stats net/core/net-sysfs.c, 568: dev_get_stats in netstat_show net/core/net-sysfs.c, 565: _raw_read_lock in netstat_show Note that [FUNC_PTR] means a function pointer call. This bug is found by my static analysis tool (DSAC-2) and checked by my manual code review. I do not know how to correctly fix this bug, so I just report it. A possible way may be to replace up() and down() with spin_lock() and spin_unlock(). Best wishes, Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/9 11:24, Yanjun Zhu wrote: If you have forcedeth NIC, you can make tests with it.:-) Ah, I would like to, but I do not have the hardware... Best wishes, Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/9 10:35, Yanjun Zhu wrote:
On 2019/1/9 10:03, Jia-Ju Bai wrote:
On 2019/1/9 9:24, Yanjun Zhu wrote:
On 2019/1/8 20:57, Jia-Ju Bai wrote:
On 2019/1/8 20:54, Zhu Yanjun wrote:
在 2019/1/8 20:45, Jia-Ju Bai 写道:
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs,
Does this really occur? Can you reproduce this ?
This bug is not found by the real execution.
It is found by a static tool written by myself, and then I check it
by manual code review.
Before "line 2004: dev_kfree_skb_any(tx_skb->skb); ",
"
nv_disable_irq(dev);
nv_napi_disable(dev);
netif_tx_lock_bh(dev);
netif_addr_lock(dev);
spin_lock(&np->lock);
/* stop engines */
nv_stop_rxtx(dev); <---this stop rxtx
nv_txrx_reset(dev);
"
In this case, does nv_start_xmit or nv_start_xmit_optimized still
work well?
nv_stop_rxtx() calls nv_stop_tx(dev).
static void nv_stop_tx(struct net_device *dev)
{
struct fe_priv *np = netdev_priv(dev);
u8 __iomem *base = get_hwbase(dev);
u32 tx_ctrl = readl(base + NvRegTransmitterControl);
if (!np->mac_in_use)
tx_ctrl &= ~NVREG_XMITCTL_START;
else
tx_ctrl |= NVREG_XMITCTL_TX_PATH_EN;
writel(tx_ctrl, base + NvRegTransmitterControl);
if (reg_delay(dev, NvRegTransmitterStatus, NVREG_XMITSTAT_BUSY, 0,
NV_TXSTOP_DELAY1, NV_TXSTOP_DELAY1MAX))
netdev_info(dev, "%s: TransmitterStatus remained busy\n",
__func__);
udelay(NV_TXSTOP_DELAY2);
if (!np->mac_in_use)
writel(readl(base + NvRegTransmitPoll) &
NVREG_TRANSMITPOLL_MAC_ADDR_REV,
base + NvRegTransmitPoll);
}
nv_stop_tx() seems to only write registers to stop transmitting for
hardware.
But it does not wait until nv_start_xmit() and
nv_start_xmit_optimized() finish execution.
There are 3 modes in forcedeth NIC.
In throughput mode (0), every tx & rx packet will generate an interrupt.
In CPU mode (1), interrupts are controlled by a timer.
In dynamic mode (2), the mode toggles between throughput and CPU mode
based on network load.
From the source code,
"np->recover_error = 1;" is related with CPU mode.
nv_start_xmit or nv_start_xmit_optimized seems related with ghroughput
mode.
In static void nv_do_nic_poll(struct timer_list *t),
when if (np->recover_error), line 2004:
dev_kfree_skb_any(tx_skb->skb); will run.
When "np->recover_error=1", do you think nv_start_xmit or
nv_start_xmit_optimized will be called?
Sorry, I do not know about these modes...
But I still think nv_start_xmit() or nv_start_xmit_optimized() can be
called here, in no matter which mode :)
Best wishes,
Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/9 9:24, Yanjun Zhu wrote:
On 2019/1/8 20:57, Jia-Ju Bai wrote:
On 2019/1/8 20:54, Zhu Yanjun wrote:
在 2019/1/8 20:45, Jia-Ju Bai 写道:
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs,
Does this really occur? Can you reproduce this ?
This bug is not found by the real execution.
It is found by a static tool written by myself, and then I check it
by manual code review.
Before "line 2004: dev_kfree_skb_any(tx_skb->skb); ",
"
nv_disable_irq(dev);
nv_napi_disable(dev);
netif_tx_lock_bh(dev);
netif_addr_lock(dev);
spin_lock(&np->lock);
/* stop engines */
nv_stop_rxtx(dev); <---this stop rxtx
nv_txrx_reset(dev);
"
In this case, does nv_start_xmit or nv_start_xmit_optimized still work
well?
nv_stop_rxtx() calls nv_stop_tx(dev).
static void nv_stop_tx(struct net_device *dev)
{
struct fe_priv *np = netdev_priv(dev);
u8 __iomem *base = get_hwbase(dev);
u32 tx_ctrl = readl(base + NvRegTransmitterControl);
if (!np->mac_in_use)
tx_ctrl &= ~NVREG_XMITCTL_START;
else
tx_ctrl |= NVREG_XMITCTL_TX_PATH_EN;
writel(tx_ctrl, base + NvRegTransmitterControl);
if (reg_delay(dev, NvRegTransmitterStatus, NVREG_XMITSTAT_BUSY, 0,
NV_TXSTOP_DELAY1, NV_TXSTOP_DELAY1MAX))
netdev_info(dev, "%s: TransmitterStatus remained busy\n",
__func__);
udelay(NV_TXSTOP_DELAY2);
if (!np->mac_in_use)
writel(readl(base + NvRegTransmitPoll) &
NVREG_TRANSMITPOLL_MAC_ADDR_REV,
base + NvRegTransmitPoll);
}
nv_stop_tx() seems to only write registers to stop transmitting for
hardware.
But it does not wait until nv_start_xmit() and nv_start_xmit_optimized()
finish execution.
Maybe netif_stop_queue() should be used here to stop transmitting for
network layer, but this function does not seem to wait, either.
Do you know any function that can wait until ".ndo_start_xmit" finish
execution?
Best wishes,
Jia-Ju Bai
[PATCH] isdn: i4l: isdn_tty: Fix some concurrency double-free bugs
The functions isdn_tty_tiocmset() and isdn_tty_set_termios() may be
concurrently executed.
isdn_tty_tiocmset
isdn_tty_modem_hup
line 719: kfree(info->dtmf_state);
line 721: kfree(info->silence_state);
line 723: kfree(info->adpcms);
line 725: kfree(info->adpcmr);
isdn_tty_set_termios
isdn_tty_modem_hup
line 719: kfree(info->dtmf_state);
line 721: kfree(info->silence_state);
line 723: kfree(info->adpcms);
line 725: kfree(info->adpcmr);
Thus, some concurrency double-free bugs may occur.
These possible bugs are found by a static tool written by myself and
my manual code review.
To fix these possible bugs, the mutex lock "modem_info_mutex" used in
isdn_tty_tiocmset() is added in isdn_tty_set_termios().
Signed-off-by: Jia-Ju Bai
---
drivers/isdn/i4l/isdn_tty.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index 1b2239c1d569..dc1cded716c1 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -1437,15 +1437,19 @@ isdn_tty_set_termios(struct tty_struct *tty, struct
ktermios *old_termios)
{
modem_info *info = (modem_info *) tty->driver_data;
+ mutex_lock(&modem_info_mutex);
if (!old_termios)
isdn_tty_change_speed(info);
else {
if (tty->termios.c_cflag == old_termios->c_cflag &&
tty->termios.c_ispeed == old_termios->c_ispeed &&
- tty->termios.c_ospeed == old_termios->c_ospeed)
+ tty->termios.c_ospeed == old_termios->c_ospeed) {
+ mutex_unlock(&modem_info_mutex);
return;
+ }
isdn_tty_change_speed(info);
}
+ mutex_unlock(&modem_info_mutex);
}
/*
--
2.17.0
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/8 20:54, Zhu Yanjun wrote: 在 2019/1/8 20:45, Jia-Ju Bai 写道: In drivers/net/ethernet/nvidia/forcedeth.c, the functions nv_start_xmit() and nv_start_xmit_optimized() can be concurrently executed with nv_poll_controller(). nv_start_xmit line 2321: prev_tx_ctx->skb = skb; nv_start_xmit_optimized line 2479: prev_tx_ctx->skb = skb; nv_poll_controller nv_do_nic_poll line 4134: spin_lock(&np->lock); nv_drain_rxtx nv_drain_tx nv_release_txskb line 2004: dev_kfree_skb_any(tx_skb->skb); Thus, two possible concurrency use-after-free bugs may occur. To fix these possible bugs, Does this really occur? Can you reproduce this ? This bug is not found by the real execution. It is found by a static tool written by myself, and then I check it by manual code review. Best wishes, Jia-Ju Bai
[PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs, the calls to spin_lock_irqsave() in
nv_start_xmit() and nv_start_xmit_optimized() are moved to the
front of "prev_tx_ctx->skb = skb;"
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/nvidia/forcedeth.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c
b/drivers/net/ethernet/nvidia/forcedeth.c
index 1d9b0d44ddb6..48fa5a0bd2cb 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -2317,6 +2317,8 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb,
struct net_device *dev)
/* set last fragment flag */
prev_tx->flaglen |= cpu_to_le32(tx_flags_extra);
+ spin_lock_irqsave(&np->lock, flags);
+
/* save skb in this slot's context area */
prev_tx_ctx->skb = skb;
@@ -2326,8 +2328,6 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb,
struct net_device *dev)
tx_flags_extra = skb->ip_summed == CHECKSUM_PARTIAL ?
NV_TX2_CHECKSUM_L3 | NV_TX2_CHECKSUM_L4 : 0;
- spin_lock_irqsave(&np->lock, flags);
-
/* set tx flags */
start_tx->flaglen |= cpu_to_le32(tx_flags | tx_flags_extra);
@@ -2475,6 +2475,8 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff
*skb,
/* set last fragment flag */
prev_tx->flaglen |= cpu_to_le32(NV_TX2_LASTPACKET);
+ spin_lock_irqsave(&np->lock, flags);
+
/* save skb in this slot's context area */
prev_tx_ctx->skb = skb;
@@ -2491,8 +2493,6 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff
*skb,
else
start_tx->txvlan = 0;
- spin_lock_irqsave(&np->lock, flags);
-
if (np->tx_limit) {
/* Limit the number of outstanding tx. Setup all fragments, but
* do not set the VALID bit on the first descriptor. Save a
pointer
--
2.17.0
[PATCH v2] net: arcnet: Fix a possible concurrency use-after-free bug in arcnet_reply_tasklet()
In drivers/net/arcnet/arcnet.c, the functions arcnet_reply_tasklet() and arcnet_send_packet() may be concurrently executed. arcnet_reply_tasklet() line 430: dev_kfree_skb(lp->outgoing.skb); arcnet_send_packet() line 682: spin_lock_irqsave(); line 690: lp->outgoing.skb = skb; line 691: proto->prepare_tx(..., skb->len, ...) Thus, a possible concurrency use-after-free bugs may occur. To fix this bug, the calls to spin_lock_irqsave() and spin_unlock_irqrestore() are added in arcnet_reply_tasklet() to protect dev_kfree_skb(lp->outgoing.skb). Signed-off-by: Jia-Ju Bai --- v2: * Add the definition of flags to fix the compilation error. --- drivers/net/arcnet/arcnet.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/net/arcnet/arcnet.c b/drivers/net/arcnet/arcnet.c index 8459115d9d4e..1e1b12650964 100644 --- a/drivers/net/arcnet/arcnet.c +++ b/drivers/net/arcnet/arcnet.c @@ -401,6 +401,7 @@ static void arcnet_reply_tasklet(unsigned long data) struct sock_exterr_skb *serr; struct sock *sk; int ret; + unsigned long flags; local_irq_disable(); skb = lp->outgoing.skb; @@ -426,10 +427,14 @@ static void arcnet_reply_tasklet(unsigned long data) serr->ee.ee_data = skb_shinfo(skb)->tskey; serr->ee.ee_info = lp->reply_status; + spin_lock_irqsave(&lp->lock, flags); + /* finally erasing outgoing skb */ dev_kfree_skb(lp->outgoing.skb); lp->outgoing.skb = NULL; + spin_unlock_irqrestore(&lp->lock, flags); + ackskb->dev = lp->dev; ret = sock_queue_err_skb(sk, ackskb); -- 2.17.0
[BUG] net: brocade: bna: Possible concurrency use-after-free bugs
In drivers/net/ethernet/brocade/bna/bnad_debugfs.c, the functions bnad_debugfs_read_regrd() and bnad_debugfs_write_regrd() may be concurrently executed. bnad_debugfs_read_regrd() line 293: if (!bnad->regdata) line 297: simple_read_from_buffer(..., bnad->regdata, ...) line 300: kfree(bnad->regdata) bnad_debugfs_write_regrd() line 335: kfree(bnad->regdata) line 338: kfree(bnad->regdata) line 357: regbuf = (u32 *)bnad->regdata All these accesses to bnad->regdata are not protected by any lock. Thus, possible concurrency use-after-free bugs may occur. A possible fixing way is to use a lock to protect these accesses. I am not sure about this way, so I only report the bugs. Best wishes, Jia-Ju Bai
[PATCH] net: arcnet: Fix a possible concurrency use-after-free bug in arcnet_reply_tasklet()
In drivers/net/arcnet/arcnet.c, the functions arcnet_reply_tasklet() and arcnet_send_packet() may be concurrently executed. arcnet_reply_tasklet() line 430: dev_kfree_skb(lp->outgoing.skb); arcnet_send_packet() line 682: spin_lock_irqsave(); line 690: lp->outgoing.skb = skb; line 691: proto->prepare_tx(..., skb->len, ...) Thus, a possible concurrency use-after-free bugs may occur. To fix this bug, the calls to spin_lock_irqsave() and spin_unlock_irqrestore() are added in arcnet_reply_tasklet() to protect dev_kfree_skb(lp->outgoing.skb). Signed-off-by: Jia-Ju Bai --- drivers/net/arcnet/arcnet.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/net/arcnet/arcnet.c b/drivers/net/arcnet/arcnet.c index 8459115d9d4e..c5e943d01d66 100644 --- a/drivers/net/arcnet/arcnet.c +++ b/drivers/net/arcnet/arcnet.c @@ -426,10 +426,14 @@ static void arcnet_reply_tasklet(unsigned long data) serr->ee.ee_data = skb_shinfo(skb)->tskey; serr->ee.ee_info = lp->reply_status; + spin_lock_irqsave(&lp->lock, flags); + /* finally erasing outgoing skb */ dev_kfree_skb(lp->outgoing.skb); lp->outgoing.skb = NULL; + spin_unlock_irqrestore(&lp->lock, flags); + ackskb->dev = lp->dev; ret = sock_queue_err_skb(sk, ackskb); -- 2.17.0
[PATCH] isdn: hisax: hfc_pci: Fix a possible concurrency use-after-free bug in HFCPCI_l1hw()
In drivers/isdn/hisax/hfc_pci.c, the functions hfcpci_interrupt() and
HFCPCI_l1hw() may be concurrently executed.
HFCPCI_l1hw()
line 1173: if (!cs->tx_skb)
hfcpci_interrupt()
line 942: spin_lock_irqsave();
line 1066: dev_kfree_skb_irq(cs->tx_skb);
Thus, a possible concurrency use-after-free bug may occur
in HFCPCI_l1hw().
To fix these bugs, the calls to spin_lock_irqsave() and
spin_unlock_irqrestore() are added in HFCPCI_l1hw(), to protect the
access to cs->tx_skb.
Signed-off-by: Jia-Ju Bai
---
drivers/isdn/hisax/hfc_pci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
index ea0e4c6de3fb..0109e0e8bcb6 100644
--- a/drivers/isdn/hisax/hfc_pci.c
+++ b/drivers/isdn/hisax/hfc_pci.c
@@ -1170,11 +1170,13 @@ HFCPCI_l1hw(struct PStack *st, int pr, void *arg)
if (cs->debug & L1_DEB_LAPD)
debugl1(cs, "-> PH_REQUEST_PULL");
#endif
+ spin_lock_irqsave(&cs->lock, flags);
if (!cs->tx_skb) {
test_and_clear_bit(FLG_L1_PULL_REQ, &st->l1.Flags);
st->l1.l1l2(st, PH_PULL | CONFIRM, NULL);
} else
test_and_set_bit(FLG_L1_PULL_REQ, &st->l1.Flags);
+ spin_unlock_irqrestore(&cs->lock, flags);
break;
case (HW_RESET | REQUEST):
spin_lock_irqsave(&cs->lock, flags);
--
2.17.0
[PATCH] cw1200: Fix concurrency use-after-free bugs in cw1200_hw_scan()
The function cw1200_bss_info_changed() and cw1200_hw_scan() can be
concurrently executed.
The two functions both access a possible shared variable "frame.skb".
This shared variable is freed by dev_kfree_skb() in cw1200_upload_beacon(),
which is called by cw1200_bss_info_changed(). The free operation is
protected by a mutex lock "priv->conf_mutex" in cw1200_bss_info_changed().
In cw1200_hw_scan(), this shared variable is accessed without the
protection of the mutex lock "priv->conf_mutex".
Thus, concurrency use-after-free bugs may occur.
To fix these bugs, the original calls to mutex_lock(&priv->conf_mutex) and
mutex_unlock(&priv->conf_mutex) are moved to the places, which can
protect the accesses to the shared variable.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/st/cw1200/scan.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/st/cw1200/scan.c
b/drivers/net/wireless/st/cw1200/scan.c
index 67213f11acbd..0a9eac93dd01 100644
--- a/drivers/net/wireless/st/cw1200/scan.c
+++ b/drivers/net/wireless/st/cw1200/scan.c
@@ -78,6 +78,10 @@ int cw1200_hw_scan(struct ieee80211_hw *hw,
if (req->n_ssids > WSM_SCAN_MAX_NUM_OF_SSIDS)
return -EINVAL;
+ /* will be unlocked in cw1200_scan_work() */
+ down(&priv->scan.lock);
+ mutex_lock(&priv->conf_mutex);
+
frame.skb = ieee80211_probereq_get(hw, priv->vif->addr, NULL, 0,
req->ie_len);
if (!frame.skb)
@@ -86,19 +90,15 @@ int cw1200_hw_scan(struct ieee80211_hw *hw,
if (req->ie_len)
skb_put_data(frame.skb, req->ie, req->ie_len);
- /* will be unlocked in cw1200_scan_work() */
- down(&priv->scan.lock);
- mutex_lock(&priv->conf_mutex);
-
ret = wsm_set_template_frame(priv, &frame);
if (!ret) {
/* Host want to be the probe responder. */
ret = wsm_set_probe_responder(priv, true);
}
if (ret) {
+ dev_kfree_skb(frame.skb);
mutex_unlock(&priv->conf_mutex);
up(&priv->scan.lock);
- dev_kfree_skb(frame.skb);
return ret;
}
@@ -120,10 +120,9 @@ int cw1200_hw_scan(struct ieee80211_hw *hw,
++priv->scan.n_ssids;
}
- mutex_unlock(&priv->conf_mutex);
-
if (frame.skb)
dev_kfree_skb(frame.skb);
+ mutex_unlock(&priv->conf_mutex);
queue_work(priv->workqueue, &priv->scan.work);
return 0;
}
--
2.17.0
[PATCH] net: wireless: ath: ath9k: Fix a possible data race in ath_chanctx_set_next
The write operation to "sc->next_chan" is protected by
the lock on line 1287, but the read operation to
this data on line 1262 is not protected by the lock.
Thus, there may exist a data race for "sc->next_chan".
To fix this data race, the read operation to "sc->next_chan"
should be also protected by the lock.
Signed-off-by: Jia-Ju Bai
---
drivers/net/wireless/ath/ath9k/channel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/channel.c
b/drivers/net/wireless/ath/ath9k/channel.c
index 1b05b5d7a038..ed3cd5523481 100644
--- a/drivers/net/wireless/ath/ath9k/channel.c
+++ b/drivers/net/wireless/ath/ath9k/channel.c
@@ -1257,12 +1257,12 @@ void ath_chanctx_set_next(struct ath_softc *sc, bool
force)
"Stopping current chanctx: %d\n",
sc->cur_chan->chandef.center_freq1);
sc->cur_chan->stopped = true;
- spin_unlock_bh(&sc->chan_lock);
if (sc->next_chan == &sc->offchannel.chan) {
getrawmonotonic(&ts);
measure_time = true;
}
+ spin_unlock_bh(&sc->chan_lock);
ath9k_chanctx_stop_queues(sc, sc->cur_chan);
queues_stopped = true;
--
2.17.0
Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats
On 2018/5/8 13:04, Eric Dumazet wrote: On 05/07/2018 07:16 PM, Jia-Ju Bai wrote: Yes, "&dev->stats" will not change, because it is a fixed address. But the field data in "dev->stats" is changed (rx_frame_errors, rx_crc_errors and rx_missed_errors). So if the driver returns "&dev->stats" without lock protection (like on line 858), the field data value of this return value can be the changed field data value or unchanged field data value. We do not care. This function can be called by multiple cpus at the same time. As soon as one cpu returns from it, another cpu can happily modify dev->stats.ANYFIELD. Your patch fixes nothing at all. Okay, thanks. I also find that my patch does not work... Best wishes, Jia-Ju Bai
Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats
On 2018/5/8 9:56, Eric Dumazet wrote:
On 05/07/2018 05:51 PM, Jia-Ju Bai wrote:
On 2018/5/7 22:15, Eric Dumazet wrote:
On 05/07/2018 07:08 AM, Jia-Ju Bai wrote:
The write operations to "dev->stats" are protected by
the spinlock on line 862-864, but the read operations to
this data on line 858 and 867 are not protected by the spinlock.
Thus, there may exist data races for "dev->stats".
To fix the data races, the read operations to "dev->stats" are
protected by the spinlock, and a local variable is used for return.
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/8390/lib8390.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/8390/lib8390.c
b/drivers/net/ethernet/8390/lib8390.c
index c9c55c9eab9f..198952247d30 100644
--- a/drivers/net/ethernet/8390/lib8390.c
+++ b/drivers/net/ethernet/8390/lib8390.c
@@ -852,19 +852,25 @@ static struct net_device_stats *__ei_get_stats(struct
net_device *dev)
unsigned long ioaddr = dev->base_addr;
struct ei_device *ei_local = netdev_priv(dev);
unsigned long flags;
+struct net_device_stats *stats;
+
+spin_lock_irqsave(&ei_local->page_lock, flags);
/* If the card is stopped, just return the present stats. */
-if (!netif_running(dev))
-return &dev->stats;
+if (!netif_running(dev)) {
+stats = &dev->stats;
+spin_unlock_irqrestore(&ei_local->page_lock, flags);
+return stats;
+}
-spin_lock_irqsave(&ei_local->page_lock, flags);
/* Read the counter registers, assuming we are in page 0. */
dev->stats.rx_frame_errors += ei_inb_p(ioaddr + EN0_COUNTER0);
dev->stats.rx_crc_errors+= ei_inb_p(ioaddr + EN0_COUNTER1);
dev->stats.rx_missed_errors += ei_inb_p(ioaddr + EN0_COUNTER2);
+stats = &dev->stats;
spin_unlock_irqrestore(&ei_local->page_lock, flags);
-return &dev->stats;
+return stats;
}
/*
dev->stats is not a pointer, it is an array embedded in the
struct net_device
So this patch is not needed, since dev->stats can not change.
Thanks for your reply :)
I do not understand that why "dev->stats can not change".
Its data is indeed changed by the code:
dev->stats.rx_frame_errors += ei_inb_p(ioaddr + EN0_COUNTER0);
dev->stats.rx_crc_errors+= ei_inb_p(ioaddr + EN0_COUNTER1);
dev->stats.rx_missed_errors += ei_inb_p(ioaddr + EN0_COUNTER2);
So ?
So I think a data race may occur when returning "dev->stats" without lock
protection.
&dev->stats is a stable value.
It wont change over the lifetime of net_device object.
Adding a barrier before or after getting &dev->stats is useless, confusing and
really not needed.
Yes, "&dev->stats" will not change, because it is a fixed address.
But the field data in "dev->stats" is changed (rx_frame_errors,
rx_crc_errors and rx_missed_errors).
So if the driver returns "&dev->stats" without lock protection (like on
line 858), the field data value of this return value can be the changed
field data value or unchanged field data value.
Best wishes,
Jia-Ju Bai
Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats
On 2018/5/7 22:15, Eric Dumazet wrote:
On 05/07/2018 07:08 AM, Jia-Ju Bai wrote:
The write operations to "dev->stats" are protected by
the spinlock on line 862-864, but the read operations to
this data on line 858 and 867 are not protected by the spinlock.
Thus, there may exist data races for "dev->stats".
To fix the data races, the read operations to "dev->stats" are
protected by the spinlock, and a local variable is used for return.
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/8390/lib8390.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/8390/lib8390.c
b/drivers/net/ethernet/8390/lib8390.c
index c9c55c9eab9f..198952247d30 100644
--- a/drivers/net/ethernet/8390/lib8390.c
+++ b/drivers/net/ethernet/8390/lib8390.c
@@ -852,19 +852,25 @@ static struct net_device_stats *__ei_get_stats(struct
net_device *dev)
unsigned long ioaddr = dev->base_addr;
struct ei_device *ei_local = netdev_priv(dev);
unsigned long flags;
+ struct net_device_stats *stats;
+
+ spin_lock_irqsave(&ei_local->page_lock, flags);
/* If the card is stopped, just return the present stats. */
- if (!netif_running(dev))
- return &dev->stats;
+ if (!netif_running(dev)) {
+ stats = &dev->stats;
+ spin_unlock_irqrestore(&ei_local->page_lock, flags);
+ return stats;
+ }
- spin_lock_irqsave(&ei_local->page_lock, flags);
/* Read the counter registers, assuming we are in page 0. */
dev->stats.rx_frame_errors += ei_inb_p(ioaddr + EN0_COUNTER0);
dev->stats.rx_crc_errors+= ei_inb_p(ioaddr + EN0_COUNTER1);
dev->stats.rx_missed_errors += ei_inb_p(ioaddr + EN0_COUNTER2);
+ stats = &dev->stats;
spin_unlock_irqrestore(&ei_local->page_lock, flags);
- return &dev->stats;
+ return stats;
}
/*
dev->stats is not a pointer, it is an array embedded in the
struct net_device
So this patch is not needed, since dev->stats can not change.
Thanks for your reply :)
I do not understand that why "dev->stats can not change".
Its data is indeed changed by the code:
dev->stats.rx_frame_errors += ei_inb_p(ioaddr + EN0_COUNTER0);
dev->stats.rx_crc_errors+= ei_inb_p(ioaddr + EN0_COUNTER1);
dev->stats.rx_missed_errors += ei_inb_p(ioaddr + EN0_COUNTER2);
So I think a data race may occur when returning "dev->stats" without
lock protection.
By the way, I find this possible data race is similar to the previous
commit 7b31b4deda76 for the tg3 driver.
Best wishes,
Jia-Ju Bai
[PATCH] net: 8390: Fix possible data races in __ei_get_stats
The write operations to "dev->stats" are protected by
the spinlock on line 862-864, but the read operations to
this data on line 858 and 867 are not protected by the spinlock.
Thus, there may exist data races for "dev->stats".
To fix the data races, the read operations to "dev->stats" are
protected by the spinlock, and a local variable is used for return.
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/8390/lib8390.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/8390/lib8390.c
b/drivers/net/ethernet/8390/lib8390.c
index c9c55c9eab9f..198952247d30 100644
--- a/drivers/net/ethernet/8390/lib8390.c
+++ b/drivers/net/ethernet/8390/lib8390.c
@@ -852,19 +852,25 @@ static struct net_device_stats *__ei_get_stats(struct
net_device *dev)
unsigned long ioaddr = dev->base_addr;
struct ei_device *ei_local = netdev_priv(dev);
unsigned long flags;
+ struct net_device_stats *stats;
+
+ spin_lock_irqsave(&ei_local->page_lock, flags);
/* If the card is stopped, just return the present stats. */
- if (!netif_running(dev))
- return &dev->stats;
+ if (!netif_running(dev)) {
+ stats = &dev->stats;
+ spin_unlock_irqrestore(&ei_local->page_lock, flags);
+ return stats;
+ }
- spin_lock_irqsave(&ei_local->page_lock, flags);
/* Read the counter registers, assuming we are in page 0. */
dev->stats.rx_frame_errors += ei_inb_p(ioaddr + EN0_COUNTER0);
dev->stats.rx_crc_errors+= ei_inb_p(ioaddr + EN0_COUNTER1);
dev->stats.rx_missed_errors += ei_inb_p(ioaddr + EN0_COUNTER2);
+ stats = &dev->stats;
spin_unlock_irqrestore(&ei_local->page_lock, flags);
- return &dev->stats;
+ return stats;
}
/*
--
2.17.0
Re: [PATCH v2] dec: tulip: de4x5: Replace mdelay with usleep_range in de4x5_hw_init
On 2018/4/12 10:21, arvindY wrote: On Thursday 12 April 2018 07:00 AM, Jia-Ju Bai wrote: On 2018/4/12 0:16, James Bottomley wrote: On Wed, 2018-04-11 at 23:39 +0800, Jia-Ju Bai wrote: de4x5_hw_init() is never called in atomic context. de4x5_hw_init() is only called by de4x5_pci_probe(), which is only set as ".probe" in struct pci_driver. Despite never getting called from atomic context, de4x5_hw_init() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Did you actually test this? The usual reason for wanting m/udelay is that the timing must be exact. The driver is filled with mdelay()s for this reason. The one you've picked on is in the init path so it won't affect the runtime in any way. I also don't think we have the hrtimer machinery for usleep_range() to work properly on parisc, so I don't think the replacement works. James Hello, James. Thanks for your reply :) I agree that usleep_range() here will not much affect the real execution of this driver. But I think usleep_range() can more opportunity for other threads to use the CPU core to schedule during waiting. That is why I detect mdelay() that can be replaced with msleep() or usleep_range(). James is right, You have added all usleep_range() during system boot-up time. During boot-up system will run as single threaded. Where this change will not make much sense. System first priority is match the exact timing on each and every boot-up. Hello, Arvind. Thanks for your reply :) I admit I am not familiar with this driver. I did not know this driver is only loaded during system boot-up time, I thought this driver can be loaded as a kernel module (like many drivers) after system booting. After knowing this, I admit my patch is not proper, sorry... Best wishes, Jia-Ju Bai
Re: [PATCH] net: dsa: b53: Replace mdelay with msleep in b53_switch_reset_gpio
On 2018/4/12 0:19, Florian Fainelli wrote: On 04/11/2018 12:14 AM, Jia-Ju Bai wrote: On 2018/4/11 13:30, Phil Reid wrote: On 11/04/2018 09:51, Jia-Ju Bai wrote: b53_switch_reset_gpio() is never called in atomic context. The call chain ending up at b53_switch_reset_gpio() is: [1] b53_switch_reset_gpio() <- b53_switch_reset() <- b53_reset_switch() <- b53_setup() b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops. This function is not called in atomic context. Despite never getting called from atomic context, b53_switch_reset_gpio() calls mdelay() to busily wait. This is not necessary and can be replaced with msleep() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/net/dsa/b53/b53_common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 274f367..e070ff6 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -597,10 +597,10 @@ static void b53_switch_reset_gpio(struct b53_device *dev) /* Reset sequence: RESET low(50ms)->high(20ms) */ gpio_set_value(gpio, 0); -mdelay(50); +msleep(50); gpio_set_value(gpio, 1); -mdelay(20); +msleep(20); dev->current_page = 0xff; } Would that also imply gpio_set_value could be gpio_set_value_cansleep? Yes, I think gpio_set_value_cansleep() is okay here? Do I need to send a V2 patch to replace gpio_set_value()? Yes, I would lump these two changes in the same patch since this is effectively about solving sleeping vs. non sleeping operations. Okay, I have sent a V2 patch, and you can have a look :) Best wishes, Jia-Ju Bai
[PATCH v2] net: dsa: b53: Using sleep-able operations in b53_switch_reset_gpio
b53_switch_reset_gpio() is never called in atomic context. The call chain ending up at b53_switch_reset_gpio() is: [1] b53_switch_reset_gpio() <- b53_switch_reset() <- b53_reset_switch() <- b53_setup() b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops. This function is not called in atomic context. Despite never getting called from atomic context, b53_switch_reset_gpio() calls non-sleep operations mdelay() and gpio_set_value(). They are not necessary and can be replaced with msleep() and gpio_set_value_cansleep(). This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- v2: * Use gpio_set_value_cansleep() to replace gpio_set_value() additionally. Thanks for Florian and Phil for good advice. --- drivers/net/dsa/b53/b53_common.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 274f367..36cc60d 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -596,11 +596,11 @@ static void b53_switch_reset_gpio(struct b53_device *dev) /* Reset sequence: RESET low(50ms)->high(20ms) */ - gpio_set_value(gpio, 0); - mdelay(50); + gpio_set_value_cansleep(gpio, 0); + msleep(50); - gpio_set_value(gpio, 1); - mdelay(20); + gpio_set_value_cansleep(gpio, 1); + msleep(20); dev->current_page = 0xff; } -- 1.9.1
Re: [PATCH v2] dec: tulip: de4x5: Replace mdelay with usleep_range in de4x5_hw_init
On 2018/4/12 0:16, James Bottomley wrote: On Wed, 2018-04-11 at 23:39 +0800, Jia-Ju Bai wrote: de4x5_hw_init() is never called in atomic context. de4x5_hw_init() is only called by de4x5_pci_probe(), which is only set as ".probe" in struct pci_driver. Despite never getting called from atomic context, de4x5_hw_init() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Did you actually test this? The usual reason for wanting m/udelay is that the timing must be exact. The driver is filled with mdelay()s for this reason. The one you've picked on is in the init path so it won't affect the runtime in any way. I also don't think we have the hrtimer machinery for usleep_range() to work properly on parisc, so I don't think the replacement works. James Hello, James. Thanks for your reply :) I agree that usleep_range() here will not much affect the real execution of this driver. But I think usleep_range() can more opportunity for other threads to use the CPU core to schedule during waiting. That is why I detect mdelay() that can be replaced with msleep() or usleep_range(). Best wishes, Jia-Ju Bai
[PATCH v2] net: samsung: sxgbe: Replace mdelay with usleep_range in sxgbe_sw_reset
sxgbe_sw_reset() is never called in atomic context. sxgbe_sw_reset() is only called by sxgbe_drv_probe(), which is only called by sxgbe_platform_probe(). sxgbe_platform_probe() is set as ".probe" in struct platform_driver. This function is not called in atomic context. Despite never getting called from atomic context, sxgbe_sw_reset() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- v2: * Use usleep_range() to correct usleep() in v1. --- drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c index 89831ad..99cd586 100644 --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c @@ -2038,7 +2038,7 @@ static int sxgbe_sw_reset(void __iomem *addr) if (!(readl(addr + SXGBE_DMA_MODE_REG) & SXGBE_DMA_SOFT_RESET)) break; - mdelay(10); + usleep_range(1, 11000); } if (retry_count < 0) -- 1.9.1
[PATCH v2] dec: tulip: de4x5: Replace mdelay with usleep_range in de4x5_hw_init
de4x5_hw_init() is never called in atomic context.
de4x5_hw_init() is only called by de4x5_pci_probe(), which is only
set as ".probe" in struct pci_driver.
Despite never getting called from atomic context, de4x5_hw_init()
calls mdelay() to busily wait.
This is not necessary and can be replaced with usleep_range() to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
v2:
* Use usleep_range() to correct usleep() in v1.
---
drivers/net/ethernet/dec/tulip/de4x5.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/dec/tulip/de4x5.c
b/drivers/net/ethernet/dec/tulip/de4x5.c
index 0affee9..3fb0119 100644
--- a/drivers/net/ethernet/dec/tulip/de4x5.c
+++ b/drivers/net/ethernet/dec/tulip/de4x5.c
@@ -1107,7 +1107,7 @@ static int (*dc_infoblock[])(struct net_device *dev,
u_char, u_char *) = {
pdev = to_pci_dev (gendev);
pci_write_config_byte(pdev, PCI_CFDA_PSM, WAKEUP);
}
-mdelay(10);
+usleep_range(1, 11000);
RESET_DE4X5;
--
1.9.1
Re: [PATCH 1/2] staging: irda: Replace mdelay with usleep_range in stir421x_fw_upload
On 2018/4/11 22:26, David Miller wrote: From: Jia-Ju Bai Date: Wed, 11 Apr 2018 16:20:22 +0800 Okay, I now know why many of my patches were not replied. Many of your patches are not responded to because you handle patch feedback poorly sometimes. Okay, thanks for pointing it out. I will handle patch feedback much more carefully. Also, all of your networking submissions have been dropped because the net-next tree is closed. Okay, I will choose the proper tree to submit. Best wishes, Jia-Ju Bai
[PATCH v3] net: tipc: Replace GFP_ATOMIC with GFP_KERNEL in tipc_mon_create
tipc_mon_create() is never called in atomic context.
The call chain ending up at tipc_mon_create() is:
[1] tipc_mon_create() <- tipc_enable_bearer() <- tipc_nl_bearer_enable()
tipc_nl_bearer_enable() calls rtnl_lock(), which indicates this function
is not called in atomic context.
Despite never getting called from atomic context,
tipc_mon_create() calls kzalloc() with GFP_ATOMIC,
which does not sleep for allocation.
GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
which can sleep and improve the possibility of successful allocation.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
v2:
* Modify the description of GFP_ATOMIC in v1.
Thank Eric for good advice.
v3:
* Modify wrong text in description in v2.
Thank Ying for good advice.
---
net/tipc/monitor.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/tipc/monitor.c b/net/tipc/monitor.c
index 9e109bb..9714d80 100644
--- a/net/tipc/monitor.c
+++ b/net/tipc/monitor.c
@@ -604,9 +604,9 @@ int tipc_mon_create(struct net *net, int bearer_id)
if (tn->monitors[bearer_id])
return 0;
- mon = kzalloc(sizeof(*mon), GFP_ATOMIC);
- self = kzalloc(sizeof(*self), GFP_ATOMIC);
- dom = kzalloc(sizeof(*dom), GFP_ATOMIC);
+ mon = kzalloc(sizeof(*mon), GFP_KERNEL);
+ self = kzalloc(sizeof(*self), GFP_KERNEL);
+ dom = kzalloc(sizeof(*dom), GFP_KERNEL);
if (!mon || !self || !dom) {
kfree(mon);
kfree(self);
--
1.9.1
Re: [PATCH v2] net: tipc: Replace GFP_ATOMIC with GFP_KERNEL in tipc_mon_create
On 2018/4/11 18:11, Ying Xue wrote: On 04/10/2018 09:17 AM, Jia-Ju Bai wrote: tipc_mon_create() is never called in atomic context. The call chain ending up at dn_route_init() is: Sorry, I don't think there is any relationship between the following call chain with dn_route_init(). [1] tipc_mon_create() <- tipc_enable_bearer() <- tipc_nl_bearer_enable() tipc_nl_bearer_enable() calls rtnl_lock(), which indicates this function is not called in atomic context. Despite never getting called from atomic context, tipc_mon_create() calls kzalloc() with GFP_ATOMIC, which does not sleep for allocation. GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL, which can sleep and improve the possibility of sucessful allocation. s/sucessful/successful Thanks for your reply. I am sorry for my mistakes. I will revised the text and send a V3. Jia-Ju Bai
Re: [PATCH 1/2] staging: irda: Replace mdelay with usleep_range in stir421x_fw_upload
On 2018/4/11 16:17, Greg KH wrote: On Wed, Apr 11, 2018 at 04:11:00PM +0800, Jia-Ju Bai wrote: On 2018/4/11 16:03, Greg KH wrote: On Wed, Apr 11, 2018 at 03:17:10PM +0800, Jia-Ju Bai wrote: On 2018/4/11 14:41, Greg KH wrote: On Wed, Apr 11, 2018 at 09:29:34AM +0800, Jia-Ju Bai wrote: stir421x_fw_upload() is never called in atomic context. The call chain ending up at stir421x_fw_upload() is: [1] stir421x_fw_upload() <- stir421x_patch_device() <- irda_usb_probe() irda_usb_probe() is set as ".probe" in struct usb_driver. This function is not called in atomic context. Despite never getting called from atomic context, stir421x_fw_upload() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/staging/irda/drivers/irda-usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Please, at the very least, work off of Linus's tree. There is no drivers/staging/irda/ anymore :) Okay, sorry. Could you please recommend me a right tree or its git address? Have you looked in the MAINTAINERS file? Worst case, always use linux-next. greg k-h Oh, sorry, I did notice the git tree in the MAINTAINERS file. I always used linux-stable. linux-stable is almost never the tree to use as it is almost always 12000 patches behind what is in Linus's tree and about 2 changes behind linux-next. Okay, I now know why many of my patches were not replied. I should choose correct tree in the MAINTAINERS file. Thanks :) Best wishes, Jia-Ju Bai
Re: [PATCH 1/2] staging: irda: Replace mdelay with usleep_range in stir421x_fw_upload
On 2018/4/11 16:03, Greg KH wrote: On Wed, Apr 11, 2018 at 03:17:10PM +0800, Jia-Ju Bai wrote: On 2018/4/11 14:41, Greg KH wrote: On Wed, Apr 11, 2018 at 09:29:34AM +0800, Jia-Ju Bai wrote: stir421x_fw_upload() is never called in atomic context. The call chain ending up at stir421x_fw_upload() is: [1] stir421x_fw_upload() <- stir421x_patch_device() <- irda_usb_probe() irda_usb_probe() is set as ".probe" in struct usb_driver. This function is not called in atomic context. Despite never getting called from atomic context, stir421x_fw_upload() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/staging/irda/drivers/irda-usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Please, at the very least, work off of Linus's tree. There is no drivers/staging/irda/ anymore :) Okay, sorry. Could you please recommend me a right tree or its git address? Have you looked in the MAINTAINERS file? Worst case, always use linux-next. greg k-h Oh, sorry, I did notice the git tree in the MAINTAINERS file. I always used linux-stable. Thanks for telling me this :) Best wishes, Jia-Ju Bai
Re: [PATCH 1/2] staging: irda: Replace mdelay with usleep_range in stir421x_fw_upload
On 2018/4/11 14:41, Greg KH wrote: On Wed, Apr 11, 2018 at 09:29:34AM +0800, Jia-Ju Bai wrote: stir421x_fw_upload() is never called in atomic context. The call chain ending up at stir421x_fw_upload() is: [1] stir421x_fw_upload() <- stir421x_patch_device() <- irda_usb_probe() irda_usb_probe() is set as ".probe" in struct usb_driver. This function is not called in atomic context. Despite never getting called from atomic context, stir421x_fw_upload() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/staging/irda/drivers/irda-usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Please, at the very least, work off of Linus's tree. There is no drivers/staging/irda/ anymore :) Okay, sorry. Could you please recommend me a right tree or its git address? Best wishes, Jia-Ju Bai
Re: [PATCH] net: dsa: b53: Replace mdelay with msleep in b53_switch_reset_gpio
On 2018/4/11 13:30, Phil Reid wrote: On 11/04/2018 09:51, Jia-Ju Bai wrote: b53_switch_reset_gpio() is never called in atomic context. The call chain ending up at b53_switch_reset_gpio() is: [1] b53_switch_reset_gpio() <- b53_switch_reset() <- b53_reset_switch() <- b53_setup() b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops. This function is not called in atomic context. Despite never getting called from atomic context, b53_switch_reset_gpio() calls mdelay() to busily wait. This is not necessary and can be replaced with msleep() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/net/dsa/b53/b53_common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 274f367..e070ff6 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -597,10 +597,10 @@ static void b53_switch_reset_gpio(struct b53_device *dev) /* Reset sequence: RESET low(50ms)->high(20ms) */ gpio_set_value(gpio, 0); -mdelay(50); +msleep(50); gpio_set_value(gpio, 1); -mdelay(20); +msleep(20); dev->current_page = 0xff; } Would that also imply gpio_set_value could be gpio_set_value_cansleep? Yes, I think gpio_set_value_cansleep() is okay here? Do I need to send a V2 patch to replace gpio_set_value()? Best wishes, Jia-Ju Bai
[PATCH 1/2] isdn: hisax_fcpcipnp: Replace mdelay with usleep_range in fcpci_init
fcpci_init() is never called in atomic context. The call chain ending up at fcpci_init() is: [1] fcpci_init() <- fcpcipnp_setup() <- fcpnp_probe() fcpnp_probe() is set as ".probe" in struct pnp_driver. This function is not called in atomic context. Despite never getting called from atomic context, fcpci_init() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/isdn/hisax/hisax_fcpcipnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/isdn/hisax/hisax_fcpcipnp.c b/drivers/isdn/hisax/hisax_fcpcipnp.c index e4f7573..4789c9d 100644 --- a/drivers/isdn/hisax/hisax_fcpcipnp.c +++ b/drivers/isdn/hisax/hisax_fcpcipnp.c @@ -706,7 +706,7 @@ static inline void fcpci_init(struct fritz_adapter *adapter) outb(AVM_STATUS1_ENA_IOM | adapter->irq, adapter->io + AVM_STATUS1); - mdelay(10); + usleep_range(1, 11000); } // -- -- 1.9.1
[PATCH 2/2] isdn: hisax_fcpcipnp: Replace mdelay with usleep_range in fcpcipnp_setup
fcpcipnp_setup() is never called in atomic context.
The call chain ending up at fcpcipnp_setup() is:
[1] fcpcipnp_setup() <- fcpnp_probe()
fcpnp_probe() is set as ".probe" in struct pnp_driver.
This function is not called in atomic context.
Despite never getting called from atomic context, fcpcipnp_setup()
calls mdelay() to busily wait.
This is not necessary and can be replaced with usleep_range() to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
drivers/isdn/hisax/hisax_fcpcipnp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/hisax/hisax_fcpcipnp.c
b/drivers/isdn/hisax/hisax_fcpcipnp.c
index e4f7573..06068a42 100644
--- a/drivers/isdn/hisax/hisax_fcpcipnp.c
+++ b/drivers/isdn/hisax/hisax_fcpcipnp.c
@@ -772,11 +772,11 @@ static int fcpcipnp_setup(struct fritz_adapter *adapter)
// Reset
outb(0, adapter->io + AVM_STATUS0);
- mdelay(10);
+ usleep_range(1, 11000);
outb(AVM_STATUS0_RESET, adapter->io + AVM_STATUS0);
- mdelay(10);
+ usleep_range(1, 11000);
outb(0, adapter->io + AVM_STATUS0);
- mdelay(10);
+ usleep_range(1, 11000);
switch (adapter->type) {
case AVM_FRITZ_PCIV2:
--
1.9.1
[PATCH] net: sun: cassini: Replace GFP_ATOMIC with GFP_KERNEL in cas_check_invariants
cas_check_invariants() is never called in atomic context.
cas_check_invariants() is only called by cas_init_one(), which is
only set as ".probe" in struct pci_driver.
Despite never getting called from atomic context,
cas_check_invariants() calls alloc_pages() with GFP_ATOMIC,
which does not sleep for allocation.
GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
which can sleep and improve the possibility of sucessful allocation.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/sun/cassini.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sun/cassini.c
b/drivers/net/ethernet/sun/cassini.c
index 382993c..4dd38e3 100644
--- a/drivers/net/ethernet/sun/cassini.c
+++ b/drivers/net/ethernet/sun/cassini.c
@@ -3412,7 +3412,7 @@ static int cas_check_invariants(struct cas *cp)
#ifdef USE_PAGE_ORDER
if (PAGE_SHIFT < CAS_JUMBO_PAGE_SHIFT) {
/* see if we can allocate larger pages */
- struct page *page = alloc_pages(GFP_ATOMIC,
+ struct page *page = alloc_pages(GFP_KERNEL,
CAS_JUMBO_PAGE_SHIFT -
PAGE_SHIFT);
if (page) {
--
1.9.1
[PATCH] net: samsung: sxgbe: Replace mdelay with usleep_range in sxgbe_sw_reset
sxgbe_sw_reset() is never called in atomic context. sxgbe_sw_reset() is only called by sxgbe_drv_probe(), which is only called by sxgbe_platform_probe(). sxgbe_platform_probe() is set as ".probe" in struct platform_driver. This function is not called in atomic context. Despite never getting called from atomic context, sxgbe_sw_reset() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c index 89831ad..99cd586 100644 --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c @@ -2038,7 +2038,7 @@ static int sxgbe_sw_reset(void __iomem *addr) if (!(readl(addr + SXGBE_DMA_MODE_REG) & SXGBE_DMA_SOFT_RESET)) break; - mdelay(10); + usleep(1, 11000); } if (retry_count < 0) -- 1.9.1
[PATCH] net: ieee802154: atusb: Replace GFP_ATOMIC with GFP_KERNEL in atusb_probe
atusb_probe() is never called in atomic context. This function is only set as ".probe" in struct usb_driver. Despite never getting called from atomic context, atusb_probe() calls usb_alloc_urb() with GFP_ATOMIC, which does not sleep for allocation. GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL, which can sleep and improve the possibility of sucessful allocation. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/net/ieee802154/atusb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c index ef68851..ab6a505 100644 --- a/drivers/net/ieee802154/atusb.c +++ b/drivers/net/ieee802154/atusb.c @@ -789,7 +789,7 @@ static int atusb_probe(struct usb_interface *interface, atusb->tx_dr.bRequest = ATUSB_TX; atusb->tx_dr.wValue = cpu_to_le16(0); - atusb->tx_urb = usb_alloc_urb(0, GFP_ATOMIC); + atusb->tx_urb = usb_alloc_urb(0, GFP_KERNEL); if (!atusb->tx_urb) goto fail; -- 1.9.1
[PATCH] intel: i40evf: Replace GFP_ATOMIC with GFP_KERNEL in i40evf_add_vlan
i40evf_add_vlan() is never called in atomic context.
i40evf_add_vlan() is only called by i40evf_vlan_rx_add_vid(),
which is only set as ".ndo_vlan_rx_add_vid" in struct net_device_ops.
".ndo_vlan_rx_add_vid" is not called in atomic context.
Despite never getting called from atomic context,
i40evf_add_vlan() calls kzalloc() with GFP_ATOMIC,
which does not sleep for allocation.
GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
which can sleep and improve the possibility of sucessful allocation.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 1825d95..04b2b9c 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -770,7 +770,7 @@ i40evf_vlan_filter *i40evf_add_vlan(struct i40evf_adapter
*adapter, u16 vlan)
f = i40evf_find_vlan(adapter, vlan);
if (!f) {
- f = kzalloc(sizeof(*f), GFP_ATOMIC);
+ f = kzalloc(sizeof(*f), GFP_KERNEL);
if (!f)
goto clearout;
--
1.9.1
[PATCH] dec: tulip: de4x5: Replace mdelay with usleep_range in de4x5_hw_init
de4x5_hw_init() is never called in atomic context.
de4x5_hw_init() is only called by de4x5_pci_probe(), which is only
set as ".probe" in struct pci_driver.
Despite never getting called from atomic context, de4x5_hw_init()
calls mdelay() to busily wait.
This is not necessary and can be replaced with usleep_range() to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/dec/tulip/de4x5.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/dec/tulip/de4x5.c
b/drivers/net/ethernet/dec/tulip/de4x5.c
index 0affee9..3fb0119 100644
--- a/drivers/net/ethernet/dec/tulip/de4x5.c
+++ b/drivers/net/ethernet/dec/tulip/de4x5.c
@@ -1107,7 +1107,7 @@ static int (*dc_infoblock[])(struct net_device *dev,
u_char, u_char *) = {
pdev = to_pci_dev (gendev);
pci_write_config_byte(pdev, PCI_CFDA_PSM, WAKEUP);
}
-mdelay(10);
+usleep(1, 11000);
RESET_DE4X5;
--
1.9.1
[PATCH] net: dsa: b53: Replace mdelay with msleep in b53_switch_reset_gpio
b53_switch_reset_gpio() is never called in atomic context. The call chain ending up at b53_switch_reset_gpio() is: [1] b53_switch_reset_gpio() <- b53_switch_reset() <- b53_reset_switch() <- b53_setup() b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops. This function is not called in atomic context. Despite never getting called from atomic context, b53_switch_reset_gpio() calls mdelay() to busily wait. This is not necessary and can be replaced with msleep() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/net/dsa/b53/b53_common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 274f367..e070ff6 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -597,10 +597,10 @@ static void b53_switch_reset_gpio(struct b53_device *dev) /* Reset sequence: RESET low(50ms)->high(20ms) */ gpio_set_value(gpio, 0); - mdelay(50); + msleep(50); gpio_set_value(gpio, 1); - mdelay(20); + msleep(20); dev->current_page = 0xff; } -- 1.9.1
[PATCH 2/2] net: can: sja1000: Replace mdelay with usleep_range in pcan_add_channels
pcan_add_channels() is never called in atomic context. pcan_add_channels() is only called by pcan_probe(), which is only set as ".probe" in struct pcmcia_driver. Despite never getting called from atomic context, pcan_add_channels() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/net/can/sja1000/peak_pcmcia.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/can/sja1000/peak_pcmcia.c b/drivers/net/can/sja1000/peak_pcmcia.c index dd56133..1a1fbf3 100644 --- a/drivers/net/can/sja1000/peak_pcmcia.c +++ b/drivers/net/can/sja1000/peak_pcmcia.c @@ -530,7 +530,7 @@ static int pcan_add_channels(struct pcan_pccard *card) pcan_write_reg(card, PCC_CCR, ccr); /* wait 2ms before unresetting channels */ - mdelay(2); + usleep_range(2000, 3000); ccr &= ~PCC_CCR_RST_ALL; pcan_write_reg(card, PCC_CCR, ccr); -- 1.9.1
[PATCH 1/2] net: can: sja1000: Replace mdelay with usleep_range in peak_pci_probe
peak_pci_probe() is never called in atomic context. peak_pci_probe() is set as ".probe" in struct pci_driver. Despite never getting called from atomic context, peak_pci_probe() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/net/can/sja1000/peak_pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/can/sja1000/peak_pci.c b/drivers/net/can/sja1000/peak_pci.c index 131026f..48cf821 100644 --- a/drivers/net/can/sja1000/peak_pci.c +++ b/drivers/net/can/sja1000/peak_pci.c @@ -608,7 +608,7 @@ static int peak_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) writeb(0x00, cfg_base + PITA_GPIOICR); /* Toggle reset */ writeb(0x05, cfg_base + PITA_MISC + 3); - mdelay(5); + usleep_range(5000, 6000); /* Leave parport mux mode */ writeb(0x04, cfg_base + PITA_MISC + 3); -- 1.9.1
[PATCH 2/2] staging: irda: Replace mdelay with usleep_range in irda_usb_probe
irda_usb_probe() is never called in atomic context.
irda_usb_probe() is only set as ".probe" in struct usb_driver.
Despite never getting called from atomic context, irda_usb_probe()
calls mdelay() to busily wait.
This is not necessary and can be replaced with usleep_range() to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai
---
drivers/staging/irda/drivers/irda-usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/irda/drivers/irda-usb.c
b/drivers/staging/irda/drivers/irda-usb.c
index 723e49b..6ff5b08 100644
--- a/drivers/staging/irda/drivers/irda-usb.c
+++ b/drivers/staging/irda/drivers/irda-usb.c
@@ -1710,7 +1710,7 @@ static int irda_usb_probe(struct usb_interface *intf,
pr_debug("usb_control_msg failed %d\n", ret);
goto err_out_3;
} else {
- mdelay(10);
+ usleep_range(1, 11000);
}
}
--
1.9.1
[PATCH 1/2] staging: irda: Replace mdelay with usleep_range in stir421x_fw_upload
stir421x_fw_upload() is never called in atomic context. The call chain ending up at stir421x_fw_upload() is: [1] stir421x_fw_upload() <- stir421x_patch_device() <- irda_usb_probe() irda_usb_probe() is set as ".probe" in struct usb_driver. This function is not called in atomic context. Despite never getting called from atomic context, stir421x_fw_upload() calls mdelay() to busily wait. This is not necessary and can be replaced with usleep_range() to avoid busy waiting. This is found by a static analysis tool named DCNS written by myself. And I also manually check it. Signed-off-by: Jia-Ju Bai --- drivers/staging/irda/drivers/irda-usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/irda/drivers/irda-usb.c b/drivers/staging/irda/drivers/irda-usb.c index 723e49b..c6c8c2c 100644 --- a/drivers/staging/irda/drivers/irda-usb.c +++ b/drivers/staging/irda/drivers/irda-usb.c @@ -1050,7 +1050,7 @@ static int stir421x_fw_upload(struct irda_usb_cb *self, if (ret < 0) break; - mdelay(10); + usleep_range(1, 11000); } kfree(patch_block); -- 1.9.1
