Linux wireless drivers development
 help / color / mirror / Atom feed
* [RFC PATCH] introduce WEXT scan capabilities
@ 2007-12-06 11:28 Dan Williams
  2007-12-06 19:11 ` Jean Tourrilhes
  0 siblings, 1 reply; 21+ messages in thread
From: Dan Williams @ 2007-12-06 11:28 UTC (permalink / raw)
  To: Jean Tourrilhes; +Cc: linux-wireless, Johannes Berg

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 <dcbw@redhat.com>


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;
 }
 


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-06 11:28 [RFC PATCH] introduce WEXT scan capabilities Dan Williams
@ 2007-12-06 19:11 ` Jean Tourrilhes
  2007-12-07 10:20   ` Dan Williams
  0 siblings, 1 reply; 21+ messages in thread
From: Jean Tourrilhes @ 2007-12-06 19:11 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-wireless, Johannes Berg

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 <jt@hpl.hp.com>

--- 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 */
 };
 
 /*

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-06 19:11 ` Jean Tourrilhes
@ 2007-12-07 10:20   ` Dan Williams
  2007-12-07 19:27     ` Jean Tourrilhes
  0 siblings, 1 reply; 21+ messages in thread
From: Dan Williams @ 2007-12-07 10:20 UTC (permalink / raw)
  To: jt; +Cc: linux-wireless, Johannes Berg

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 <jt@hpl.hp.com>
> 
> --- 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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-07 10:20   ` Dan Williams
@ 2007-12-07 19:27     ` Jean Tourrilhes
  2007-12-07 21:38       ` Johannes Berg
  2007-12-08  2:04       ` David Miller
  0 siblings, 2 replies; 21+ messages in thread
From: Jean Tourrilhes @ 2007-12-07 19:27 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-wireless, Johannes Berg

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 <jt@hpl.hp.com>
> > 
> > --- 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-07 19:27     ` Jean Tourrilhes
@ 2007-12-07 21:38       ` Johannes Berg
  2007-12-07 22:19         ` Jean Tourrilhes
  2007-12-08  2:04       ` David Miller
  1 sibling, 1 reply; 21+ messages in thread
From: Johannes Berg @ 2007-12-07 21:38 UTC (permalink / raw)
  To: jt; +Cc: Dan Williams, linux-wireless

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]


> > >         __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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-07 21:38       ` Johannes Berg
@ 2007-12-07 22:19         ` Jean Tourrilhes
  2007-12-07 22:27           ` Johannes Berg
  0 siblings, 1 reply; 21+ messages in thread
From: Jean Tourrilhes @ 2007-12-07 22:19 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Dan Williams, linux-wireless

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-07 22:19         ` Jean Tourrilhes
@ 2007-12-07 22:27           ` Johannes Berg
  0 siblings, 0 replies; 21+ messages in thread
From: Johannes Berg @ 2007-12-07 22:27 UTC (permalink / raw)
  To: jt; +Cc: Dan Williams, linux-wireless

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]


> > 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-07 19:27     ` Jean Tourrilhes
  2007-12-07 21:38       ` Johannes Berg
@ 2007-12-08  2:04       ` David Miller
  2007-12-09 17:35         ` Dan Williams
  1 sibling, 1 reply; 21+ messages in thread
From: David Miller @ 2007-12-08  2:04 UTC (permalink / raw)
  To: jt; +Cc: dcbw, linux-wireless, johannes

From: Jean Tourrilhes <jt@hpl.hp.com>
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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-08  2:04       ` David Miller
@ 2007-12-09 17:35         ` Dan Williams
  2007-12-09 17:59           ` Dan Williams
  2007-12-10 18:11           ` Jean Tourrilhes
  0 siblings, 2 replies; 21+ messages in thread
From: Dan Williams @ 2007-12-09 17:35 UTC (permalink / raw)
  To: David Miller; +Cc: jt, linux-wireless, johannes

On Fri, 2007-12-07 at 18:04 -0800, David Miller wrote:
> From: Jean Tourrilhes <jt@hpl.hp.com>
> 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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-09 17:35         ` Dan Williams
@ 2007-12-09 17:59           ` Dan Williams
  2007-12-10  6:10             ` David Miller
  2007-12-10 12:15             ` Johannes Berg
  2007-12-10 18:11           ` Jean Tourrilhes
  1 sibling, 2 replies; 21+ messages in thread
From: Dan Williams @ 2007-12-09 17:59 UTC (permalink / raw)
  To: David Miller; +Cc: jt, linux-wireless, johannes

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 <jt@hpl.hp.com>
> > 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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-09 17:59           ` Dan Williams
@ 2007-12-10  6:10             ` David Miller
  2007-12-10 17:23               ` Dan Williams
  2007-12-10 12:15             ` Johannes Berg
  1 sibling, 1 reply; 21+ messages in thread
From: David Miller @ 2007-12-10  6:10 UTC (permalink / raw)
  To: dcbw; +Cc: jt, linux-wireless, johannes

From: Dan Williams <dcbw@redhat.com>
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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-09 17:59           ` Dan Williams
  2007-12-10  6:10             ` David Miller
@ 2007-12-10 12:15             ` Johannes Berg
  2007-12-10 18:09               ` Jean Tourrilhes
  1 sibling, 1 reply; 21+ messages in thread
From: Johannes Berg @ 2007-12-10 12:15 UTC (permalink / raw)
  To: Dan Williams; +Cc: David Miller, jt, linux-wireless

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]


> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-10  6:10             ` David Miller
@ 2007-12-10 17:23               ` Dan Williams
  2007-12-11  0:11                 ` David Miller
  0 siblings, 1 reply; 21+ messages in thread
From: Dan Williams @ 2007-12-10 17:23 UTC (permalink / raw)
  To: David Miller; +Cc: jt, linux-wireless, johannes

On Sun, 2007-12-09 at 22:10 -0800, David Miller wrote:
> From: Dan Williams <dcbw@redhat.com>
> 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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-10 12:15             ` Johannes Berg
@ 2007-12-10 18:09               ` Jean Tourrilhes
  2007-12-11  0:15                 ` David Miller
  2007-12-11 13:21                 ` Johannes Berg
  0 siblings, 2 replies; 21+ messages in thread
From: Jean Tourrilhes @ 2007-12-10 18:09 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Dan Williams, David Miller, linux-wireless

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-09 17:35         ` Dan Williams
  2007-12-09 17:59           ` Dan Williams
@ 2007-12-10 18:11           ` Jean Tourrilhes
  1 sibling, 0 replies; 21+ messages in thread
From: Jean Tourrilhes @ 2007-12-10 18:11 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-wireless

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-10 17:23               ` Dan Williams
@ 2007-12-11  0:11                 ` David Miller
  2007-12-11  4:22                   ` Dan Williams
  0 siblings, 1 reply; 21+ messages in thread
From: David Miller @ 2007-12-11  0:11 UTC (permalink / raw)
  To: dcbw; +Cc: jt, linux-wireless, johannes

From: Dan Williams <dcbw@redhat.com>
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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-10 18:09               ` Jean Tourrilhes
@ 2007-12-11  0:15                 ` David Miller
  2007-12-11 13:21                 ` Johannes Berg
  1 sibling, 0 replies; 21+ messages in thread
From: David Miller @ 2007-12-11  0:15 UTC (permalink / raw)
  To: jt; +Cc: johannes, dcbw, linux-wireless

From: Jean Tourrilhes <jt@hpl.hp.com>
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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-11  0:11                 ` David Miller
@ 2007-12-11  4:22                   ` Dan Williams
  2007-12-11  4:51                     ` David Miller
  0 siblings, 1 reply; 21+ messages in thread
From: Dan Williams @ 2007-12-11  4:22 UTC (permalink / raw)
  To: David Miller; +Cc: jt, linux-wireless, johannes

On Mon, 2007-12-10 at 16:11 -0800, David Miller wrote:
> From: Dan Williams <dcbw@redhat.com>
> 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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-11  4:22                   ` Dan Williams
@ 2007-12-11  4:51                     ` David Miller
  2007-12-11 15:01                       ` Dan Williams
  0 siblings, 1 reply; 21+ messages in thread
From: David Miller @ 2007-12-11  4:51 UTC (permalink / raw)
  To: dcbw; +Cc: jt, linux-wireless, johannes

From: Dan Williams <dcbw@redhat.com>
Date: Mon, 10 Dec 2007 23:22:10 -0500

> On Mon, 2007-12-10 at 16:11 -0800, David Miller wrote:
> > From: Dan Williams <dcbw@redhat.com>
> > 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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-10 18:09               ` Jean Tourrilhes
  2007-12-11  0:15                 ` David Miller
@ 2007-12-11 13:21                 ` Johannes Berg
  1 sibling, 0 replies; 21+ messages in thread
From: Johannes Berg @ 2007-12-11 13:21 UTC (permalink / raw)
  To: jt; +Cc: Dan Williams, David Miller, linux-wireless

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]


> > 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RFC PATCH] introduce WEXT scan capabilities
  2007-12-11  4:51                     ` David Miller
@ 2007-12-11 15:01                       ` Dan Williams
  0 siblings, 0 replies; 21+ messages in thread
From: Dan Williams @ 2007-12-11 15:01 UTC (permalink / raw)
  To: David Miller; +Cc: jt, linux-wireless, johannes

On Mon, 2007-12-10 at 20:51 -0800, David Miller wrote:
> From: Dan Williams <dcbw@redhat.com>
> Date: Mon, 10 Dec 2007 23:22:10 -0500
> 
> > On Mon, 2007-12-10 at 16:11 -0800, David Miller wrote:
> > > From: Dan Williams <dcbw@redhat.com>
> > > 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



^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2007-12-11 15:11 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-12-06 11:28 [RFC PATCH] introduce WEXT scan capabilities Dan Williams
2007-12-06 19:11 ` Jean Tourrilhes
2007-12-07 10:20   ` Dan Williams
2007-12-07 19:27     ` Jean Tourrilhes
2007-12-07 21:38       ` Johannes Berg
2007-12-07 22:19         ` Jean Tourrilhes
2007-12-07 22:27           ` Johannes Berg
2007-12-08  2:04       ` David Miller
2007-12-09 17:35         ` Dan Williams
2007-12-09 17:59           ` Dan Williams
2007-12-10  6:10             ` David Miller
2007-12-10 17:23               ` Dan Williams
2007-12-11  0:11                 ` David Miller
2007-12-11  4:22                   ` Dan Williams
2007-12-11  4:51                     ` David Miller
2007-12-11 15:01                       ` Dan Williams
2007-12-10 12:15             ` Johannes Berg
2007-12-10 18:09               ` Jean Tourrilhes
2007-12-11  0:15                 ` David Miller
2007-12-11 13:21                 ` Johannes Berg
2007-12-10 18:11           ` Jean Tourrilhes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox