* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Johannes Berg @ 2017-10-18 10:29 UTC (permalink / raw)
To: Kees Cook, linux-wireless; +Cc: David S. Miller, netdev, linux-kernel
In-Reply-To: <20171017202545.GA115810@beast>
Hi,
[quoting your other email:]
> This has been the least trivial timer conversion yet. Given the use of
> RCU and other things I may not even know about, I'd love to get a close
> look at this. I *think* this is correct, as it will re-lookup the tid
> entries when firing the timer.
I'm not really sure why you're doing the lookup again? That seems
pointless, since you already have the right structure, and already rely
on it being valid. You can't really get a new struct assigned to the
same TID without the old one being destroyed.
> -static void sta_rx_agg_session_timer_expired(unsigned long data)
> +static void sta_rx_agg_session_timer_expired(struct timer_list *t)
> {
> - /* not an elegant detour, but there is no choice as the timer passes
> - * only one argument, and various sta_info are needed here, so init
> - * flow in sta_info_create gives the TID as data, while the timer_to_id
> - * array gives the sta through container_of */
> - u8 *ptid = (u8 *)data;
> - u8 *timer_to_id = ptid - *ptid;
> - struct sta_info *sta = container_of(timer_to_id, struct sta_info,
> - timer_to_tid[0]);
> + struct tid_ampdu_rx *tid_rx_timer =
> + from_timer(tid_rx_timer, t, session_timer);
> + struct sta_info *sta = tid_rx_timer->sta;
> + u16 tid = tid_rx_timer->tid;
> struct tid_ampdu_rx *tid_rx;
> unsigned long timeout;
>
> rcu_read_lock();
> - tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[*ptid]);
> + tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
> if (!tid_rx) {
> rcu_read_unlock();
So through all of this, I'm pretty sure we can just use tid_rx_timer
instead of tid_rx.
(Same for TX)
Anyway, the change here looks correct to me, so I'll apply it and then
perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since
the valid range is 0-15 (in theory, in practice 0-7).
johannes
^ permalink raw reply
* pull-request: iwlwifi-next 2017-10-18
From: Luca Coelho @ 2017-10-18 10:30 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, linuxwifi
[-- Attachment #1: Type: text/plain, Size: 3063 bytes --]
Hi Kalle,
Here's the second batch of patches intended for v4.15. It contains the
last patch set I send out with v2 of the lq_color patch.
I have sent this out before and kbuildbot reported success.
Please let me know if there are any issues.
Cheers,
Luca.
The following changes since commit 66cc044249603e12e1dbba347f03bdbc9f171fdf:
bcma: use bcma_debug and pr_cont in MIPS driver (2017-10-17 17:22:07 +0300)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git tags/iwlwifi-next-for-kalle-2017-10-18
for you to fetch changes up to 3c798a45318e098e9937b0fee1e0cf986174fbbe:
iwlwifi: pcie: remove set but not used variable tcph (2017-10-18 13:02:01 +0300)
----------------------------------------------------------------
Second batch of iwlwifi patches for 4.15
* Allocate reorder buffer dynamically to save memory;
* Fix a FW dump problem in the A000 family;
* Fix for a statistics gathering issue (v2);
* Sort the list of 9000 devices to make it easier to find entries;
* A couple of cleanups in the FW dump code;
* Remove some unnecessary variables and fields and calculations;
----------------------------------------------------------------
Beni Lev (1):
iwlwifi: mvm: allow reading UMAC error data from SMEM in A000 devices
Johannes Berg (3):
iwlwifi: mvm: allocate reorder buffer according to need
iwlwifi: mvm: pass baid_data to iwl_mvm_release_frames()
iwlwifi: pcie: remove set but not used variable tcph
Liad Kaufman (1):
iwlwifi: mvm: add missing lq_color
Luca Coelho (3):
iwlwifi: mvm: move umac_error_event_table validity check to where it's set
iwlwifi: define minimum valid address for umac_error_event_table in cfg
iwlwifi: pcie: sort IDs for the 9000 series for easier comparisons
Sara Sharon (1):
iwlwifi: mvm: remove duplicated fields in mvm reorder buffer
drivers/net/wireless/intel/iwlwifi/cfg/8000.c | 3 ++-
drivers/net/wireless/intel/iwlwifi/cfg/9000.c | 3 ++-
drivers/net/wireless/intel/iwlwifi/cfg/a000.c | 3 ++-
drivers/net/wireless/intel/iwlwifi/fw/api/tx.h | 4 ++--
drivers/net/wireless/intel/iwlwifi/iwl-config.h | 1 +
drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 20 +++++++++++++++++---
drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 43 ++++++++++++++++++++++++++++++++++---------
drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 49 +++++++++++++++++++++++++++++++------------------
drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 44 ++++++++++++++++++++++++++++++++++++--------
drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 10 ++++++++++
drivers/net/wireless/intel/iwlwifi/mvm/utils.c | 17 ++++-------------
drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------------------------------------
drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c | 5 +----
13 files changed, 184 insertions(+), 102 deletions(-)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH V6 1/5] mac80211: Enable TDLS peer buffer STA feature
From: Johannes Berg @ 2017-10-18 10:35 UTC (permalink / raw)
To: yintang, ath10k; +Cc: linux-wireless
In-Reply-To: <1508133633-23214-2-git-send-email-yintang@qti.qualcomm.com>
On Mon, 2017-10-16 at 14:00 +0800, yintang@qti.qualcomm.com wrote:
> From: Yingying Tang <yintang@qti.qualcomm.com>
>
> Enable TDLS peer buffer STA feature.
> Set extended capability bit to enable buffer STA when driver
> support it.
It would help if you could (build) test your changes :-)
johannes
^ permalink raw reply
* Re: [PATCH] netlink: use NETLINK_CB(in_skb).sk instead of looking it up
From: David Miller @ 2017-10-18 11:21 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, netdev, ebiederman, johannes.berg
In-Reply-To: <20171016145749.31793-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 16 Oct 2017 16:57:49 +0200
> From: Johannes Berg <johannes.berg@intel.com>
>
> When netlink_ack() reports an allocation error to the sending
> socket, there's no need to look up the sending socket since
> it's available in the SKB's CB. Use that instead of going to
> the trouble of looking it up.
>
> Note that the pointer is only available since Eric Biederman's
> commit 3fbc290540a1 ("netlink: Make the sending netlink socket availabe in NETLINK_CB")
> which is far newer than the original lookup code (Oct 2003)
> (though the field was called 'ssk' in that commit and only got
> renamed to 'sk' later, I'd actually argue 'ssk' was better - or
> perhaps it should've been 'source_sk' - since there are so many
> different 'sk's involved.)
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Applied to net-next.
^ permalink raw reply
* Re: [PATCH] netlink: fix netlink_ack() extack race
From: David Miller @ 2017-10-18 11:23 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, netdev, johannes.berg
In-Reply-To: <20171016150953.17612-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 16 Oct 2017 17:09:53 +0200
> From: Johannes Berg <johannes.berg@intel.com>
>
> It seems that it's possible to toggle NETLINK_F_EXT_ACK
> through setsockopt() while another thread/CPU is building
> a message inside netlink_ack(), which could then trigger
> the WARN_ON()s I added since if it goes from being turned
> off to being turned on between allocating and filling the
> message, the skb could end up being too small.
>
> Avoid this whole situation by storing the value of this
> flag in a separate variable and using that throughout the
> function instead.
>
> Fixes: 2d4bc93368f5 ("netlink: extended ACK reporting")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Johannes Berg @ 2017-10-18 11:31 UTC (permalink / raw)
To: Kees Cook, linux-wireless; +Cc: David S. Miller, netdev, linux-kernel
In-Reply-To: <1508322566.2674.17.camel@sipsolutions.net>
On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote:
> Anyway, the change here looks correct to me, so I'll apply it and then
> perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since
> the valid range is 0-15 (in theory, in practice 0-7).
Well, I guess I'm clearly wrong - it's crashing our test suite left and
right.
I'll dig a little bit, but I don't have much time today, and will be
out for a few days starting tomorrow.
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Johannes Berg @ 2017-10-18 11:37 UTC (permalink / raw)
To: Kees Cook, linux-wireless; +Cc: David S. Miller, netdev, linux-kernel
In-Reply-To: <1508326291.2674.21.camel@sipsolutions.net>
On Wed, 2017-10-18 at 13:31 +0200, Johannes Berg wrote:
> On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote:
>
> > Anyway, the change here looks correct to me, so I'll apply it and then
> > perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since
> > the valid range is 0-15 (in theory, in practice 0-7).
>
> Well, I guess I'm clearly wrong - it's crashing our test suite left and
> right.
>
> I'll dig a little bit, but I don't have much time today, and will be
> out for a few days starting tomorrow.
Ok, it's pretty obvious - you never initialize the new fields in tid_tx
(sta and tid), only in tid_rx. Let's see if it passes with that fixed.
johannes
^ permalink raw reply
* [PATCH] mac80211: avoid looking up tid_tx/tid_rx from timers
From: Johannes Berg @ 2017-10-18 12:10 UTC (permalink / raw)
To: linux-wireless; +Cc: Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
There's no need to re-lookup the data structures now that
we actually get them immediately with from_timer(), just
avoid that. The struct has to be valid anyway, otherwise
the timer object itself would no longer be valid, and we
can't have a different version of the struct since only a
single session per TID is permitted.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
net/mac80211/agg-rx.c | 17 +++--------------
net/mac80211/agg-tx.c | 31 ++++++++-----------------------
2 files changed, 11 insertions(+), 37 deletions(-)
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index d444752dbf40..35e94483fb8c 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -153,27 +153,16 @@ EXPORT_SYMBOL(ieee80211_stop_rx_ba_session);
*/
static void sta_rx_agg_session_timer_expired(struct timer_list *t)
{
- struct tid_ampdu_rx *tid_rx_timer =
- from_timer(tid_rx_timer, t, session_timer);
- struct sta_info *sta = tid_rx_timer->sta;
- u8 tid = tid_rx_timer->tid;
- struct tid_ampdu_rx *tid_rx;
+ struct tid_ampdu_rx *tid_rx = from_timer(tid_rx, t, session_timer);
+ struct sta_info *sta = tid_rx->sta;
+ u8 tid = tid_rx->tid;
unsigned long timeout;
- rcu_read_lock();
- tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
- if (!tid_rx) {
- rcu_read_unlock();
- return;
- }
-
timeout = tid_rx->last_rx + TU_TO_JIFFIES(tid_rx->timeout);
if (time_is_after_jiffies(timeout)) {
mod_timer(&tid_rx->session_timer, timeout);
- rcu_read_unlock();
return;
}
- rcu_read_unlock();
ht_dbg(sta->sdata, "RX session timer expired on %pM tid %d\n",
sta->sta.addr, tid);
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 3680b380e70c..1d0bf857a3b5 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -424,18 +424,12 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid,
*/
static void sta_addba_resp_timer_expired(struct timer_list *t)
{
- struct tid_ampdu_tx *tid_tx_timer =
- from_timer(tid_tx_timer, t, addba_resp_timer);
- struct sta_info *sta = tid_tx_timer->sta;
- u8 tid = tid_tx_timer->tid;
- struct tid_ampdu_tx *tid_tx;
+ struct tid_ampdu_tx *tid_tx = from_timer(tid_tx, t, addba_resp_timer);
+ struct sta_info *sta = tid_tx->sta;
+ u8 tid = tid_tx->tid;
/* check if the TID waits for addBA response */
- rcu_read_lock();
- tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
- if (!tid_tx ||
- test_bit(HT_AGG_STATE_RESPONSE_RECEIVED, &tid_tx->state)) {
- rcu_read_unlock();
+ if (test_bit(HT_AGG_STATE_RESPONSE_RECEIVED, &tid_tx->state)) {
ht_dbg(sta->sdata,
"timer expired on %pM tid %d not expecting addBA response\n",
sta->sta.addr, tid);
@@ -446,7 +440,6 @@ static void sta_addba_resp_timer_expired(struct timer_list *t)
sta->sta.addr, tid);
ieee80211_stop_tx_ba_session(&sta->sta, tid);
- rcu_read_unlock();
}
void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
@@ -524,29 +517,21 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
*/
static void sta_tx_agg_session_timer_expired(struct timer_list *t)
{
- struct tid_ampdu_tx *tid_tx_timer =
- from_timer(tid_tx_timer, t, session_timer);
- struct sta_info *sta = tid_tx_timer->sta;
- u8 tid = tid_tx_timer->tid;
- struct tid_ampdu_tx *tid_tx;
+ struct tid_ampdu_tx *tid_tx = from_timer(tid_tx, t, session_timer);
+ struct sta_info *sta = tid_tx->sta;
+ u8 tid = tid_tx->tid;
unsigned long timeout;
- rcu_read_lock();
- tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
- if (!tid_tx || test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
- rcu_read_unlock();
+ if (test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
return;
}
timeout = tid_tx->last_tx + TU_TO_JIFFIES(tid_tx->timeout);
if (time_is_after_jiffies(timeout)) {
mod_timer(&tid_tx->session_timer, timeout);
- rcu_read_unlock();
return;
}
- rcu_read_unlock();
-
ht_dbg(sta->sdata, "tx session timer expired on %pM tid %d\n",
sta->sta.addr, tid);
--
2.14.2
^ permalink raw reply related
* Re: wireless-regdb: Update regulatory rules for Kazakhstan (KZ) on 5GHz
From: Seth Forshee @ 2017-10-18 13:49 UTC (permalink / raw)
To: Андрей Иванов
Cc: wireless-regdb, linux-wireless
In-Reply-To: <1508325977.297437473@f42.i.mail.ru>
On Wed, Oct 18, 2017 at 02:26:17PM +0300, Андрей Иванов wrote:
> Please add support for 5 GHz in Kazakhstan
> (5170 - 5250 @ 80), (20), AUTO-BW
> (5250 - 5330 @ 80), (20), DFS, AUTO-BW
> (5650 - 5730 @ 80), (30), DFS
> (5735 - 5835 @ 80), (30)
We got a very similar submission not that long ago, and I had some
questions which were never answered.
http://lists.infradead.org/pipermail/wireless-regdb/2017-August/001086.html
That one provided a link to some documentation for Kazakhstan, but the
information there seemed incomplete to me. Can you answer the questions
I asked there, or provide a link to a more complete source of
information for the regulations in Kazakhstan?
Thanks,
Seth
^ permalink raw reply
* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-18 14:17 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, David S. Miller, Network Development, LKML
In-Reply-To: <1508326655.2674.22.camel@sipsolutions.net>
On Wed, Oct 18, 2017 at 4:37 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2017-10-18 at 13:31 +0200, Johannes Berg wrote:
>> On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote:
>>
>> > Anyway, the change here looks correct to me, so I'll apply it and then
>> > perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since
>> > the valid range is 0-15 (in theory, in practice 0-7).
I started with u8 tid, but I saw it cast to u16 and in a few other
places it was u16, so I went with that ultimately.
>> Well, I guess I'm clearly wrong - it's crashing our test suite left and
>> right.
>>
>> I'll dig a little bit, but I don't have much time today, and will be
>> out for a few days starting tomorrow.
>
> Ok, it's pretty obvious - you never initialize the new fields in tid_tx
> (sta and tid), only in tid_rx. Let's see if it passes with that fixed.
Argh, whoops, thanks for working on this.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-18 14:19 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, David S. Miller, Network Development, LKML
In-Reply-To: <1508322566.2674.17.camel@sipsolutions.net>
On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> This has been the least trivial timer conversion yet. Given the use of
>> RCU and other things I may not even know about, I'd love to get a close
>> look at this. I *think* this is correct, as it will re-lookup the tid
>> entries when firing the timer.
>
> I'm not really sure why you're doing the lookup again? That seems
> pointless, since you already have the right structure, and already rely
> on it being valid. You can't really get a new struct assigned to the
> same TID without the old one being destroyed.
I couldn't tell what the lifetime expectation was, so I left the
re-lookup. I assumed it was possible to have a timer fire after the
structure had been removed from the station structure.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
From: Ben Greear @ 2017-10-18 14:50 UTC (permalink / raw)
To: Johannes Berg, linux-wireless@vger.kernel.org, ath10k, kirtika
In-Reply-To: <1508312033.2674.9.camel@sipsolutions.net>
On 10/18/2017 12:33 AM, Johannes Berg wrote:
> Hi,
>
>> The call to set the rate in the driver comes before the error
>> check.
>>
>> if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
>> ret = drv_set_bitrate_mask(local, sdata, mask);
>> if (ret) {
>> pr_err("%s: drv-set-bitrate-mask had error
>> return: %d\n",
>> sdata->dev->name, ret);
>> return ret;
>> }
>> }
>>
>> /*
>> * If active validate the setting and reject it if it doesn't
>> leave
>> * at least one basic rate usable, since we really have to be
>> able
>> * to send something, and if we're an AP we have to be able to
>> do
>> * so at a basic rate so that all clients can receive it.
>> */
>> if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
>> sdata->vif.bss_conf.chandef.chan) {
>> u32 basic_rates = sdata->vif.bss_conf.basic_rates;
>> enum nl80211_band band = sdata-
>>> vif.bss_conf.chandef.chan->band;
>>
>> if (!(mask->control[band].legacy & basic_rates)) {
>> #### I changed this code so I could set a
>> single rate... --Ben
>> pr_err("%s: WARNING: no legacy rates for
>> band[%d] in set-bitrate-mask.\n",
>> sdata->dev->name, band);
>> }
>> }
>
> Heh, that's just dumb. I guess I'll fix that by putting the test first,
> no idea how that happened.
>
>>>
>>>> So, I think we should relax this check, at least for ath10k.
>>>
>>> Well, yes and no. I don't think we should make ath10k special here,
>>> and
>>> this fixes a real problem - namely that you can set up the system
>>> so
>>> that you have no usable rates at all, and then you just get a
>>> WARN_ON
>>> and start using the lowest possible rate...
>>
>> Well, there are a million ways to set up a broken system,
>
> True, but this one actually happened in practice, for some reason, and
> stopping the user from constantly shooting themselves in the foot seems
> like a good idea to me. Especially if the user (or application) can't
> really even know what they're getting into.
>
> Now, the case in question was _client_ mode, but still.
>
>> and setting a single rate has a useful purpose, especially with
>> ath10k since it has such limited rate-setting capabilities.
>
> You're stretching the definition of "useful purpose" a bit here though,
> you're about the only one who's ever going to need to set a single
> rate.
People trying to do regulatory testing want this feature, and other people
that are not me also like to test with specific rates. Still a small-ish set
of people, but bigger than just me at least.
This used to work, and now is broken, so it is a regression of at least somewhat
useful feature. I think we need a way to re-enable this, even if it requires
poking some sysfs or debugfs file to allow this check to be relaxed.
And for that matter, it might be nice to allow purposefully broken ratesets
(with part of basic missing, for instance), in order to test peer devices (including
other Linux stacks).
>> There is basically never going to be a case where setting a single
>> tx-rate on an AP is a good idea in a general production environment,
>> so maybe a possible WARN-ON is fine?
>
> A WARN_ON() for a user configuration is never fine.
>
> You're assuming that there's actually a user sitting there and doing
> this, which is not necessarily the case.
>
> Even rejecting a single rate setting wouldn't be enough because you can
> get into problems even when you enable multiple rates, e.g. if you
> enable all the CCK rates while connected on 5 GHz.
>
>> If you *do* set up an AP with a limited rateset, then it should
>> simply fail to allow a station to connect if it does not have any
>> matching rates.
>
> That's what requiring at least one basic rate to remain does. If you
> want to have basic rates 6,9,12 and then configure only 18, how would
> the client get rejected? Just configure basic rates differently
> beforehand, and then you can do this easily, and the right thing with
> rejecting clients will happen automatically (in fact, clients might not
> even attempt to connect - even better!)
>
>> This might go back to a previous idea I had (and patches I posted and
>> carry in my tree) to allow setting a different advertise rateset vs
>> usable tx-rateset.
>
> You still have the same problem with which clients support them and
> require them etc.
For regulatory testing purposes: You can advertise normal basic rateset,
and you can accept those (and even transmit mgt and bcast frames on them),
but for data tx, you use a single fixed rate. That is one reason it is nice
to have the advertised rateset different from the tx rateset.
>> For existing stations that might not match the new fixed rate, we
>> could purposefully kick them off I guess, but seems like a lot of
>> work for a case that seems pretty irrelevant.
>
> For better or worse, there are people using this API programmatically
> without a user baby-sitting it, so we need to make it work in all
> cases. We can let the user shoot themselves in the foot and have only a
> single usable rate left, but we can't let them hang themselves and have
> no rate left at all.
Out of curiosity, couldn't 'iw' do the same checks? It is a lot easier to
return a useful error code/message from a user-space app anyway, in case that
can work.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: using verifier to ensure a BPF program uses certain metadata?
From: Alexei Starovoitov @ 2017-10-18 17:42 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Daniel Borkmann, linux-wireless
In-Reply-To: <1508309791.2674.1.camel@sipsolutions.net>
On Wed, Oct 18, 2017 at 08:56:31AM +0200, Johannes Berg wrote:
> > > Now, I realize that people could trivially just work around this in
> > > their program if they wanted, but I think most will take the
> > > reminder
> > > and just implement
> > >
> > > if (ctx->is_data_ethernet)
> > > return DROP_FRAME;
> > >
> > > instead, since mostly data frames will not be very relevant to
> > > them.
> > >
> > > What do you think?
> >
> > sounds fine and considering new verifier ops after Jakub refactoring
> > a check that is_data_ethernet was accessed would fit nicely.
> > Without void** hack.
>
> Ok, thanks! I'll have to check what Jakub is doing there, do you have a
> pointer to that refactoring?
something similar to
commit 4f9218aaf8a4 ("bpf: move knowledge about post-translation offsets out of verifier")
^ permalink raw reply
* Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
From: Oleksij Rempel @ 2017-10-18 17:56 UTC (permalink / raw)
To: Ben Greear, Johannes Berg, linux-wireless@vger.kernel.org, ath10k,
kirtika
In-Reply-To: <2ad42671-59c4-80ed-4bca-f874eb53d653@candelatech.com>
Am 18.10.2017 um 16:50 schrieb Ben Greear:
>
>
> On 10/18/2017 12:33 AM, Johannes Berg wrote:
>> Hi,
>>
>>> The call to set the rate in the driver comes before the error
>>> check.
>>>
>>> if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) {
>>> ret = drv_set_bitrate_mask(local, sdata, mask);
>>> if (ret) {
>>> pr_err("%s: drv-set-bitrate-mask had error
>>> return: %d\n",
>>> sdata->dev->name, ret);
>>> return ret;
>>> }
>>> }
>>>
>>> /*
>>> * If active validate the setting and reject it if it doesn't
>>> leave
>>> * at least one basic rate usable, since we really have to be
>>> able
>>> * to send something, and if we're an AP we have to be able to
>>> do
>>> * so at a basic rate so that all clients can receive it.
>>> */
>>> if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
>>> sdata->vif.bss_conf.chandef.chan) {
>>> u32 basic_rates = sdata->vif.bss_conf.basic_rates;
>>> enum nl80211_band band = sdata-
>>>> vif.bss_conf.chandef.chan->band;
>>>
>>> if (!(mask->control[band].legacy & basic_rates)) {
>>> #### I changed this code so I could set a
>>> single rate... --Ben
>>> pr_err("%s: WARNING: no legacy rates for
>>> band[%d] in set-bitrate-mask.\n",
>>> sdata->dev->name, band);
>>> }
>>> }
>>
>> Heh, that's just dumb. I guess I'll fix that by putting the test first,
>> no idea how that happened.
>>
>>>>
>>>>> So, I think we should relax this check, at least for ath10k.
>>>>
>>>> Well, yes and no. I don't think we should make ath10k special here,
>>>> and
>>>> this fixes a real problem - namely that you can set up the system
>>>> so
>>>> that you have no usable rates at all, and then you just get a
>>>> WARN_ON
>>>> and start using the lowest possible rate...
>>>
>>> Well, there are a million ways to set up a broken system,
>>
>> True, but this one actually happened in practice, for some reason, and
>> stopping the user from constantly shooting themselves in the foot seems
>> like a good idea to me. Especially if the user (or application) can't
>> really even know what they're getting into.
>>
>> Now, the case in question was _client_ mode, but still.
>>
>>> and setting a single rate has a useful purpose, especially with
>>> ath10k since it has such limited rate-setting capabilities.
>>
>> You're stretching the definition of "useful purpose" a bit here though,
>> you're about the only one who's ever going to need to set a single
>> rate.
>
> People trying to do regulatory testing want this feature, and other people
> that are not me also like to test with specific rates. Still a
> small-ish set
> of people, but bigger than just me at least.
Till now i was interviewing different people who was asking for this for
ath9k-htc. So I would say we have:
- academical researchers
- testers
- R&D
- exploit and penetration testers
- HAM
- just hackers
As for me, it sounds a s lot.
--
Regards,
Oleksij
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: update Johannes Berg's entries
From: Joe Perches @ 2017-10-18 19:27 UTC (permalink / raw)
To: Johannes Berg, linux-wireless; +Cc: Johannes Berg
In-Reply-To: <20171010075759.28377-1-johannes@sipsolutions.net>
On Tue, 2017-10-10 at 09:57 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Update my MAINTAINERS file entries to list all the right files.
> Since I'm also the de-facto wireless extensions maintainer,
> there's little point in excluding those.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> MAINTAINERS | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f0c37be4e04a..e90cdecd7b5d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3329,17 +3329,22 @@ S: Maintained
> F: drivers/auxdisplay/cfag12864bfb.c
> F: include/linux/cfag12864b.h
>
> -CFG80211 and NL80211
> +802.11 (including CFG80211/NL80211)
You should also move this block in MAINTAINERS to
the appropriate alphabetic order location.
> M: Johannes Berg <johannes@sipsolutions.net>
> L: linux-wireless@vger.kernel.org
> W: http://wireless.kernel.org/
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git
> S: Maintained
> +F: net/wireless/
> F: include/uapi/linux/nl80211.h
> +F: include/linux/ieee80211.h
> +F: include/net/wext.h
> F: include/net/cfg80211.h
> -F: net/wireless/*
> -X: net/wireless/wext*
> +F: include/net/iw_handler.h
> +F: include/net/ieee80211_radiotap.h
> +F: Documentation/driver-api/80211/cfg80211.rst
> +F: Documentation/networking/regulatory.txt
>
> CHAR and MISC DRIVERS
> M: Arnd Bergmann <arnd@arndb.de>
> @@ -8207,6 +8212,7 @@ F: Documentation/networking/mac80211-injection.txt
> F: include/net/mac80211.h
> F: net/mac80211/
> F: drivers/net/wireless/mac80211_hwsim.[ch]
> +F: Documentation/networking/mac80211_hwsim/README
>
> MAILBOX API
> M: Jassi Brar <jassisinghbrar@gmail.com>
> @@ -11491,6 +11497,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git
> S: Maintained
> F: Documentation/rfkill.txt
> +F: Documentation/ABI/stable/sysfs-class-rfkill
> F: net/rfkill/
>
> RHASHTABLE
^ permalink raw reply
* Re: [PATCH 00/58] networking: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-18 19:45 UTC (permalink / raw)
To: Kalle Valo
Cc: David S. Miller, Network Development, Thomas Gleixner, LKML,
linux-wireless
In-Reply-To: <87h8ux2ena.fsf@codeaurora.org>
On Tue, Oct 17, 2017 at 10:44 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Kees Cook <keescook@chromium.org> writes:
>> Which split is preferred? I had been trying to separate wireless from
>> the rest of net (but missed some cases).
>
> So what we try to follow is that I apply all patches for
> drivers/net/wireless to my wireless-drivers trees, with exception of
> Johannes taking mac80211_hwsim.c patches to his mac80211 trees. And
> Johannes of course takes all patches for net/wireless and net/mac80211.
>
> So in general I prefer that I take all drivers/net/wireless patches and
> make it obvious for Dave that he can ignore those patches (not mix
> wireless-drivers and net patches into same set etc). But like I said,
> it's ok to push API changes like these via Dave's net trees as well if
> you want (and if Dave is ok with that). The chances of conflicts is low,
> and if there are be any those would be easy to fix either by me or Dave.
Okay, great. That'll help. I'll wait for the dust to settle and rebase
against -next, and then I'll see what's outstanding and double-check
where they need to be sent (and I'll queue new conversions up
accordingly too).
>>> For now I'll just drop all your timer_setup() related patches from my
>>> queue and I'll assume Dave will take those. Ok?
>>>
>>> [1] https://patchwork.kernel.org/project/linux-wireless/list/
>>
>> I guess I'll wait to see what Dave says.
>
> Ok, I don't drop the patches from my queue quite yet then.
Alright, thanks very much!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
From: Johannes Berg @ 2017-10-18 20:34 UTC (permalink / raw)
To: Oleksij Rempel, Ben Greear, linux-wireless@vger.kernel.org,
ath10k, kirtika
In-Reply-To: <8c2e0e98-f3f4-6e0c-1bf0-43dfa6e97275@rempel-privat.de>
Hi,
On Wed, 2017-10-18 at 19:56 +0200, Oleksij Rempel wrote:
>
> > People trying to do regulatory testing want this feature, and other people
> > that are not me also like to test with specific rates. Still a
> > small-ish set of people, but bigger than just me at least.
>
> Till now i was interviewing different people who was asking for this for
> ath9k-htc. So I would say we have:
> - academical researchers
> - testers
> - R&D
> - exploit and penetration testers
> - HAM
> - just hackers
>
> As for me, it sounds a s lot.
Making (literally) millions of devices in the field hit a WARN_ON() is
not really acceptable either though.
You can argue that this introduced a regression, but putting the old
behaviour back would equally be a regression, for more systems by a few
orders of magnitude.
In any case, I've already suggested a way to fix this, but you've both
completely ignored that part of my email. All I've been reading is that
you're demanding that I fix this, and arguments about how much people
are allowed to shoot themselves in the foot, none of which is very
constructive.
I might even fix it myself eventually, if only to appease the people
who say we have a zero tolerance no regressions rule, but it's not
exactly the most important thing I'm doing right now (also, I'll be
going on vacation for a few days, and you can probably implement my
suggestion in that time, and then I can review it when I get back on
Monday.)
Let's just say that I think we're discussing the wrong thing here - we
ought to be discussing how it can be fixed, and perhaps you can even be
constructive in suggesting (and testing, which I can't really do)
changes.
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Johannes Berg @ 2017-10-18 20:50 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-wireless, David S. Miller, Network Development, LKML
In-Reply-To: <CAGXu5jKY5rTuR_w=zj5Bz06f9OrW35eD4sYfkai2Gac_edG4ag@mail.gmail.com>
On Wed, 2017-10-18 at 07:19 -0700, Kees Cook wrote:
> On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > > This has been the least trivial timer conversion yet. Given the use of
> > > RCU and other things I may not even know about, I'd love to get a close
> > > look at this. I *think* this is correct, as it will re-lookup the tid
> > > entries when firing the timer.
> >
> > I'm not really sure why you're doing the lookup again? That seems
> > pointless, since you already have the right structure, and already rely
> > on it being valid. You can't really get a new struct assigned to the
> > same TID without the old one being destroyed.
>
> I couldn't tell what the lifetime expectation was, so I left the
> re-lookup. I assumed it was possible to have a timer fire after the
> structure had been removed from the station structure.
Oh, right, I guess that would've been possible in theory. It's not
actually possible though since the aggregation sessions can't live on
without a station, so I've already made a follow-up patch to remove the
indirection.
johannes
^ permalink raw reply
* Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
From: Ben Greear @ 2017-10-18 20:51 UTC (permalink / raw)
To: Johannes Berg, Oleksij Rempel, linux-wireless@vger.kernel.org,
ath10k, kirtika
In-Reply-To: <1508358869.2674.55.camel@sipsolutions.net>
On 10/18/2017 01:34 PM, Johannes Berg wrote:
> Hi,
>
> On Wed, 2017-10-18 at 19:56 +0200, Oleksij Rempel wrote:
>>
>>> People trying to do regulatory testing want this feature, and other people
>>> that are not me also like to test with specific rates. Still a
>>> small-ish set of people, but bigger than just me at least.
>>
>> Till now i was interviewing different people who was asking for this for
>> ath9k-htc. So I would say we have:
>> - academical researchers
>> - testers
>> - R&D
>> - exploit and penetration testers
>> - HAM
>> - just hackers
>>
>> As for me, it sounds a s lot.
>
> Making (literally) millions of devices in the field hit a WARN_ON() is
> not really acceptable either though.
>
> You can argue that this introduced a regression, but putting the old
> behaviour back would equally be a regression, for more systems by a few
> orders of magnitude.
>
> In any case, I've already suggested a way to fix this, but you've both
> completely ignored that part of my email. All I've been reading is that
> you're demanding that I fix this, and arguments about how much people
> are allowed to shoot themselves in the foot, none of which is very
> constructive.
You mean this part? It wasn't clear to me that you thought this was a good
solution that should be implemented.
"
I guess you could implement this part? I.e. iterating the clients and
checking that they all support the rate that is set. However, then you
also need to implement that this gets reset when a new client that
doesn't support this rate connects.
"
The first part seems OK, but the second seems like a pain. Maybe just
keep a new client from being able to connect at all if it doesn't support
any available rates?
Thanks,
Ben
> I might even fix it myself eventually, if only to appease the people
> who say we have a zero tolerance no regressions rule, but it's not
> exactly the most important thing I'm doing right now (also, I'll be
> going on vacation for a few days, and you can probably implement my
> suggestion in that time, and then I can review it when I get back on
> Monday.)
>
> Let's just say that I think we're discussing the wrong thing here - we
> ought to be discussing how it can be fixed, and perhaps you can even be
> constructive in suggesting (and testing, which I can't really do)
> changes.
>
> johannes
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
From: Johannes Berg @ 2017-10-18 21:02 UTC (permalink / raw)
To: Ben Greear, Oleksij Rempel, linux-wireless@vger.kernel.org,
ath10k, kirtika
In-Reply-To: <9791c86a-a15f-4c99-ab84-ce00b242d70f@candelatech.com>
On Wed, 2017-10-18 at 13:51 -0700, Ben Greear wrote:
> "
> I guess you could implement this part? I.e. iterating the clients and
> checking that they all support the rate that is set. However, then you
> also need to implement that this gets reset when a new client that
> doesn't support this rate connects.
> "
>
> The first part seems OK, but the second seems like a pain. Maybe just
> keep a new client from being able to connect at all if it doesn't support
> any available rates?
I suppose if you reject the NEW_STATION command, then hostapd will
reject the association though, so it's probably not hard to do.
However, I'm not really sure why you'd want that? If you do want that
then basically you're just implemented a very roundabout way of adding
this rate to the basic rates, so you might as well just add it and work
with the current basic rates check?
We'll need to be a little careful what happens with client-mode
interfaces, which is where we actually observed hitting the WARN_ON()
about not having any rates at all, but I think I already put a reset of
the rates there anyway if they're not compatible with the AP. At least
that's easier because there's only one client.
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-18 21:02 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, David S. Miller, Network Development, LKML
In-Reply-To: <1508359811.2674.56.camel@sipsolutions.net>
On Wed, Oct 18, 2017 at 1:50 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2017-10-18 at 07:19 -0700, Kees Cook wrote:
>> On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > > This has been the least trivial timer conversion yet. Given the use of
>> > > RCU and other things I may not even know about, I'd love to get a close
>> > > look at this. I *think* this is correct, as it will re-lookup the tid
>> > > entries when firing the timer.
>> >
>> > I'm not really sure why you're doing the lookup again? That seems
>> > pointless, since you already have the right structure, and already rely
>> > on it being valid. You can't really get a new struct assigned to the
>> > same TID without the old one being destroyed.
>>
>> I couldn't tell what the lifetime expectation was, so I left the
>> re-lookup. I assumed it was possible to have a timer fire after the
>> structure had been removed from the station structure.
>
> Oh, right, I guess that would've been possible in theory. It's not
> actually possible though since the aggregation sessions can't live on
> without a station, so I've already made a follow-up patch to remove the
> indirection.
Okay, cool, thanks.
It seems like I have a similar case in the iwlwifi driver too, but
it's different enough that I'm not sure about that one either. I'll
send that when I've got it ready.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: Setting single rate in ath10k broken by "reject/clear user rate mask if not usable"
From: Ben Greear @ 2017-10-18 21:30 UTC (permalink / raw)
To: Johannes Berg, Oleksij Rempel, linux-wireless@vger.kernel.org,
ath10k, kirtika
In-Reply-To: <1508360527.2674.61.camel@sipsolutions.net>
On 10/18/2017 02:02 PM, Johannes Berg wrote:
> On Wed, 2017-10-18 at 13:51 -0700, Ben Greear wrote:
>> "
>> I guess you could implement this part? I.e. iterating the clients and
>> checking that they all support the rate that is set. However, then you
>> also need to implement that this gets reset when a new client that
>> doesn't support this rate connects.
>> "
>>
>> The first part seems OK, but the second seems like a pain. Maybe just
>> keep a new client from being able to connect at all if it doesn't support
>> any available rates?
>
> I suppose if you reject the NEW_STATION command, then hostapd will
> reject the association though, so it's probably not hard to do.
> However, I'm not really sure why you'd want that? If you do want that
> then basically you're just implemented a very roundabout way of adding
> this rate to the basic rates, so you might as well just add it and work
> with the current basic rates check?
If a user specifies a specific rate or rate-set, then I do not think we
should quietly change it out from under them. So, we could check at the
time the rate-set is applied (all current peers). We can reject the change
at that point if one of the peers does not support any common rates.
And, whenever a peer tries to be associated, we can check that there is some common rateset
in place. If there is no common rateset, then reject the association
one way or another.
> We'll need to be a little careful what happens with client-mode
> interfaces, which is where we actually observed hitting the WARN_ON()
> about not having any rates at all, but I think I already put a reset of
> the rates there anyway if they're not compatible with the AP. At least
> that's easier because there's only one client.
It would be easy to configure a station to do VHT MCS 8 only, and then
it would never be able to associate with an HT AP, for instance. I don't
think this should be a WARN_ON event, just a failure. It would be great
if we could get a useful error message back to the caller, but I am not
sure how feasible that is with the current netlink and mac80211 code.
If your main concern is hitting a WARN_ON, maybe just change it to a
quieter error message or remove it entirely and just return a failure
code?
Thanks,
Ben
>
> johannes
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* rtlwifi oops
From: nirinA @ 2017-10-18 22:31 UTC (permalink / raw)
To: linux-wireless
hello there,
i got the oops below with a rtl8192cu:0bda:8178 and kernel 4.13.6, but
cannot reproduce it.
i use this device since 4.3 or so without noticing any issue.
nirinA
[ 239.338040] usb 2-1.3: new high-speed USB device number 4 using ehci-pci
[ 239.417728] usb 2-1.3: New USB device found, idVendor=0bda,
idProduct=8178
[ 239.417730] usb 2-1.3: New USB device strings: Mfr=1, Product=2,
SerialNumber=3
[ 239.417731] usb 2-1.3: Product: 802.11n WLAN Adapter
[ 239.417732] usb 2-1.3: Manufacturer: Realtek
[ 239.417733] usb 2-1.3: SerialNumber: 00e04c000001
[ 239.578100] rtl8192cu: Chip version 0x11
[ 239.678225] usb 2-1-port3: disabled by hub (EMI?), re-enabling...
[ 239.678230] usb 2-1.3: USB disconnect, device number 4
[ 239.679128] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut!
status:0xffffffed value=0x0
[ 239.679131] rtl_usb: reg 0x32, usbctrl_vendorreq TimeOut!
status:0xffffffed value=0x20
[ 239.679133] rtl_usb: reg 0x33, usbctrl_vendorreq TimeOut!
status:0xffffffed value=0x80
[ 239.679134] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut!
status:0xffffffed value=0x80206f30
[ 239.700640] rtl_usb: rx_max_size 15360, rx_urb_num 8, in_ep 1
[ 239.700648] BUG: unable to handle kernel NULL pointer dereference
at (null)
[ 239.700682] IP: rtl_deinit_core+0x2e/0x90 [rtlwifi]
[ 239.700693] PGD 1c5d5d067
[ 239.700694] P4D 1c5d5d067
[ 239.700701] PUD 1c5d9b067
[ 239.700708] PMD 0
[ 239.700727] Oops: 0000 [#1] SMP
[ 239.700735] Modules linked in: rtl8192cu(+) rtl_usb rtl8192c_common
rtlwifi mac80211 cfg80211 rfkill appletalk ipx p8023 p8022 psnap llc
nct6775 hwmon_vid ipv6 hid_generic usbhid hid coretemp hwmon
x86_pkg_temp_thermal intel_powerclamp snd_hda_codec_hdmi
snd_hda_codec_realtek snd_hda_codec_generic kvm_intel i915 kvm
drm_kms_helper evdev i2c_dev irqbypass syscopyarea sysfillrect sysimgblt
r8169 crc32_pclmul serio_raw crc32c_intel mii snd_pcsp fb_sys_fops drm
fan thermal button battery 8250 8250_base serial_core mei_me
snd_hda_intel mei intel_gtt snd_hda_codec video agpgart ehci_pci
ehci_hcd lpc_ich snd_hwdep snd_hda_core snd_pcm i2c_algo_bit snd_timer
i2c_i801 i2c_core snd soundcore fuse
[ 239.700875] CPU: 0 PID: 1174 Comm: udevd Not tainted 4.13.6.20171012 #1
[ 239.700889] Hardware name: To be filled by O.E.M. To be filled by
O.E.M./ONDA H61V Ver:4.01, BIOS 4.6.5 01/07/2013
[ 239.700909] task: ffff8801c5fbc980 task.stack: ffffc900009a0000
[ 239.700924] RIP: 0010:rtl_deinit_core+0x2e/0x90 [rtlwifi]
[ 239.700936] RSP: 0000:ffffc900009a3b40 EFLAGS: 00010207
[ 239.700947] RAX: 0000000000000282 RBX: ffff8801b9440780 RCX:
00000000ffffffea
[ 239.700962] RDX: 0000000000000001 RSI: 0000000000000282 RDI:
0000000000000000
[ 239.700977] RBP: ffff8801b944b070 R08: 0000000000000001 R09:
00000000000002b0
[ 239.700991] R10: ffffea0008473480 R11: 0000000000000000 R12:
ffff8801b9441560
[ 239.701006] R13: ffff8801b944b880 R14: 0000000000000000 R15:
ffff8801b9441560
[ 239.701021] FS: 00007f3eb636e7c0(0000) GS:ffff88021f200000(0000)
knlGS:0000000000000000
[ 239.701037] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 239.701050] CR2: 0000000000000000 CR3: 00000001c5d99000 CR4:
00000000001406f0
[ 239.701064] Call Trace:
[ 239.701074] ? rtl_usb_probe+0x6b6/0xc50 [rtl_usb]
[ 239.701088] ? usb_probe_interface+0xe2/0x2a0
[ 239.701100] ? driver_probe_device+0x21d/0x2d0
[ 239.701111] ? __driver_attach+0x8a/0x90
[ 239.701121] ? driver_probe_device+0x2d0/0x2d0
[ 239.701133] ? bus_for_each_dev+0x5c/0x90
[ 239.701143] ? bus_add_driver+0x196/0x220
[ 239.701154] ? driver_register+0x57/0xc0
[ 239.701165] ? usb_register_driver+0x7c/0x140
[ 239.701175] ? 0xffffffffa064e000
[ 239.701185] ? do_one_initcall+0x4b/0x190
[ 239.701197] ? kmem_cache_alloc_trace+0xe4/0x1c0
[ 239.701208] ? do_init_module+0x22/0x1e1
[ 239.701219] ? do_init_module+0x5b/0x1e1
[ 239.701229] ? load_module+0x21ff/0x2760
[ 239.701239] ? SYSC_finit_module+0x90/0xb0
[ 239.701249] ? SYSC_finit_module+0x90/0xb0
[ 239.701261] ? entry_SYSCALL_64_fastpath+0x1a/0xa5
[ 239.701272] Code: 00 00 41 57 31 f6 41 56 41 55 41 54 55 53 48 89 fb
e8 e7 fe ff ff 4c 8b 7b 48 49 8b bf 10 9b 00 00 49 8d af 10 9b 00 00 48
39 ef <48> 8b 1f 74 44 49 bd 00 01 00 00 00 00 ad de 49 89 de 49 bc 00
[ 239.701327] RIP: rtl_deinit_core+0x2e/0x90 [rtlwifi] RSP:
ffffc900009a3b40
[ 239.702028] CR2: 0000000000000000
[ 239.705370] ---[ end trace 6ec9029c0d9c0e13 ]---
[ 239.706311] udevd[528]: worker [1174] failed while handling
'/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.3/2-1.3:1.0'
^ permalink raw reply
* Re: rtlwifi oops
From: James Cameron @ 2017-10-18 22:33 UTC (permalink / raw)
To: linux-wireless
In-Reply-To: <cf61cfda-1e32-3ec3-d558-b508c3b923b6@gmail.com>
On Thu, Oct 19, 2017 at 01:31:43AM +0300, nirinA wrote:
> hello there,
> i got the oops below with a rtl8192cu:0bda:8178 and kernel 4.13.6, but
> cannot reproduce it.
> i use this device since 4.3 or so without noticing any issue.
>
> nirinA
>
> [ 239.338040] usb 2-1.3: new high-speed USB device number 4 using ehci-pci
> [ 239.417728] usb 2-1.3: New USB device found, idVendor=0bda,
> idProduct=8178
> [ 239.417730] usb 2-1.3: New USB device strings: Mfr=1, Product=2,
> SerialNumber=3
> [ 239.417731] usb 2-1.3: Product: 802.11n WLAN Adapter
> [ 239.417732] usb 2-1.3: Manufacturer: Realtek
> [ 239.417733] usb 2-1.3: SerialNumber: 00e04c000001
> [ 239.578100] rtl8192cu: Chip version 0x11
> [ 239.678225] usb 2-1-port3: disabled by hub (EMI?), re-enabling...
Just prior to the oops, your USB hub disabled the port being used by
the wireless device.
While the response of the driver seems wrong, it is a difficult
condition to reproduce; one must either force or forge the disabling
by the hub.
> [ 239.678230] usb 2-1.3: USB disconnect, device number 4
> [ 239.679128] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut!
> status:0xffffffed value=0x0
> [ 239.679131] rtl_usb: reg 0x32, usbctrl_vendorreq TimeOut!
> status:0xffffffed value=0x20
> [ 239.679133] rtl_usb: reg 0x33, usbctrl_vendorreq TimeOut!
> status:0xffffffed value=0x80
> [ 239.679134] rtl_usb: reg 0x30, usbctrl_vendorreq TimeOut!
> status:0xffffffed value=0x80206f30
> [ 239.700640] rtl_usb: rx_max_size 15360, rx_urb_num 8, in_ep 1
> [ 239.700648] BUG: unable to handle kernel NULL pointer dereference at
> (null)
> [ 239.700682] IP: rtl_deinit_core+0x2e/0x90 [rtlwifi]
> [ 239.700693] PGD 1c5d5d067
> [ 239.700694] P4D 1c5d5d067
> [ 239.700701] PUD 1c5d9b067
> [ 239.700708] PMD 0
>
> [ 239.700727] Oops: 0000 [#1] SMP
> [ 239.700735] Modules linked in: rtl8192cu(+) rtl_usb rtl8192c_common
> rtlwifi mac80211 cfg80211 rfkill appletalk ipx p8023 p8022 psnap llc nct6775
> hwmon_vid ipv6 hid_generic usbhid hid coretemp hwmon x86_pkg_temp_thermal
> intel_powerclamp snd_hda_codec_hdmi snd_hda_codec_realtek
> snd_hda_codec_generic kvm_intel i915 kvm drm_kms_helper evdev i2c_dev
> irqbypass syscopyarea sysfillrect sysimgblt r8169 crc32_pclmul serio_raw
> crc32c_intel mii snd_pcsp fb_sys_fops drm fan thermal button battery 8250
> 8250_base serial_core mei_me snd_hda_intel mei intel_gtt snd_hda_codec video
> agpgart ehci_pci ehci_hcd lpc_ich snd_hwdep snd_hda_core snd_pcm
> i2c_algo_bit snd_timer i2c_i801 i2c_core snd soundcore fuse
> [ 239.700875] CPU: 0 PID: 1174 Comm: udevd Not tainted 4.13.6.20171012 #1
> [ 239.700889] Hardware name: To be filled by O.E.M. To be filled by
> O.E.M./ONDA H61V Ver:4.01, BIOS 4.6.5 01/07/2013
> [ 239.700909] task: ffff8801c5fbc980 task.stack: ffffc900009a0000
> [ 239.700924] RIP: 0010:rtl_deinit_core+0x2e/0x90 [rtlwifi]
> [ 239.700936] RSP: 0000:ffffc900009a3b40 EFLAGS: 00010207
> [ 239.700947] RAX: 0000000000000282 RBX: ffff8801b9440780 RCX:
> 00000000ffffffea
> [ 239.700962] RDX: 0000000000000001 RSI: 0000000000000282 RDI:
> 0000000000000000
> [ 239.700977] RBP: ffff8801b944b070 R08: 0000000000000001 R09:
> 00000000000002b0
> [ 239.700991] R10: ffffea0008473480 R11: 0000000000000000 R12:
> ffff8801b9441560
> [ 239.701006] R13: ffff8801b944b880 R14: 0000000000000000 R15:
> ffff8801b9441560
> [ 239.701021] FS: 00007f3eb636e7c0(0000) GS:ffff88021f200000(0000)
> knlGS:0000000000000000
> [ 239.701037] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 239.701050] CR2: 0000000000000000 CR3: 00000001c5d99000 CR4:
> 00000000001406f0
> [ 239.701064] Call Trace:
> [ 239.701074] ? rtl_usb_probe+0x6b6/0xc50 [rtl_usb]
> [ 239.701088] ? usb_probe_interface+0xe2/0x2a0
> [ 239.701100] ? driver_probe_device+0x21d/0x2d0
> [ 239.701111] ? __driver_attach+0x8a/0x90
> [ 239.701121] ? driver_probe_device+0x2d0/0x2d0
> [ 239.701133] ? bus_for_each_dev+0x5c/0x90
> [ 239.701143] ? bus_add_driver+0x196/0x220
> [ 239.701154] ? driver_register+0x57/0xc0
> [ 239.701165] ? usb_register_driver+0x7c/0x140
> [ 239.701175] ? 0xffffffffa064e000
> [ 239.701185] ? do_one_initcall+0x4b/0x190
> [ 239.701197] ? kmem_cache_alloc_trace+0xe4/0x1c0
> [ 239.701208] ? do_init_module+0x22/0x1e1
> [ 239.701219] ? do_init_module+0x5b/0x1e1
> [ 239.701229] ? load_module+0x21ff/0x2760
> [ 239.701239] ? SYSC_finit_module+0x90/0xb0
> [ 239.701249] ? SYSC_finit_module+0x90/0xb0
> [ 239.701261] ? entry_SYSCALL_64_fastpath+0x1a/0xa5
> [ 239.701272] Code: 00 00 41 57 31 f6 41 56 41 55 41 54 55 53 48 89 fb e8
> e7 fe ff ff 4c 8b 7b 48 49 8b bf 10 9b 00 00 49 8d af 10 9b 00 00 48 39 ef
> <48> 8b 1f 74 44 49 bd 00 01 00 00 00 00 ad de 49 89 de 49 bc 00
> [ 239.701327] RIP: rtl_deinit_core+0x2e/0x90 [rtlwifi] RSP:
> ffffc900009a3b40
> [ 239.702028] CR2: 0000000000000000
> [ 239.705370] ---[ end trace 6ec9029c0d9c0e13 ]---
> [ 239.706311] udevd[528]: worker [1174] failed while handling
> '/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.3/2-1.3:1.0'
>
--
James Cameron
http://quozl.netrek.org/
^ permalink raw reply
* Re: [PATCH] staging: wilc1000: replace redundant computations with 0
From: Joe Perches @ 2017-10-18 22:54 UTC (permalink / raw)
To: Colin King, Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
linux-wireless, devel
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20171010140548.18016-1-colin.king@canonical.com>
On Tue, 2017-10-10 at 15:05 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Shifting and masking strHostIfSetMulti->enabled is redundant since
> enabled is a bool and so all the shifted and masked values will be
> zero. Replace them with zero to simplify the code.
>
> Detected by CoverityScan, CID#1339458 ("Bad shift operation") and
> CID#1339506 ("Operands don't affect result").
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/staging/wilc1000/host_interface.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index 7b620658ec38..94477dd08c85 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -2417,9 +2417,9 @@ static void Handle_SetMulticastFilter(struct wilc_vif *vif,
>
> pu8CurrByte = wid.val;
> *pu8CurrByte++ = (strHostIfSetMulti->enabled & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 16) & 0xFF);
> - *pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 24) & 0xFF);
> + *pu8CurrByte++ = 0;
> + *pu8CurrByte++ = 0;
> + *pu8CurrByte++ = 0;
This might be more an indication of another defect
Perhaps this is just supposed to be
*pu8CurrByte++ = strHostIfSetMulti->enabled;
without the three byte sets to zero after that.
> *pu8CurrByte++ = (strHostIfSetMulti->cnt & 0xFF);
> *pu8CurrByte++ = ((strHostIfSetMulti->cnt >> 8) & 0xFF);
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox