introduce WEXT scan capabilities

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?

Quoted 26 lines · Dan Williams

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

Quoted 1 line

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

include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800 renamed from include/linux/wireless.d1.h 2007-12-06 11:04:16.000000000 -0800 +1 -0

@@ -1040,6 +1040,16 @@ struct   iw_range
         * because each entry contain its channel index */
 
        __u32           enc_capa;       /* IW_ENC_CAPA_* bit field */
+

Quoted 46 lines · Jean Tourrilhes

Quoted 26 lines · Dan Williams

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

Quoted 1 line

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

include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800 renamed from include/linux/wireless.d1.h 2007-12-06 11:04:16.000000000 -0800 +0 -0

@@ -1040,6 +1040,16 @@ struct   iw_range
         * because each entry contain its channel index */
 

include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800 struct iw_range · line 1053

Quoted 12 lines

include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800 renamed from include/linux/wireless.d1.h 2007-12-06 11:04:16.000000000 -0800 +10 -0

        __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

Quoted 12 lines

include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800 struct iw_range · line 1052

Quoted 12 lines

__u32 enc_capa; /* IW_ENC_CAPA_* bit field */

include/linux/wireless.h 2007-12-06 11:06:26.000000000 -0800 renamed from include/linux/wireless.d1.h 2007-12-06 11:04:16.000000000 -0800 +10 -0

+
+       /* 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

Quoted 2 lines

Eww. That's absolutely tasteless.

It was not my decision.

Quoted 1 line

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.

From: Jean Tourrilhes <[email protected]>
Date: Fri, 7 Dec 2007 11:27:56 -0800

Quoted 4 lines

Quoted 1 line

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.

Quoted 20 lines · David Miller

From: Jean Tourrilhes <[email protected]>
Date: Fri, 7 Dec 2007 11:27:56 -0800

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

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.

Do either of those sound better to you than extending struct iw_range?

From: Dan Williams <[email protected]>
Date: Sun, 09 Dec 2007 12:59:34 -0500

Quoted 2 lines

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.

Is this an unconditional NAK for any changes to WEXT?

From: Dan Williams <[email protected]>
Date: Mon, 10 Dec 2007 12:23:23 -0500

Quoted 1 line

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.

Quoted 5 lines

From: Dan Williams <[email protected]>
Date: Mon, 10 Dec 2007 12:23:23 -0500

Quoted 1 line

Is this an unconditional NAK for any changes to WEXT?

Quoted 6 lines

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?

From: Dan Williams <[email protected]>
Date: Mon, 10 Dec 2007 23:22:10 -0500

Quoted 13 lines · David Miller

Quoted 11 lines

From: Dan Williams <[email protected]>
Date: Mon, 10 Dec 2007 12:23:23 -0500

Quoted 1 line

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.

Quoted 1 line

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.

Do either of those sound better to you than extending struct iw_range?

Quoted 2 lines

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

Quoted 9 lines

Quoted 2 lines

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

Quoted 7 lines

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

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