See the previous thread about fixing the ap_scan mess. I think Jean's
correct in asserting that we need more bits for scan capability.
This patch introduces scan capability bits for WEXT; hopefully cfg80211
can also pick up equivalent functionality. Capability bits are provided
for all the current options that may be passed to drivers in the
iw_scan_req structure. It can be assumed that if the driver reports the
scan capability, that the driver respects the options specified in the
iw_scan_req structure when performing the scan.
Clients can use logic like (cribbed from wpa_supplicant's driver_wext.c)
this to figure out whether or not the capability bits are supported:
struct iwreq iwr;
struct iw_range *range;
<set up iwr/range for request>
if (ioctl(drv->ioctl_sock, SIOCGIWRANGE, &iwr) == 0) {
u8 minlen = ((char *) &range->scan_capa) - (char *) range + sizeof(range->scan_capa);
if (iwr.u.data.length >= minlen) {
/* SCAN_CAPA is supported */
}
}
Jean; what do you think?
Signed-off-by: Dan Williams <[email protected]>
diff --git a/drivers/net/wireless/ipw2200.c b/drivers/net/wireless/ipw2200.c
index 54f44e5..e30ad24 100644
--- a/drivers/net/wireless/ipw2200.c
+++ b/drivers/net/wireless/ipw2200.c
@@ -8901,6 +8901,8 @@ static int ipw_wx_get_range(struct net_device *dev,
range->enc_capa = IW_ENC_CAPA_WPA | IW_ENC_CAPA_WPA2 |
IW_ENC_CAPA_CIPHER_TKIP | IW_ENC_CAPA_CIPHER_CCMP;
+ range->scan_capa = IW_SCAN_CAPA_ESSID | IW_SCAN_CAPA_TYPE;
+
IPW_DEBUG_WX("GET Range\n");
return 0;
}
diff --git a/include/linux/wireless.h b/include/linux/wireless.h
index 0987aa7..9342801 100644
--- a/include/linux/wireless.h
+++ b/include/linux/wireless.h
@@ -541,6 +541,15 @@
/* Maximum size of returned data */
#define IW_SCAN_MAX_DATA 4096 /* In bytes */
+/* Scan capabilitiy macros - in (struct iw_range *)->scan_capa */
+#define IW_SCAN_CAPA_ESSID 0x00000001
+#define IW_SCAN_CAPA_BSSID 0x00000002
+#define IW_SCAN_CAPA_CHANNEL 0x00000004
+#define IW_SCAN_CAPA_MODE 0x00000008
+#define IW_SCAN_CAPA_RATE 0x00000010
+#define IW_SCAN_CAPA_TYPE 0x00000020
+#define IW_SCAN_CAPA_TIME 0x00000040
+
/* Max number of char in custom event - use multiple of them if needed */
#define IW_CUSTOM_MAX 256 /* In bytes */
@@ -1040,6 +1049,8 @@ struct iw_range
* because each entry contain its channel index */
__u32 enc_capa; /* IW_ENC_CAPA_* bit field */
+
+ __u32 scan_capa; /* IW_SCAN_CAPA_* bit field */
};
/*
diff --git a/net/mac80211/ieee80211_ioctl.c b/net/mac80211/ieee80211_ioctl.c
index 646e2f2..0c52ed8 100644
--- a/net/mac80211/ieee80211_ioctl.c
+++ b/net/mac80211/ieee80211_ioctl.c
@@ -218,6 +218,8 @@ static int ieee80211_ioctl_giwrange(struct net_device *dev,
IW_EVENT_CAPA_SET(range->event_capa, SIOCGIWAP);
IW_EVENT_CAPA_SET(range->event_capa, SIOCGIWSCAN);
+ range->scan_capa |= IW_SCAN_CAPA_ESSID;
+
return 0;
}
From: Dan Williams <[email protected]>
Date: Sun, 09 Dec 2007 12:59:34 -0500
> Do either of those sound better to you than extending struct
> iw_range?
I find it interesting that we're willing to invest so much into
new WEXT hacks, but zero effort in converting the same applications
over the nl80211.
Even if you did it only for this new functionality, it would be
100 times better investment of your time than continuing this
WEXT nightmare.
On Mon, 2007-12-10 at 20:51 -0800, David Miller wrote:
> From: Dan Williams <[email protected]>
> Date: Mon, 10 Dec 2007 23:22:10 -0500
>
> > On Mon, 2007-12-10 at 16:11 -0800, David Miller wrote:
> > > From: Dan Williams <[email protected]>
> > > Date: Mon, 10 Dec 2007 12:23:23 -0500
> > >
> > > > Is this an unconditional NAK for any changes to WEXT?
> > >
> > > Well, it is at least completely proven that you cannot
> > > extend the structures without crapping all over random
> > > areas of the stack space of the user application.
> > >
> > > So at a minimum you're going to get NAKs for any patch
> > > that does things that way.
> >
> > So would another WEXT subcommand be acceptable to you?
>
> I might find using the existing spare space bearable, but
> even that is a stretch.
>
> We need to completely deprecate WEXT as fast as possible
> and adding new features to it won't help that.
Yeah, but dropping WEXT on the floor is unacceptable when it is the only
thing that is in use today. We will certainly work to get
nl80211/cfg80211 in shape, but we must live with WEXT. I will post a
patch that uses the existing space inside the range structure.
Dan
On Sun, 2007-12-09 at 12:35 -0500, Dan Williams wrote:
> On Fri, 2007-12-07 at 18:04 -0800, David Miller wrote:
> > From: Jean Tourrilhes <[email protected]>
> > Date: Fri, 7 Dec 2007 11:27:56 -0800
> >
> > > > Can you explain a bit more about this patch?
> > >
> > > Those fields are defined and used in userspace, please check
> > > wireless.21.h in Wireless Tools.
> >
> > If you need auxiliary data in the userspace application, define
> > an auxiliary structure which references the thing you get back
> > from the kernel.
> >
> > I totally disagree with embedding things in kernel defined
> > interfaces that are purely userland internal data structures.
> >
> > You really need to clean up the way you handle the wireless
> > kernel APIs, it is getting worse not better and you really
> > do not use good judgment or good interface design practices
> > when you makes these changes.
> >
> > Everything is one big hack.
> >
> > I will seriously NACK wireless API changes like this until
> > the situation starts to improve.
>
> So how would _you_ add a scan capabilities bitfield (or a new generic
> capabilities bitfield) to the WEXT range call?
>
> We need the scan capability flag functionality; I don't care how we get
> it as long as the patch is not too invasive. But userspace needs to
> know what the driver can do, and the patch needs to be written so that
> drivers that don't have the capabilities don't need to be touched.
I'm happy to do the patch, just need to figure out how to do it within
the constraints in a way that will be acceptable.
- We could create another WEXT sub-ioctl (SIOCGIWCAPA ?) that would have
all the capabilities together in a nice structure or something, but that
was what GIWRANGE was supposed to be, and it would add yet another WEXT
sub-ioctl. Still, if nobody is opposed to this, it would work nicely
and would be a fresh start.
(We can't have SIOCSIWSCAN return an error if you pass it options it
doesn't know about, because that requires modifying _all_ drivers to
return that error. We need to ensure that drivers that don't support
the scan capabilities don't need changing, otherwise the field is
useless because you can't tell whether a driver from 2.6.23 has the
capability or not. It needs to be a _positive_ flag saying "yes I do
support this" instead of drivers from, say, 2.6.23 silently accepting
the command and not honoring the option)
- Overload struct iw_range's enc_capa like I had originally proposed,
perhaps changing the field's name to "capabilities" or something.
However, there are only 32 bits available and we would already be using
11 bits in the field. Not too many. Jean was against this though. But
if we changed the name/redefine it and maintain the current bit values
it would only break stuff that checks for enc_capa != 0, which nothing
should do because that check doesn't tell you anything at all.
Do either of those sound better to you than extending struct iw_range?
Dan
On Sun, 2007-12-09 at 22:10 -0800, David Miller wrote:
> From: Dan Williams <[email protected]>
> Date: Sun, 09 Dec 2007 12:59:34 -0500
>
> > Do either of those sound better to you than extending struct
> > iw_range?
>
> I find it interesting that we're willing to invest so much into
> new WEXT hacks, but zero effort in converting the same applications
> over the nl80211.
>
> Even if you did it only for this new functionality, it would be
> 100 times better investment of your time than continuing this
> WEXT nightmare.
Not everyone is on unreleased 2.6.25 kernels. We need to work in many
places, and we must use WEXT for quite a while yet. It's gonna need
maintenance. Therefore, we still have to fix bugs, and this is a fix
for a bug whereby hidden SSID handling is really, really crappy right
now.
Is this an unconditional NAK for any changes to WEXT? Again, I'm happy
to do the patch, what would be an acceptable way to fix this bug _in_
_WEXT_ where drivers do not advertise what their scan capabilities are?
Dan
> Do either of those sound better to you than extending struct iw_range?
Because wext is stupidly defined, you can never extend any structures it
uses. Wext never passes in the length that userspace expects to passing
in longer structures than the fixed one userspace expects will always
overwrite something in userspace, possibly on the stack.
johannes
From: Dan Williams <[email protected]>
Date: Mon, 10 Dec 2007 12:23:23 -0500
> Is this an unconditional NAK for any changes to WEXT?
Well, it is at least completely proven that you cannot
extend the structures without crapping all over random
areas of the stack space of the user application.
So at a minimum you're going to get NAKs for any patch
that does things that way.
From: Dan Williams <[email protected]>
Date: Mon, 10 Dec 2007 23:22:10 -0500
> On Mon, 2007-12-10 at 16:11 -0800, David Miller wrote:
> > From: Dan Williams <[email protected]>
> > Date: Mon, 10 Dec 2007 12:23:23 -0500
> >
> > > Is this an unconditional NAK for any changes to WEXT?
> >
> > Well, it is at least completely proven that you cannot
> > extend the structures without crapping all over random
> > areas of the stack space of the user application.
> >
> > So at a minimum you're going to get NAKs for any patch
> > that does things that way.
>
> So would another WEXT subcommand be acceptable to you?
I might find using the existing spare space bearable, but
even that is a stretch.
We need to completely deprecate WEXT as fast as possible
and adding new features to it won't help that.
On Mon, 2007-12-10 at 16:11 -0800, David Miller wrote:
> From: Dan Williams <[email protected]>
> Date: Mon, 10 Dec 2007 12:23:23 -0500
>
> > Is this an unconditional NAK for any changes to WEXT?
>
> Well, it is at least completely proven that you cannot
> extend the structures without crapping all over random
> areas of the stack space of the user application.
>
> So at a minimum you're going to get NAKs for any patch
> that does things that way.
So would another WEXT subcommand be acceptable to you?
Dan
On Mon, Dec 10, 2007 at 01:15:27PM +0100, Johannes Berg wrote:
>
> > Do either of those sound better to you than extending struct iw_range?
>
> Because wext is stupidly defined, you can never extend any structures it
> uses. Wext never passes in the length that userspace expects to passing
> in longer structures than the fixed one userspace expects will always
> overwrite something in userspace, possibly on the stack.
>
> johannes
Please check again...
Jean
On Fri, Dec 07, 2007 at 10:38:06PM +0100, Johannes Berg wrote:
>
> Eww. That's absolutely tasteless.
It was not my decision.
> johannes
Jean
> > > __u32 enc_capa; /* IW_ENC_CAPA_* bit field */
> > > +
> > > + /* Do *NOT* use those fields, they are just used as padding to get
> > > + * proper alignement with user space */
> > > + __s32 min_pms;
> > > + __s32 max_pms;
> > > + __u16 pms_flags;
> > > + __s32 modul_capa;
> > > + __u32 bitrate_capa;
> > > +
> > > + __u32 scan_capa; /* IW_SCAN_CAPA_* bit field */
> Those fields are defined and used in userspace, please check
> wireless.21.h in Wireless Tools.
Eww. That's absolutely tasteless.
johannes
> > Because wext is stupidly defined, you can never extend any structures it
> > uses. Wext never passes in the length that userspace expects to passing
> > in longer structures than the fixed one userspace expects will always
> > overwrite something in userspace, possibly on the stack.
> >
> > johannes
>
> Please check again...
I have. It's worse than I thought, there's a length parameter but it's
not used properly.
johannes
On Thu, 2007-12-06 at 11:11 -0800, Jean Tourrilhes wrote:
> On Thu, Dec 06, 2007 at 06:28:39AM -0500, Dan Williams wrote:
> > See the previous thread about fixing the ap_scan mess. I think Jean's
> > correct in asserting that we need more bits for scan capability.
> >
> > This patch introduces scan capability bits for WEXT; hopefully cfg80211
> > can also pick up equivalent functionality. Capability bits are provided
> > for all the current options that may be passed to drivers in the
> > iw_scan_req structure. It can be assumed that if the driver reports the
> > scan capability, that the driver respects the options specified in the
> > iw_scan_req structure when performing the scan.
> >
> > Clients can use logic like (cribbed from wpa_supplicant's driver_wext.c)
> > this to figure out whether or not the capability bits are supported:
> >
> > struct iwreq iwr;
> > struct iw_range *range;
> >
> > <set up iwr/range for request>
> >
> > if (ioctl(drv->ioctl_sock, SIOCGIWRANGE, &iwr) == 0) {
> > u8 minlen = ((char *) &range->scan_capa) - (char *) range + sizeof(range->scan_capa);
> >
> > if (iwr.u.data.length >= minlen) {
> > /* SCAN_CAPA is supported */
> > }
> > }
> >
> > Jean; what do you think?
>
> Actually, I like your new proposal. I told you there was a
> gotcha, you need to add some padding to not screw up userspace. Note
> that we have already some padding in that structure (look at 'old_*'),
> so it's not the first time we do that.
> Basically, it should look like the patch below...
>
> Regards,
>
> Jean
>
> Signed-off-by: Jean Tourrilhes <[email protected]>
>
> --- linux/include/linux/wireless.d1.h 2007-12-06 11:04:16.000000000 -0800
> +++ linux/include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800
> @@ -1040,6 +1040,16 @@ struct iw_range
> * because each entry contain its channel index */
>
> __u32 enc_capa; /* IW_ENC_CAPA_* bit field */
> +
> + /* Do *NOT* use those fields, they are just used as padding to get
> + * proper alignement with user space */
> + __s32 min_pms;
> + __s32 max_pms;
> + __u16 pms_flags;
> + __s32 modul_capa;
> + __u32 bitrate_capa;
> +
> + __u32 scan_capa; /* IW_SCAN_CAPA_* bit field */
> };
Can you explain a bit more about this patch? Also, if nobody is
supposed to use these fields, shouldn't their names be 'reserved' or
something like that?
Thanks,
Dan
On Fri, Dec 07, 2007 at 05:20:18AM -0500, Dan Williams wrote:
> On Thu, 2007-12-06 at 11:11 -0800, Jean Tourrilhes wrote:
> > On Thu, Dec 06, 2007 at 06:28:39AM -0500, Dan Williams wrote:
> > > See the previous thread about fixing the ap_scan mess. I think Jean's
> > > correct in asserting that we need more bits for scan capability.
> > >
> > > This patch introduces scan capability bits for WEXT; hopefully cfg80211
> > > can also pick up equivalent functionality. Capability bits are provided
> > > for all the current options that may be passed to drivers in the
> > > iw_scan_req structure. It can be assumed that if the driver reports the
> > > scan capability, that the driver respects the options specified in the
> > > iw_scan_req structure when performing the scan.
> > >
> > > Clients can use logic like (cribbed from wpa_supplicant's driver_wext.c)
> > > this to figure out whether or not the capability bits are supported:
> > >
> > > struct iwreq iwr;
> > > struct iw_range *range;
> > >
> > > <set up iwr/range for request>
> > >
> > > if (ioctl(drv->ioctl_sock, SIOCGIWRANGE, &iwr) == 0) {
> > > u8 minlen = ((char *) &range->scan_capa) - (char *) range + sizeof(range->scan_capa);
> > >
> > > if (iwr.u.data.length >= minlen) {
> > > /* SCAN_CAPA is supported */
> > > }
> > > }
> > >
> > > Jean; what do you think?
> >
> > Actually, I like your new proposal. I told you there was a
> > gotcha, you need to add some padding to not screw up userspace. Note
> > that we have already some padding in that structure (look at 'old_*'),
> > so it's not the first time we do that.
> > Basically, it should look like the patch below...
> >
> > Regards,
> >
> > Jean
> >
> > Signed-off-by: Jean Tourrilhes <[email protected]>
> >
> > --- linux/include/linux/wireless.d1.h 2007-12-06 11:04:16.000000000 -0800
> > +++ linux/include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800
> > @@ -1040,6 +1040,16 @@ struct iw_range
> > * because each entry contain its channel index */
> >
> > __u32 enc_capa; /* IW_ENC_CAPA_* bit field */
> > +
> > + /* Do *NOT* use those fields, they are just used as padding to get
> > + * proper alignement with user space */
> > + __s32 min_pms;
> > + __s32 max_pms;
> > + __u16 pms_flags;
> > + __s32 modul_capa;
> > + __u32 bitrate_capa;
> > +
> > + __u32 scan_capa; /* IW_SCAN_CAPA_* bit field */
> > };
>
> Can you explain a bit more about this patch?
Those fields are defined and used in userspace, please check
wireless.21.h in Wireless Tools.
> Also, if nobody is
> supposed to use these fields, shouldn't their names be 'reserved' or
> something like that?
Yeah, you could do that. It does not matter eather way. Using
the same name as userspace has the advantage of making consistency
check easier.
> Thanks,
> Dan
Regards,
Jean
On Thu, Dec 06, 2007 at 06:28:39AM -0500, Dan Williams wrote:
> See the previous thread about fixing the ap_scan mess. I think Jean's
> correct in asserting that we need more bits for scan capability.
>
> This patch introduces scan capability bits for WEXT; hopefully cfg80211
> can also pick up equivalent functionality. Capability bits are provided
> for all the current options that may be passed to drivers in the
> iw_scan_req structure. It can be assumed that if the driver reports the
> scan capability, that the driver respects the options specified in the
> iw_scan_req structure when performing the scan.
>
> Clients can use logic like (cribbed from wpa_supplicant's driver_wext.c)
> this to figure out whether or not the capability bits are supported:
>
> struct iwreq iwr;
> struct iw_range *range;
>
> <set up iwr/range for request>
>
> if (ioctl(drv->ioctl_sock, SIOCGIWRANGE, &iwr) == 0) {
> u8 minlen = ((char *) &range->scan_capa) - (char *) range + sizeof(range->scan_capa);
>
> if (iwr.u.data.length >= minlen) {
> /* SCAN_CAPA is supported */
> }
> }
>
> Jean; what do you think?
Actually, I like your new proposal. I told you there was a
gotcha, you need to add some padding to not screw up userspace. Note
that we have already some padding in that structure (look at 'old_*'),
so it's not the first time we do that.
Basically, it should look like the patch below...
Regards,
Jean
Signed-off-by: Jean Tourrilhes <[email protected]>
--- linux/include/linux/wireless.d1.h 2007-12-06 11:04:16.000000000 -0800
+++ linux/include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800
@@ -1040,6 +1040,16 @@ struct iw_range
* because each entry contain its channel index */
__u32 enc_capa; /* IW_ENC_CAPA_* bit field */
+
+ /* Do *NOT* use those fields, they are just used as padding to get
+ * proper alignement with user space */
+ __s32 min_pms;
+ __s32 max_pms;
+ __u16 pms_flags;
+ __s32 modul_capa;
+ __u32 bitrate_capa;
+
+ __u32 scan_capa; /* IW_SCAN_CAPA_* bit field */
};
/*
On Fri, 2007-12-07 at 18:04 -0800, David Miller wrote:
> From: Jean Tourrilhes <[email protected]>
> Date: Fri, 7 Dec 2007 11:27:56 -0800
>
> > > Can you explain a bit more about this patch?
> >
> > Those fields are defined and used in userspace, please check
> > wireless.21.h in Wireless Tools.
>
> If you need auxiliary data in the userspace application, define
> an auxiliary structure which references the thing you get back
> from the kernel.
>
> I totally disagree with embedding things in kernel defined
> interfaces that are purely userland internal data structures.
>
> You really need to clean up the way you handle the wireless
> kernel APIs, it is getting worse not better and you really
> do not use good judgment or good interface design practices
> when you makes these changes.
>
> Everything is one big hack.
>
> I will seriously NACK wireless API changes like this until
> the situation starts to improve.
So how would _you_ add a scan capabilities bitfield (or a new generic
capabilities bitfield) to the WEXT range call?
We need the scan capability flag functionality; I don't care how we get
it as long as the patch is not too invasive. But userspace needs to
know what the driver can do, and the patch needs to be written so that
drivers that don't have the capabilities don't need to be touched.
Dan
> > Eww. That's absolutely tasteless.
>
> It was not my decision.
I think it was your decision to use the "same" structure in userspace as
kernel-space but not actually use the same. That's tasteless.
But we're stuck with it.
johannes
From: Jean Tourrilhes <[email protected]>
Date: Fri, 7 Dec 2007 11:27:56 -0800
> > Can you explain a bit more about this patch?
>
> Those fields are defined and used in userspace, please check
> wireless.21.h in Wireless Tools.
If you need auxiliary data in the userspace application, define
an auxiliary structure which references the thing you get back
from the kernel.
I totally disagree with embedding things in kernel defined
interfaces that are purely userland internal data structures.
You really need to clean up the way you handle the wireless
kernel APIs, it is getting worse not better and you really
do not use good judgment or good interface design practices
when you makes these changes.
Everything is one big hack.
I will seriously NACK wireless API changes like this until
the situation starts to improve.
From: Jean Tourrilhes <[email protected]>
Date: Mon, 10 Dec 2007 10:09:21 -0800
> On Mon, Dec 10, 2007 at 01:15:27PM +0100, Johannes Berg wrote:
> >
> > > Do either of those sound better to you than extending struct iw_range?
> >
> > Because wext is stupidly defined, you can never extend any structures it
> > uses. Wext never passes in the length that userspace expects to passing
> > in longer structures than the fixed one userspace expects will always
> > overwrite something in userspace, possibly on the stack.
> >
> > johannes
>
> Please check again...
I've personally already fixed a bug like this for 64-bit because the
WEXT request struct is smaller than an ifreq and the former is what
the applications declare on the stack yet an ifreq is what was used to
size to copy back into userspace.
There are therefore definitely past and potential future problems in
this area, and indeed it is a design issue.
On Sun, Dec 09, 2007 at 12:35:06PM -0500, Dan Williams wrote:
>
> So how would _you_ add a scan capabilities bitfield (or a new generic
> capabilities bitfield) to the WEXT range call?
>
> We need the scan capability flag functionality; I don't care how we get
> it as long as the patch is not too invasive. But userspace needs to
> know what the driver can do, and the patch needs to be written so that
> drivers that don't have the capabilities don't need to be touched.
Dan,
In my first e-mail, I offered you a way to do that without
having to change the API. You rejected it because it means fixing
various drivers. Well, there was a reason why I proposed it...
> Dan
Regards,
Jean