When trying to use the NEW_STATION command, it fails with -EINVAL,
because there's no STA_SUPPORTED_RATES parameter. However, for mesh
mode, this parameter should be NULL. Besides, it requires
LISTEN_INTERVAL to be negative. We allow it to be ommited in the STA
case too, in which it will be -1.
---
Hello.
I was trying to use iw station set in mesh mode and found out I wanted
to call NEW_STATION. I've quickly added a station new command to iw. I
am experimenting with mesh mode with a mac80211 driver. Then, I
realized nl80211_new_station requires STA_SUPPORTED_RATES to be set and,
then, requires it not to be set for mesh mode. I've patched it to
require STA_SUPPORTED_RATES only for AP mode. But now, I get a warning
on rate_lowest_index (from include/net/mac80211.h) and kernel panics
right after that. I've done it on top of latest rc and I am currently
building a version on top of wireless-testing/master.
I've not got any kdump yet, but here is the oops and the patch for iw
too. I've not signed-off the nl80211 patch yet because I want some
feedback and remove this panic.
static int handle_plink_new(struct nl80211_state *state,
struct nl_cb *cb,
struct nl_msg *msg,
int argc, char **argv)
{
unsigned char mac_addr[ETH_ALEN];
if (argc < 1)
return 1;
if (mac_addr_a2n(mac_addr, argv[0])) {
fprintf(stderr, "invalid mac address\n");
return 2;
}
NLA_PUT(msg, NL80211_ATTR_MAC, ETH_ALEN, mac_addr);
return 0;
nla_put_failure:
return -ENOBUFS;
}
COMMAND(station, new, "<MAC address>",
NL80211_CMD_NEW_STATION, 0, CIB_NETDEV, handle_plink_new,
"Add new plink.");
Feb 4 20:30:56 vespa kernel: [ 368.489565] ------------[ cut here ]------------
Feb 4 20:30:56 vespa kernel: [ 368.489835] WARNING: at /home/cascardo/linux-2.6/include/net/mac80211.h:2317 minstrel_rate_init+0x63/0x2be [mac80211]()
Feb 4 20:30:56 vespa kernel: [ 368.490886] Hardware name: 2658J3U
Feb 4 20:30:56 vespa kernel: [ 368.491255] Modules linked in: ath5k mac80211 ath cfg80211 radeon ttm drm_kms_helper drm i2c_algo_bit sco bridge stp llc bnep rfcomm l2cap crc16 bluetooth nsc_ircc ipv6 arc4 ecb snd_intel8x0 snd_intel8x0m snd_ac97_codec thinkpad_acpi ac97_bus rfkill snd_pcm snd_seq snd_timer snd_seq_device pcmcia mousedev snd led_class soundcore video 8250_pnp irda rtc_cmos processor yenta_socket psmouse crc_ccitt snd_page_alloc rtc_core 8250 button battery ac nvram parport_pc rsrc_nonstatic serio_raw i2c_i801 rtc_lib serial_core parport pcspkr pcmcia_core output evdev i2c_core ext3 jbd mbcache dm_mod sg sr_mod cdrom sd_mod crc_t10dif ata_generic uhci_hcd ata_piix libata ehci_hcd e100 intel_agp mii scsi_mod usbcore nls_base agpgart thermal fan thermal_sys hwmon unix [last unloaded: cfg80211]
Feb 4 20:30:56 vespa kernel: [ 368.504638] Pid: 1493, comm: iw Not tainted 2.6.33-rc6-00007-g1c67e56a #11
Feb 4 20:30:56 vespa kernel: [ 368.504874] Call Trace:
Feb 4 20:30:56 vespa kernel: [ 368.505676] [<c102d882>] warn_slowpath_common+0x6a/0x81
Feb 4 20:30:56 vespa kernel: [ 368.514861] [<f86f1182>] ? minstrel_rate_init+0x63/0x2be [mac80211]
Feb 4 20:30:56 vespa kernel: [ 368.524272] [<c102d8ab>] warn_slowpath_null+0x12/0x15
Feb 4 20:30:56 vespa kernel: [ 368.533846] [<f86f1182>] minstrel_rate_init+0x63/0x2be [mac80211]
Feb 4 20:30:56 vespa kernel: [ 368.543567] [<c10333f7>] ? local_bh_enable+0x10/0x12
Feb 4 20:30:56 vespa kernel: [ 368.553209] [<f86ccd69>] ? sta_info_alloc+0x150/0x15d [mac80211]
Feb 4 20:30:56 vespa kernel: [ 368.562871] [<f86d8ef5>] ieee80211_add_station+0xe3/0x125 [mac80211]
Feb 4 20:30:56 vespa kernel: [ 368.572431] [<f86d8e12>] ? ieee80211_add_station+0x0/0x125 [mac80211]
Feb 4 20:30:56 vespa kernel: [ 368.582076] [<f814230f>] nl80211_new_station+0x164/0x1a2 [cfg80211]
Feb 4 20:30:56 vespa kernel: [ 368.591780] [<c11eab3c>] genl_rcv_msg+0x1a7/0x1c4
Feb 4 20:30:56 vespa kernel: [ 368.601294] [<c11ea995>] ? genl_rcv_msg+0x0/0x1c4
Feb 4 20:30:56 vespa kernel: [ 368.610600] [<c11e9c8d>] netlink_rcv_skb+0x35/0x7b
Feb 4 20:30:56 vespa kernel: [ 368.619710] [<c11ea98e>] genl_rcv+0x20/0x27
Feb 4 20:30:56 vespa kernel: [ 368.628538] [<c11e9abc>] netlink_unicast+0xeb/0x148
Feb 4 20:30:56 vespa kernel: [ 368.637372] [<c11ea1b8>] netlink_sendmsg+0x216/0x223
Feb 4 20:30:56 vespa kernel: [ 368.646265] [<c10c9392>] ? path_to_nameidata+0x16/0x2f
Feb 4 20:30:56 vespa kernel: [ 368.655292] [<c11c4a85>] __sock_sendmsg+0x4a/0x53
Feb 4 20:30:56 vespa kernel: [ 368.664485] [<c11c4d42>] sock_sendmsg+0x97/0xab
Feb 4 20:30:56 vespa kernel: [ 368.673372] [<c10f85e3>] ? proc_alloc_inode+0x1b/0x6a
Feb 4 20:30:56 vespa kernel: [ 368.682160] [<c1113d8b>] ? security_inode_alloc+0x1e/0x20
Feb 4 20:30:56 vespa kernel: [ 368.690878] [<c12368d7>] ? add_preempt_count+0x9f/0xa2
Feb 4 20:30:56 vespa kernel: [ 368.699426] [<c123682b>] ? sub_preempt_count+0x84/0x91
Feb 4 20:30:56 vespa kernel: [ 368.707818] [<c101e0d4>] ? kunmap_atomic+0x86/0x9d
Feb 4 20:30:56 vespa kernel: [ 368.716232] [<c11cd165>] ? copy_from_user+0xd/0xf
Feb 4 20:30:56 vespa kernel: [ 368.724662] [<c11cd478>] ? verify_iovec+0x43/0x6f
Feb 4 20:30:56 vespa kernel: [ 368.732805] [<c11c4f79>] sys_sendmsg+0x18c/0x1f0
Feb 4 20:30:56 vespa kernel: [ 368.740813] [<c11d4bf6>] ? dev_ioctl+0x524/0x577
Feb 4 20:30:56 vespa kernel: [ 368.748870] [<c12367b0>] ? sub_preempt_count+0x9/0x91
Feb 4 20:30:56 vespa kernel: [ 368.756850] [<c123682b>] ? sub_preempt_count+0x84/0x91
Feb 4 20:30:56 vespa kernel: [ 368.764613] [<c123682b>] ? sub_preempt_count+0x84/0x91
Feb 4 20:30:56 vespa kernel: [ 368.772215] [<c1094678>] ? unlock_page+0x42/0x45
Feb 4 20:30:56 vespa kernel: [ 368.779676] [<c10a6d93>] ? __do_fault+0x31d/0x34d
Feb 4 20:30:56 vespa kernel: [ 368.787145] [<c10a869d>] ? handle_mm_fault+0x380/0x7d7
Feb 4 20:30:56 vespa kernel: [ 368.794377] [<c10d5731>] ? mntput_no_expire+0x1e/0x8c
Feb 4 20:30:56 vespa kernel: [ 368.801584] [<c11c6a87>] sys_socketcall+0x163/0x1aa
Feb 4 20:30:56 vespa kernel: [ 368.808628] [<c113d5f4>] ? trace_hardirqs_on_thunk+0xc/0x10
Feb 4 20:30:56 vespa kernel: [ 368.815669] [<c100329f>] sysenter_do_call+0x12/0x2d
Feb 4 20:30:56 vespa kernel: [ 368.822608] ---[ end trace 5ab7f913a209c320 ]---
---
net/wireless/nl80211.c | 24 +++++++++++++-----------
1 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5b79ecf..43e0137 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2000,23 +2000,23 @@ static int nl80211_new_station(struct sk_buff *skb, struct genl_info *info)
u8 *mac_addr = NULL;
memset(¶ms, 0, sizeof(params));
+ params.listen_interval = -1;
if (!info->attrs[NL80211_ATTR_MAC])
return -EINVAL;
- if (!info->attrs[NL80211_ATTR_STA_LISTEN_INTERVAL])
- return -EINVAL;
+ mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
- if (!info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES])
- return -EINVAL;
+ if (info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]) {
+ params.supported_rates =
+ nla_data(info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]);
+ params.supported_rates_len =
+ nla_len(info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]);
+ }
- mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
- params.supported_rates =
- nla_data(info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]);
- params.supported_rates_len =
- nla_len(info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]);
- params.listen_interval =
- nla_get_u16(info->attrs[NL80211_ATTR_STA_LISTEN_INTERVAL]);
+ if (info->attrs[NL80211_ATTR_STA_LISTEN_INTERVAL])
+ params.listen_interval =
+ nla_get_u16(info->attrs[NL80211_ATTR_STA_LISTEN_INTERVAL]);
if (info->attrs[NL80211_ATTR_STA_AID]) {
params.aid = nla_get_u16(info->attrs[NL80211_ATTR_STA_AID]);
@@ -2050,6 +2050,8 @@ static int nl80211_new_station(struct sk_buff *skb, struct genl_info *info)
/* all ok but must have AID */
if (!params.aid)
err = -EINVAL;
+ if (!params.supported_rates)
+ err = -EINVAL;
break;
case NL80211_IFTYPE_MESH_POINT:
/* disallow things mesh doesn't support */
--
1.6.6.1
Am Freitag, 5. Februar 2010 18:55:56 schrieb Thadeu Lima de Souza Cascardo:
> I did not get automatic plinks for a peer.
The last time that happened I was using different checkouts of wireless-testing
on both nodes and somebody had changed the code to match a new draft version
in between.
And the one time before that there was a bug in the ath5k code, effectively
preventing the device from beaconing.
> I want automatic plinks to work. But since I've started to play with it,
> why not make the command just work. It's there. If adding plinks
> manually to mesh nodes is not permitted, I may just send another patch,
> disallowing it entirely and making it explicit in code, comment and git
> log.
If I'm not mistaken the draft standard says that every node has to actively or
passively scan for neighbors and automatically select candidate peers. A
candidate peer has to announce a profile (Mesh Information Element) in its
beacons that matches the local profile.
If manual addition of plinks is allowed you could theoretically force your
node to peer with a neighbor running a different profile, e.g. using a different
path selection or authentication algorithm, or an implementation based on a
different draft version.
I can't really think of an use for it.
Just my 0.01$.
regards
Simon Raffeiner
On Fri, Feb 5, 2010 at 11:04 AM, Simon Raffeiner
<[email protected]> wrote:
> If manual addition of plinks is allowed you could theoretically force your
> node to peer with a neighbor running a different profile, e.g. using a different
> path selection or authentication algorithm, or an implementation based on a
> different draft version.
>
> I can't really think of an use for it.
Right, in your case please ensure that your devices are beaconing
(that's the most common problem) before proceeding. plinks are
established automatically and as far as I know that's the only thing
we need to support at this time. If your device isn't beaconing, then
that's a bug and it should be fixed. That said, ath5k beaconed
correctly last time I tried it.
-Andrey
Simon,
On Mon, Feb 8, 2010 at 5:54 AM, Simon Raffeiner
<[email protected]> wrote:
> Am Sonntag 07 Februar 2010 08:51:03 pm schrieben Sie:
>
>> It was supported for automated testing and experimentation (you could
>> then force certain topologies).
>
> Wouldn't one have to disable beaconing then, or at least automatic peering? IMO you can't force a topology if the nodes are still beaconing and establishing paths at the same time.
Automatic peering can be disabled via NL80211_MESHCONF_AUTO_OPEN_PLINKS.
Javier
--
Javier Cardona
cozybit Inc.
http://www.cozybit.com
On Thu, Feb 04, 2010 at 11:00:42PM -0200, Thadeu Lima de Souza Cascardo wrote:
> Hello.
>
> I was trying to use iw station set in mesh mode and found out I wanted
> to call NEW_STATION. I've quickly added a station new command to iw. I
> am experimenting with mesh mode with a mac80211 driver. Then, I
> realized nl80211_new_station requires STA_SUPPORTED_RATES to be set and,
> then, requires it not to be set for mesh mode. I've patched it to
> require STA_SUPPORTED_RATES only for AP mode. But now, I get a warning
> on rate_lowest_index (from include/net/mac80211.h) and kernel panics
> right after that. I've done it on top of latest rc and I am currently
> building a version on top of wireless-testing/master.
>
It warns and panics in wireless-testing/master too.
However, I did found out that it does not panic when using PID rate
control instead of Minstreal. The same warning still occurs, since both
minstreal and PID rate_init functions call rate_lowest_index, which is
the responsible inline function for the WARNING.
After doing the "station new" command, I can do a "station dump" and get
my newly added station. So, I think this is ready for commit. And then,
we need to sort out both the WARNING and the panic for the Minstrel in
other patches too.
I'll resend a signed-off patch soon. Just wait a little for Andrey's
feedback.
Regards,
Cascardo.
On Mon, 2010-02-08 at 18:26 -0200, Thadeu Lima de Souza Cascardo wrote:
> > After reviewing our mesh test suite, I would vote for disallowing mesh
> > point addition via iw. We currently force topologies by 1. turning
> > auto peering off, 2. waiting for neighbors to be discovered and 3.
> > controlling peering with 'iw ... station set <MAC address>
> > plink_action <open|block>'
> >
> > Thadeu, would this approach work for you?
Javier, thanks for taking a look.
> That's OK for me. Do you want a patch?
Please. Just remove the relevant cases from net/wireless/nl80211.c.
johannes
On Fri, Feb 05, 2010 at 09:47:44AM -0800, Andrey Yurovsky wrote:
> Hi,
>
> On Fri, Feb 5, 2010 at 8:57 AM, Thadeu Lima de Souza Cascardo
> <[email protected]> wrote:
> > After doing the "station new" command, I can do a "station dump" and get
> > my newly added station. So, I think this is ready for commit. And then,
> > we need to sort out both the WARNING and the panic for the Minstrel in
> > other patches too.
>
> Just to be clear, what are you trying to do here? Are you manually
> populating the peer links (stations) list? If so, why? Thanks,
>
> -Andrey
I did not get automatic plinks for a peer. So, I decided to look at the
code and see when they got added to the sta list. It does when it
receives some management frames and also when it's added manually with
the NEW_STATION nl80211 command.
I want automatic plinks to work. But since I've started to play with it,
why not make the command just work. It's there. If adding plinks
manually to mesh nodes is not permitted, I may just send another patch,
disallowing it entirely and making it explicit in code, comment and git
log.
Did you have anything in mind for the use of this command to mesh mode?
One use case I can think of is to set the block action before the node
appears nearby (since station set will not work without the plink).
Regards,
Cascardo.
On Thu, 2010-02-04 at 23:00 -0200, Thadeu Lima de Souza Cascardo wrote:
> When trying to use the NEW_STATION command, it fails with -EINVAL,
> because there's no STA_SUPPORTED_RATES parameter. However, for mesh
> mode, this parameter should be NULL. Besides, it requires
> LISTEN_INTERVAL to be negative. We allow it to be ommited in the STA
> case too, in which it will be -1.
>
> ---
>
> Hello.
>
> I was trying to use iw station set in mesh mode and found out I wanted
> to call NEW_STATION.
Hmm. Can you explain why you want to do that? Mesh usually discovers its
peers, and you can add paths, but peers doesn't make a whole lot of
sense to me?
johannes
Am Sonntag 07 Februar 2010 08:51:03 pm schrieben Sie:
> It was supported for automated testing and experimentation (you could
> then force certain topologies).
Wouldn't one have to disable beaconing then, or at least automatic peering? IMO you can't force a topology if the nodes are still beaconing and establishing paths at the same time.
regards
Simon Raffeiner
On Mon, Feb 08, 2010 at 11:25:37AM -0800, Javier Cardona wrote:
> Thadeu, Johannes,
>
> On Mon, Feb 8, 2010 at 1:08 AM, Johannes Berg <[email protected]> wrote:
> > On Sun, 2010-02-07 at 11:51 -0800, Andrey Yurovsky wrote:
> >> On Sat, Feb 6, 2010 at 12:59 AM, Johannes Berg
> >> <[email protected]> wrote:
> >> > I do wonder though, the commit (155cc9e4b1d60161ee53) I referenced was
> >> > yours, and now you're asking the same question as I am -- "why even
> >> > allow adding stations?"
> >>
> >> It was supported for automated testing and experimentation (you could
> >> then force certain topologies). I can't think of any other reason to
> >> do it. I suppose that the same thing goes for mpaths and the mesh
> >> portal tables.
> >
> > So would you prefer to fix it (however we would do that, I don't think
> > that the patch here is correct since it leaves the station struct with
> > no usable rates afaict), or just disallow it?
>
> After reviewing our mesh test suite, I would vote for disallowing mesh
> point addition via iw. We currently force topologies by 1. turning
> auto peering off, 2. waiting for neighbors to be discovered and 3.
> controlling peering with 'iw ... station set <MAC address>
> plink_action <open|block>'
>
> Thadeu, would this approach work for you?
>
That's OK for me. Do you want a patch?
> Javier
>
>
>
> --
> Javier Cardona
> cozybit Inc.
> http://www.cozybit.com
On Sat, Feb 6, 2010 at 12:59 AM, Johannes Berg
<[email protected]> wrote:
> I do wonder though, the commit (155cc9e4b1d60161ee53) I referenced was
> yours, and now you're asking the same question as I am -- "why even
> allow adding stations?"
It was supported for automated testing and experimentation (you could
then force certain topologies). I can't think of any other reason to
do it. I suppose that the same thing goes for mpaths and the mesh
portal tables.
-Andrey
Thadeu, Johannes,
On Mon, Feb 8, 2010 at 1:08 AM, Johannes Berg <[email protected]> wrote:
> On Sun, 2010-02-07 at 11:51 -0800, Andrey Yurovsky wrote:
>> On Sat, Feb 6, 2010 at 12:59 AM, Johannes Berg
>> <[email protected]> wrote:
>> > I do wonder though, the commit (155cc9e4b1d60161ee53) I referenced was
>> > yours, and now you're asking the same question as I am -- "why even
>> > allow adding stations?"
>>
>> It was supported for automated testing and experimentation (you could
>> then force certain topologies). ?I can't think of any other reason to
>> do it. ?I suppose that the same thing goes for mpaths and the mesh
>> portal tables.
>
> So would you prefer to fix it (however we would do that, I don't think
> that the patch here is correct since it leaves the station struct with
> no usable rates afaict), or just disallow it?
After reviewing our mesh test suite, I would vote for disallowing mesh
point addition via iw. We currently force topologies by 1. turning
auto peering off, 2. waiting for neighbors to be discovered and 3.
controlling peering with 'iw ... station set <MAC address>
plink_action <open|block>'
Thadeu, would this approach work for you?
Javier
--
Javier Cardona
cozybit Inc.
http://www.cozybit.com
On Fri, 2010-02-05 at 10:39 +0100, Johannes Berg wrote:
> On Thu, 2010-02-04 at 23:00 -0200, Thadeu Lima de Souza Cascardo wrote:
> > When trying to use the NEW_STATION command, it fails with -EINVAL,
> > because there's no STA_SUPPORTED_RATES parameter. However, for mesh
> > mode, this parameter should be NULL. Besides, it requires
> > LISTEN_INTERVAL to be negative. We allow it to be ommited in the STA
> > case too, in which it will be -1.
> >
> > ---
> >
> > Hello.
> >
> > I was trying to use iw station set in mesh mode and found out I wanted
> > to call NEW_STATION.
>
> Hmm. Can you explain why you want to do that? Mesh usually discovers its
> peers, and you can add paths, but peers doesn't make a whole lot of
> sense to me?
So it's allowed due to commit 155cc9e4b1d60161ee53ffaf2c15b9411f086fa7,
but I don't understand why. Andrey?
johannes
On Sun, 2010-02-07 at 11:51 -0800, Andrey Yurovsky wrote:
> On Sat, Feb 6, 2010 at 12:59 AM, Johannes Berg
> <[email protected]> wrote:
> > I do wonder though, the commit (155cc9e4b1d60161ee53) I referenced was
> > yours, and now you're asking the same question as I am -- "why even
> > allow adding stations?"
>
> It was supported for automated testing and experimentation (you could
> then force certain topologies). I can't think of any other reason to
> do it. I suppose that the same thing goes for mpaths and the mesh
> portal tables.
So would you prefer to fix it (however we would do that, I don't think
that the patch here is correct since it leaves the station struct with
no usable rates afaict), or just disallow it?
johannes
On Fri, 2010-02-05 at 09:47 -0800, Andrey Yurovsky wrote:
> Hi,
>
> On Fri, Feb 5, 2010 at 8:57 AM, Thadeu Lima de Souza Cascardo
> <[email protected]> wrote:
> > After doing the "station new" command, I can do a "station dump" and get
> > my newly added station. So, I think this is ready for commit. And then,
> > we need to sort out both the WARNING and the panic for the Minstrel in
> > other patches too.
>
> Just to be clear, what are you trying to do here? Are you manually
> populating the peer links (stations) list? If so, why? Thanks,
Andrey, thanks for chiming in!
I do wonder though, the commit (155cc9e4b1d60161ee53) I referenced was
yours, and now you're asking the same question as I am -- "why even
allow adding stations?"
Should we just refuse it (again)? Clearly it would be safer than the
current code that's prone to crashes.
johannes
Hi,
On Fri, Feb 5, 2010 at 8:57 AM, Thadeu Lima de Souza Cascardo
<[email protected]> wrote:
> After doing the "station new" command, I can do a "station dump" and get
> my newly added station. So, I think this is ready for commit. And then,
> we need to sort out both the WARNING and the panic for the Minstrel in
> other patches too.
Just to be clear, what are you trying to do here? Are you manually
populating the peer links (stations) list? If so, why? Thanks,
-Andrey