public inbox for linux-wireless@vger.kernel.org
 help / color / mirror / Atom feed
From: Johannes Berg <johannes@sipsolutions.net>
To: John Linville <linville@tuxdriver.com>
Cc: linux-wireless <linux-wireless@vger.kernel.org>
Subject: [PATCH] wext: fix get_wireless_stats locking
Date: Sun, 10 May 2009 13:00:17 +0200	[thread overview]
Message-ID: <1241953217.12068.0.camel@johannes.local> (raw)

Currently, get_wireless_stats is racy by _design_. This is
because it returns a buffer, which needs to be statically
allocated since it cannot be freed if it was allocated
dynamically. Also, SIOCGIWSTATS and /proc/net/wireless use
no common lock, and /proc/net/wireless accesses are not
synchronised against each other. This is a design flaw in
get_wireless_stats since the beginning.

This patch fixes it by wrapping /proc/net/wireless accesses
with the RTNL so they are protected against each other and
SIOCGIWSTATS. The more correct method of fixing this would
be to pass in the buffer instead of returning it and have
the caller take care of synchronisation of the buffer, but
even then most drivers probably assume that their callback
is protected by the RTNL like all other wext callbacks.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Kudos to Jean for the great wireless extensions design. NOT!

 net/wireless/wext.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

--- wireless-testing.orig/net/wireless/wext.c	2009-05-10 11:44:34.000000000 +0200
+++ wireless-testing/net/wireless/wext.c	2009-05-10 12:26:58.000000000 +0200
@@ -656,18 +656,32 @@ static const struct seq_operations wirel
 	.show  = wireless_seq_show,
 };
 
-static int wireless_seq_open(struct inode *inode, struct file *file)
+static int seq_open_wireless(struct inode *ino, struct file *f)
 {
-	return seq_open_net(inode, file, &wireless_seq_ops,
-			    sizeof(struct seq_net_private));
+	int err;
+
+	rtnl_lock();
+	err = seq_open_net(ino, f, &wireless_seq_ops,
+			   sizeof(struct seq_net_private));
+	if (err)
+		rtnl_unlock();
+
+	return 0;
+}
+
+static int seq_release_wireless(struct inode *ino, struct file *f)
+{
+	seq_release_net(ino, f);
+	rtnl_unlock();
+	return 0;
 }
 
 static const struct file_operations wireless_seq_fops = {
 	.owner	 = THIS_MODULE,
-	.open    = wireless_seq_open,
+	.open    = seq_open_wireless,
 	.read    = seq_read,
 	.llseek  = seq_lseek,
-	.release = seq_release_net,
+	.release = seq_release_wireless,
 };
 
 int wext_proc_init(struct net *net)



             reply	other threads:[~2009-05-10 11:00 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-05-10 11:00 Johannes Berg [this message]
2009-05-11 12:54 ` [PATCH] wext: fix get_wireless_stats locking John W. Linville
2009-05-11 13:56   ` Johannes Berg
2009-05-11 14:06   ` [PATCH v2] " Johannes Berg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1241953217.12068.0.camel@johannes.local \
    --to=johannes@sipsolutions.net \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linville@tuxdriver.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox