[PATCH] cfg80211: allow wext to remove keys that don't exist

[PATCH] cfg80211: allow wext to remove keys that don't exist

[Johannes Berg]

Some applications using wireless extensions expect to be able to
remove a key that doesn't exist. One example is wpa_supplicant
which doesn't actually change behaviour when running into an
error while trying to do that, but it prints an error message
which users interpret as wpa_supplicant having problems.

The safe thing to do is not change the behaviour of wireless
extensions any more, so when the driver reports -ENOENT let
the wext bridge code return success to userspace. To guarantee
this, also document that drivers should return -ENOENT when the
key doesn't exist.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 include/net/cfg80211.h     |    5 +++--
 net/wireless/wext-compat.c |    7 +++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

--- wireless-testing.orig/include/net/cfg80211.h	2009-05-18 19:49:55.000000000 +0200
+++ wireless-testing/include/net/cfg80211.h	2009-05-18 19:50:53.000000000 +0200
@@ -778,10 +778,11 @@ enum wiphy_params_flags {
  * @get_key: get information about the key with the given parameters.
  *	@mac_addr will be %NULL when requesting information for a group
  *	key. All pointers given to the @callback function need not be valid
- *	after it returns.
+ *	after it returns. This function should return an error if it is
+ *	not possible to retrieve the key, -ENOENT if it doesn't exist.
  *
  * @del_key: remove a key given the @mac_addr (%NULL for a group key)
- *	and @key_index
+ *	and @key_index, return -ENOENT if the key doesn't exist.
  *
  * @set_default_key: set the default key on an interface
  *
--- wireless-testing.orig/net/wireless/wext-compat.c	2009-05-18 19:48:29.000000000 +0200
+++ wireless-testing/net/wireless/wext-compat.c	2009-05-18 19:49:52.000000000 +0200
@@ -504,6 +504,13 @@ static int cfg80211_set_encryption(struc
 			else if (idx == wdev->wext.default_mgmt_key)
 				wdev->wext.default_mgmt_key = -1;
 		}
+		/*
+		 * Applications using wireless extensions expect to be
+		 * able to delete keys that don't exist, so allow that.
+		 */
+		if (err == -ENOENT)
+			return 0;
+
 		return err;
 	} else {
 		if (addr)

Re: [PATCH] cfg80211: allow wext to remove keys that don't exist

[Pavel Roskin]

On Mon, 2009-05-18 at 19:56 +0200, Johannes Berg wrote:
> Some applications using wireless extensions expect to be able to
> remove a key that doesn't exist. One example is wpa_supplicant
> which doesn't actually change behaviour when running into an
> error while trying to do that, but it prints an error message
> which users interpret as wpa_supplicant having problems.

It sounds like you are working around a userspace problem in the kernel.

> The safe thing to do is not change the behaviour of wireless
> extensions any more, so when the driver reports -ENOENT let
> the wext bridge code return success to userspace. To guarantee
> this, also document that drivers should return -ENOENT when the
> key doesn't exist.

You patch is changing the behavior or wireless extensions.  It would be
much more reasonable for wpa_supplicant not to remove non-existent keys
or (if it's unsafe or non-practical for some reason) not to report
-ENOENT to the user.

-- 
Regards,
Pavel Roskin

Re: [PATCH] cfg80211: allow wext to remove keys that don't exist

[Johannes Berg]

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

On Mon, 2009-05-18 at 16:04 -0400, Pavel Roskin wrote:
> On Mon, 2009-05-18 at 19:56 +0200, Johannes Berg wrote:
> > Some applications using wireless extensions expect to be able to
> > remove a key that doesn't exist. One example is wpa_supplicant
> > which doesn't actually change behaviour when running into an
> > error while trying to do that, but it prints an error message
> > which users interpret as wpa_supplicant having problems.
> 
> It sounds like you are working around a userspace problem in the kernel.

More a "user problem" rather than "userspace problem"...

> > The safe thing to do is not change the behaviour of wireless
> > extensions any more, so when the driver reports -ENOENT let
> > the wext bridge code return success to userspace. To guarantee
> > this, also document that drivers should return -ENOENT when the
> > key doesn't exist.
> 
> You patch is changing the behavior or wireless extensions.  It would be
> much more reasonable for wpa_supplicant not to remove non-existent keys
> or (if it's unsafe or non-practical for some reason) not to report
> -ENOENT to the user.

Umm, no... my previous patch changed the behaviour and this restores it.

johannes

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

Re: [PATCH] cfg80211: allow wext to remove keys that don't exist

[Pavel Roskin]

On Mon, 2009-05-18 at 22:06 +0200, Johannes Berg wrote:
> On Mon, 2009-05-18 at 16:04 -0400, Pavel Roskin wrote:
> > On Mon, 2009-05-18 at 19:56 +0200, Johannes Berg wrote:
> > > Some applications using wireless extensions expect to be able to
> > > remove a key that doesn't exist. One example is wpa_supplicant
> > > which doesn't actually change behaviour when running into an
> > > error while trying to do that, but it prints an error message
> > > which users interpret as wpa_supplicant having problems.
> > 
> > It sounds like you are working around a userspace problem in the kernel.
> 
> More a "user problem" rather than "userspace problem"...

Let's give users a break.  They cannot know which error messages are
important and which are not.  Users' attention should not be attracted
to trivial things such as failure to remove something that never
existed.

> > > The safe thing to do is not change the behaviour of wireless
> > > extensions any more, so when the driver reports -ENOENT let
> > > the wext bridge code return success to userspace. To guarantee
> > > this, also document that drivers should return -ENOENT when the
> > > key doesn't exist.
> > 
> > You patch is changing the behavior or wireless extensions.  It would be
> > much more reasonable for wpa_supplicant not to remove non-existent keys
> > or (if it's unsafe or non-practical for some reason) not to report
> > -ENOENT to the user.
> 
> Umm, no... my previous patch changed the behaviour and this restores it.

OK, that makes me more sympathetic to the patch, at least if the change
did not get exposed in any released kernel.

-- 
Regards,
Pavel Roskin