Make the dump operation aware of changes on radio list and corresponding
inconsistent dumps. Change the dump iteration to be independent from
increasing radio indices on radio list.
Signed-off-by: Benjamin Beichler <[email protected]>
---
drivers/net/wireless/mac80211_hwsim.c | 43 ++++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 55d25e3fbbcc..2d4e97b6dc8f 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -493,6 +493,7 @@ static LIST_HEAD(hwsim_radios);
static struct workqueue_struct *hwsim_wq;
static struct rhashtable hwsim_radios_rht;
static int hwsim_radio_idx;
+static int hwsim_radios_generation = 1;
static struct platform_driver mac80211_hwsim_driver = {
.driver = {
@@ -2758,6 +2759,7 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
}
list_add_tail(&data->list, &hwsim_radios);
+ hwsim_radios_generation++;
spin_unlock_bh(&hwsim_radio_lock);
if (idx > 0)
@@ -3209,6 +3211,7 @@ static int hwsim_del_radio_nl(struct sk_buff *msg, struct genl_info *info)
list_del(&data->list);
rhashtable_remove_fast(&hwsim_radios_rht, &data->rht,
hwsim_rht_params);
+ hwsim_radios_generation++;
spin_unlock_bh(&hwsim_radio_lock);
mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy),
info);
@@ -3265,19 +3268,34 @@ static int hwsim_get_radio_nl(struct sk_buff *msg, struct genl_info *info)
static int hwsim_dump_radio_nl(struct sk_buff *skb,
struct netlink_callback *cb)
{
- int idx = cb->args[0];
- struct mac80211_hwsim_data *data = NULL;
- int res;
+ struct mac80211_hwsim_data *data =
+ (struct mac80211_hwsim_data *)cb->args[0];
+ int res = 0;
+ void *hdr;
spin_lock_bh(&hwsim_radio_lock);
+ cb->seq = hwsim_radios_generation;
+
+ /* list changed, send msg with dump interrupted header*/
+ if (cb->prev_seq && cb->seq != cb->prev_seq) {
+ hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+ cb->nlh->nlmsg_seq, &hwsim_genl_family,
+ NLM_F_MULTI, HWSIM_CMD_GET_RADIO);
+ if (!hdr)
+ res = -EMSGSIZE;
+ genl_dump_check_consistent(cb, hdr);
+ genlmsg_end(skb, hdr);
+ goto cleanup;
+ }
- if (idx == hwsim_radio_idx)
- goto done;
+ /* iterator is at head again, finish*/
+ if (data && &data->list == &hwsim_radios)
+ goto cleanup;
- list_for_each_entry(data, &hwsim_radios, list) {
- if (data->idx < idx)
- continue;
+ /* data will NULL or valid since we quit, if list changed */
+ data = list_prepare_entry(data, &hwsim_radios, list);
+ list_for_each_entry_continue(data, &hwsim_radios, list) {
if (!net_eq(wiphy_net(data->hw->wiphy), sock_net(skb->sk)))
continue;
@@ -3287,15 +3305,13 @@ static int hwsim_dump_radio_nl(struct sk_buff *skb,
NLM_F_MULTI);
if (res < 0)
break;
-
- idx = data->idx + 1;
}
- cb->args[0] = idx;
+ cb->args[0] = (long)data;
-done:
+cleanup:
spin_unlock_bh(&hwsim_radio_lock);
- return skb->len;
+ return (res)? res : skb->len;
}
/* Generic Netlink operations array */
@@ -3353,6 +3369,7 @@ static void destroy_radio(struct work_struct *work)
struct mac80211_hwsim_data *data =
container_of(work, struct mac80211_hwsim_data, destroy_work);
+ hwsim_radios_generation++;
mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy), NULL);
}
--
2.15.1
On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
> Change the dump iteration to be independent from
> increasing radio indices on radio list.
You can't do that, data structures can be deleted while the dump is
ongoing.
johannes
Am 15.01.2018 um 13:08 schrieb Johannes Berg:
> On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
>> Change the dump iteration to be independent from
>> increasing radio indices on radio list.
> You can't do that, data structures can be deleted while the dump is
> ongoing.
That's what I try to address with this patch. In the past there were
corner cases, which might have failed while deletion.
Nonetheless, the radio count is in the current version strictly
monotonic, maybe the description is misleading. :-)
So in the inner loop there is no deletion possible because of the
spinlock, and multiple calls of nl=5Fdump check the generation count and
cancel if it have changed.
>
> johannes
>
--
M.Sc. Benjamin Beichler
Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrotechnik
Institut f=C3=BCr Angewandte Mikroelektronik und Datentechnik
University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE
Richard-Wagner-Stra=C3=9Fe 31
18119 Rostock
Deutschland/Germany
phone: +49 (0) 381 498 - 7278
email: [email protected]
www: http://www.imd.uni-rostock.de/
On Mon, 2018-01-15 at 15:28 +0100, Benjamin Beichler wrote:
> Am 15.01.2018 um 13:08 schrieb Johannes Berg:
> > On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
> > > Change the dump iteration to be independent from
> > > increasing radio indices on radio list.
> >
> > You can't do that, data structures can be deleted while the dump is
> > ongoing.
>
> That's what I try to address with this patch. In the past there were
> corner cases, which might have failed while deletion.
Hah, ok.
> Nonetheless, the radio count is in the current version strictly
> monotonic, maybe the description is misleading. :-)
> So in the inner loop there is no deletion possible because of the
> spinlock, and multiple calls of nl_dump check the generation count and
> cancel if it have changed.
Ah, I missed that you cancel when it's changed, we don't usually do
that normally.
I'd still prefer if you could have the code not rely on the pointers,
because if somebody gets it wrong (forgets to update the generation
count) I'm not sure I want that to crash in some corner cases...
johannes
Am 22.01.2018 um 14:24 schrieb Johannes Berg:
> On Mon, 2018-01-15 at 15:28 +0100, Benjamin Beichler wrote:
>> Am 15.01.2018 um 13:08 schrieb Johannes Berg:
>>> On Wed, 2018-01-10 at 17:42 +0100, Benjamin Beichler wrote:
>>>> Change the dump iteration to be independent from
>>>> increasing radio indices on radio list.
>>> You can't do that, data structures can be deleted while the dump is
>>> ongoing.
>> That's what I try to address with this patch. In the past there were
>> corner cases, which might have failed while deletion.
> Hah, ok.
>
>> Nonetheless, the radio count is in the current version strictly
>> monotonic, maybe the description is misleading. :-)
>> So in the inner loop there is no deletion possible because of the
>> spinlock, and multiple calls of nl_dump check the generation count and
>> cancel if it have changed.
> Ah, I missed that you cancel when it's changed, we don't usually do
> that normally.
>
> I'd still prefer if you could have the code not rely on the pointers,
> because if somebody gets it wrong (forgets to update the generation
> count) I'm not sure I want that to crash in some corner cases...
Mkay, I saw the same pattern in other parts of the kernel, that's why I
adopted it. Then I restore the magic with searching the next radio id
bigger then the last one. Since on my machine, It needs several hundreds
of radios for a full message to cause interruptions, I think the code is
really rarely used and should be fail safe :-)
> johannes
>
--
M.Sc. Benjamin Beichler
Universität Rostock, Fakultät für Informatik und Elektrotechnik
Institut für Angewandte Mikroelektronik und Datentechnik
University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE
Richard-Wagner-Straße 31
18119 Rostock
Deutschland/Germany
phone: +49 (0) 381 498 - 7278
email: [email protected]
www: http://www.imd.uni-rostock.de/