[PATCH] wext: fix get_wireless_stats locking

[PATCH] wext: fix get_wireless_stats locking

[Johannes Berg]

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)

Re: [PATCH] wext: fix get_wireless_stats locking

[John W. Linville]

On Sun, May 10, 2009 at 01:00:17PM +0200, Johannes Berg wrote:
> 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>

Is it acceptable to hold rtnl between ->open and ->release?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] wext: fix get_wireless_stats locking

[Johannes Berg]

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

On Mon, 2009-05-11 at 08:54 -0400, John W. Linville wrote:
> On Sun, May 10, 2009 at 01:00:17PM +0200, Johannes Berg wrote:
> > 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>
> 
> Is it acceptable to hold rtnl between ->open and ->release?

Hmm. It worked for me, but now I'm having doubts. This stuff confuses
me. I'll check it out.

johannes

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

[PATCH v2] wext: fix get_wireless_stats locking

[Johannes Berg]

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>
---
v2: don't hold lock across open/release but rather start/stop

 net/wireless/wext.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

--- wireless-testing.orig/net/wireless/wext.c	2009-05-11 13:34:00.000000000 +0200
+++ wireless-testing/net/wireless/wext.c	2009-05-11 16:02:31.000000000 +0200
@@ -649,14 +649,28 @@ static int wireless_seq_show(struct seq_
 	return 0;
 }
 
+static void *wireless_dev_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(dev_base_lock)
+{
+	rtnl_lock();
+	return dev_seq_start(seq, pos);
+}
+
+static void wireless_dev_seq_stop(struct seq_file *seq, void *v)
+	__releases(dev_base_lock)
+{
+	dev_seq_stop(seq, v);
+	rtnl_unlock();
+}
+
 static const struct seq_operations wireless_seq_ops = {
-	.start = dev_seq_start,
+	.start = wireless_dev_seq_start,
 	.next  = dev_seq_next,
-	.stop  = dev_seq_stop,
+	.stop  = wireless_dev_seq_stop,
 	.show  = wireless_seq_show,
 };
 
-static int wireless_seq_open(struct inode *inode, struct file *file)
+static int seq_open_wireless(struct inode *inode, struct file *file)
 {
 	return seq_open_net(inode, file, &wireless_seq_ops,
 			    sizeof(struct seq_net_private));
@@ -664,7 +678,7 @@ static int wireless_seq_open(struct inod
 
 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,