by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH 5.10 047/126] ath10k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

Hi!

> Fix ath10k_wmi_tlv_op_pull_peer_stats_info() to hold RCU lock before it
> calls ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed.

It does that. But is also does the unlock even if it did not take the
lock:

> +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> @@ -576,13 +576,13 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar, struct sk_buff *skb)
> case WMI_TDLS_TEARDOWN_REASON_TX:
> case WMI_TDLS_TEARDOWN_REASON_RSSI:
> case WMI_TDLS_TEARDOWN_REASON_PTR_TIMEOUT:
> + rcu_read_lock();
> station = ieee80211_find_sta_by_ifaddr(ar->hw,
> ev->peer_macaddr.addr,
> NULL);
> if (!station) {
> ath10k_warn(ar, "did not find station from tdls peer event");
> - kfree(tb);
> - return;
> + goto exit;
> }
> arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
> ieee80211_tdls_oper_request(
> @@ -593,6 +593,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar, struct sk_buff *skb)
> );
> break;
> }
> +
> +exit:
> + rcu_read_unlock();
> kfree(tb);
> }

The switch only takes the lock in 3 branches, but it is released
unconditionally at the end.

Something like this?

Best regards,
Pavel

Signed-off-by: Pavel Machek (CIP) <[email protected]>

diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index e7072fc4f487..e03ff56d938b 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -582,20 +582,19 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar, struct sk_buff *skb)
NULL);
if (!station) {
ath10k_warn(ar, "did not find station from tdls peer event");
- goto exit;
- }
- arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
- ieee80211_tdls_oper_request(
+ } else {
+ arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
+ ieee80211_tdls_oper_request(
arvif->vif, station->addr,
NL80211_TDLS_TEARDOWN,
WLAN_REASON_TDLS_TEARDOWN_UNREACHABLE,
GFP_ATOMIC
);
+ }
+ rcu_read_unlock();
break;
}

-exit:
- rcu_read_unlock();
kfree(tb);
}

--
http://www.livejournal.com/~pavelmachek

Attachments:

(No filename) (2.25 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2021-04-05 22:59:36

by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH 5.10 048/126] net: ethernet: aquantia: Handle error cleanup of start on open

Hi!

> From: Nathan Rossi <[email protected]>
>
> [ Upstream commit 8a28af7a3e85ddf358f8c41e401a33002f7a9587 ]
>
> The aq_nic_start function can fail in a variety of cases which leaves
> the device in broken state.
>
> An example case where the start function fails is the
> request_threaded_irq which can be interrupted, resulting in a EINTR
> result. This can be manually triggered by bringing the link up (e.g. ip
> link set up) and triggering a SIGINT on the initiating process (e.g.
> Ctrl+C). This would put the device into a half configured state.
> Subsequently bringing the link up again would cause the napi_enable to
> BUG.
>
> In order to correctly clean up the failed attempt to start a device call
> aq_nic_stop.

No.

> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> @@ -71,8 +71,10 @@ static int aq_ndev_open(struct net_device *ndev)
> goto err_exit;
>
> err = aq_nic_start(aq_nic);
> - if (err < 0)
> + if (err < 0) {
> + aq_nic_stop(aq_nic);
> goto err_exit;
> + }
>
> err_exit:
> if (err < 0)

First, take a look at the goto. Does not need to be there.

Second check the crazy calling convention. If nic_start() fails, it
should clean up after itself.

Then, check the code. nic_stop() undoes initialization that was not
even done in the nic_start().

This introduces more problems than it solves.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Attachments:

(No filename) (1.54 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2021-04-06 00:16:26

Hi!

> This is the start of the stable review cycle for the 5.10.28 release.
> There are 126 patches in this series, all will be posted as a response
> to this one. If anyone has any issues with these being applied, please
> let me know.

CIP testing did not find any kernel problems here: (Siemens boards
are unavailable)

https://gitlab.com/cip-project/cip-testing/linux-stable-rc-ci/-/tree/linux-5.10.y

Tested-by: Pavel Machek (CIP) <[email protected]>

Best regards,
Pavel

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Attachments:

(No filename) (648.00 B)
signature.asc (188.00 B)
Digital signature Download all attachments