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 | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 48b9efed725e..fc8d9664cbfa 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -494,6 +494,7 @@ static struct rhashtable hwsim_radios_rht;
static spinlock_t hwsim_delete_lock;
static LIST_HEAD(delete_radios);
static int hwsim_radio_idx;
+static int hwsim_radios_generation = 1;
static struct platform_driver mac80211_hwsim_driver = {
.driver = {
@@ -2754,6 +2755,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)
@@ -3202,6 +3204,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);
@@ -3258,19 +3261,25 @@ 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;
+ struct mac80211_hwsim_data *data =
+ (struct mac80211_hwsim_data *)cb->args[0];
int res;
spin_lock_bh(&hwsim_radio_lock);
+ cb->seq = hwsim_radios_generation;
- if (idx == hwsim_radio_idx)
- goto done;
+ /* list changed */
+ if (cb->prev_seq && cb->seq != cb->prev_seq)
+ goto cleanup;
- list_for_each_entry(data, &hwsim_radios, list) {
- if (data->idx < idx)
- continue;
+ /* iterator is at head again, finish*/
+ if (data && &data->list == &hwsim_radios)
+ goto cleanup;
+ /* 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;
@@ -3280,13 +3289,11 @@ 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;
}
@@ -3348,6 +3355,7 @@ static void destroy_radio(struct work_struct *work)
spin_lock_bh(&hwsim_delete_lock);
list_del(&data->list);
+ hwsim_radios_generation++;
spin_unlock_bh(&hwsim_delete_lock);
mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy), NULL);
--
2.15.0
On Mon, 2017-12-11 at 14:02 +0100, Benjamin Beichler wrote:
>
> > But you added this:
> >
> > + /* list changed */
> > + if (cb->prev_seq && cb->seq != cb->prev_seq)
> > + goto cleanup;
> >
> > which is mostly just a copy of the inline.
> >
> > johannes
>
> Year you are right, but for nl_dump_check_consistent() I also need a
> header struct to write the flag to it and I thought a ghost header only
> to this function is also misleading. But if you think this is better, I
> can do that. Or we introduce a function, which really only check
> consistency and not also set the flag. I also thought the line is
> readable at it's own, because it's simply inconsistent if the sequence
> numbers are not equal.
It's readable, but there should be an indication to userspace in this
case, no?
johannes
On Mon, 2017-12-11 at 14:29 +0100, Benjamin Beichler wrote:
> I think we need to send something like an empty message, containing the
> flag. Because there exist the corner case, that while a dump is
> interrupted the complete list is deleted. Currently this could not
> happen because of non-parallel netlink callbacks, but maybe in the
> future parallel callbacks are enabled.
I don't know if we really want parallel, but I guess it's possible
eventually.
> Would it break things, if I simply create an header with
> HWSIM_CMD_GET_RADIO, but put no other information in it? Or how could it
> be done correctly?
I think that's probably OK.
johannes
Am 11.12.2017 um 13:14 schrieb Johannes Berg:
> On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
>> 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.
> Looks like this should use nl=5Fdump=5Fcheck=5Fconsistent()=3F
>
> johannes
>
It is called in mac80211=5Fhwsim=5Fget=5Fradio, I didn't changed that.
--
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, 2017-12-11 at 13:37 +0100, Benjamin Beichler wrote:
> Am 11.12.2017 um 13:14 schrieb Johannes Berg:
> > On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
> > > 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.
> >
> > Looks like this should use nl_dump_check_consistent()?
> >
> > johannes
> >
>
> It is called in mac80211_hwsim_get_radio, I didn't changed that.
But you added this:
+ /* list changed */
+ if (cb->prev_seq && cb->seq != cb->prev_seq)
+ goto cleanup;
which is mostly just a copy of the inline.
johannes
Am 11.12.2017 um 13:49 schrieb Johannes Berg:
> On Mon, 2017-12-11 at 13:37 +0100, Benjamin Beichler wrote:
>> Am 11.12.2017 um 13:14 schrieb Johannes Berg:
>>> On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
>>>> Make the dump operation aware of changes on radio list and correspon=
ding
>>>> inconsistent dumps. Change the dump iteration to be independent from=
>>>> increasing radio indices on radio list.
>>> Looks like this should use nl_dump_check_consistent()?
>>>
>>> johannes
>>>
>> It is called in mac80211_hwsim_get_radio, I didn't changed that.
> But you added this:
>
> + /* list changed */
> + if (cb->prev_seq && cb->seq !=3D cb->prev_seq)
> + goto cleanup;
>
> which is mostly just a copy of the inline.
>
> johannes
Year you are right, but for nl_dump_check_consistent() I also need a
header struct to write the flag to it and I thought a ghost header only
to this function is also misleading. But if you think this is better, I
can do that. Or we introduce a function, which really only check
consistency and not also set the flag. I also thought the line is
readable at it's own, because it's simply inconsistent if the sequence
numbers are not equal.
--=20
M.Sc. Benjamin Beichler
Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrote=
chnik
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 Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote:
> 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.
Looks like this should use nl_dump_check_consistent()?
johannes
Am 11.12.2017 um 14:07 schrieb Johannes Berg:
> On Mon, 2017-12-11 at 14:02 +0100, Benjamin Beichler wrote:
>>> But you added this:
>>>
>>> + /* list changed */
>>> + if (cb->prev_seq && cb->seq !=3D cb->prev_seq)
>>> + goto cleanup;
>>>
>>> which is mostly just a copy of the inline.
>>>
>>> johannes
>> Year you are right, but for nl_dump_check_consistent() I also need a
>> header struct to write the flag to it and I thought a ghost header onl=
y
>> to this function is also misleading. But if you think this is better, =
I
>> can do that. Or we introduce a function, which really only check
>> consistency and not also set the flag. I also thought the line is
>> readable at it's own, because it's simply inconsistent if the sequence=
>> numbers are not equal.
> It's readable, but there should be an indication to userspace in this
> case, no?
>
> johannes
>
Mhh, OK you are totally right.
I think we need to send something like an empty message, containing the
flag. Because there exist the corner case, that while a dump is
interrupted the complete list is deleted. Currently this could not
happen because of non-parallel netlink callbacks, but maybe in the
future parallel callbacks are enabled.
Would it break things, if I simply create an header with
HWSIM_CMD_GET_RADIO, but put no other information in it? Or how could it
be done correctly?
--=20
M.Sc. Benjamin Beichler
Universit=C3=A4t Rostock, Fakult=C3=A4t f=C3=BCr Informatik und Elektrote=
chnik
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/