From 7be69c0b9aa93ef655db4d46e5654996489d62f5 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Mon, 11 May 2009 16:06:36 +0200 Subject: wext: fix get_wireless_stats locking 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 Signed-off-by: John W. Linville --- net/wireless/wext.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) (limited to 'net/wireless/wext.c') diff --git a/net/wireless/wext.c b/net/wireless/wext.c index cb6a5bb85d80..dc5b47e84a3d 100644 --- a/net/wireless/wext.c +++ b/net/wireless/wext.c @@ -649,14 +649,28 @@ static int wireless_seq_show(struct seq_file *seq, void *v) 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 inode *inode, struct file *file) 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, -- cgit v1.2.2 From f7eef3563cb3f05e7f0db992716c514af6f4d089 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Tue, 12 May 2009 08:36:43 +0200 Subject: wext: remove seq_start/stop sparse annotations Even though they are true, they cause sparse to complain because it doesn't see the __acquires(dev_base_lock) on dev_seq_start() because it is only added to the function in net/core/dev.c, not the header file. To keep track of the nesting correctly we should probably annotate those functions publically, but for now let's just remove the annotation I added to wext. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/wireless/wext.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'net/wireless/wext.c') diff --git a/net/wireless/wext.c b/net/wireless/wext.c index dc5b47e84a3d..d3bbef70cc7c 100644 --- a/net/wireless/wext.c +++ b/net/wireless/wext.c @@ -650,14 +650,12 @@ static int wireless_seq_show(struct seq_file *seq, void *v) } 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(); -- cgit v1.2.2 From 87057825824973f29cf2f37cff1e549170b2d7e6 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Tue, 19 May 2009 17:19:36 +0200 Subject: wext: remove atomic requirement for wireless stats The requirement for wireless stats to be atomic is now mostly artificial since we hold the rtnl _and_ the dev_base_lock for iterating the device list. Doing that is not required, just the rtnl is sufficient (and the rtnl is required for other reasons outlined in commit "wext: fix get_wireless_stats locking"). This will fix http://bugzilla.kernel.org/show_bug.cgi?id=13344 and make things easier for drivers. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/wireless/wext.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) (limited to 'net/wireless/wext.c') diff --git a/net/wireless/wext.c b/net/wireless/wext.c index d3bbef70cc7c..22378daceb94 100644 --- a/net/wireless/wext.c +++ b/net/wireless/wext.c @@ -636,8 +636,10 @@ static void wireless_seq_printf_stats(struct seq_file *seq, /* * Print info for /proc/net/wireless (print all entries) */ -static int wireless_seq_show(struct seq_file *seq, void *v) +static int wireless_dev_seq_show(struct seq_file *seq, void *v) { + might_sleep(); + if (v == SEQ_START_TOKEN) seq_printf(seq, "Inter-| sta-| Quality | Discarded " "packets | Missed | WE\n" @@ -651,21 +653,41 @@ static int wireless_seq_show(struct seq_file *seq, void *v) static void *wireless_dev_seq_start(struct seq_file *seq, loff_t *pos) { + struct net *net = seq_file_net(seq); + loff_t off; + struct net_device *dev; + rtnl_lock(); - return dev_seq_start(seq, pos); + if (!*pos) + return SEQ_START_TOKEN; + + off = 1; + for_each_netdev(net, dev) + if (off++ == *pos) + return dev; + return NULL; +} + +static void *wireless_dev_seq_next(struct seq_file *seq, void *v, loff_t *pos) +{ + struct net *net = seq_file_net(seq); + + ++*pos; + + return v == SEQ_START_TOKEN ? + first_net_device(net) : next_net_device(v); } static void wireless_dev_seq_stop(struct seq_file *seq, void *v) { - dev_seq_stop(seq, v); rtnl_unlock(); } static const struct seq_operations wireless_seq_ops = { .start = wireless_dev_seq_start, - .next = dev_seq_next, + .next = wireless_dev_seq_next, .stop = wireless_dev_seq_stop, - .show = wireless_seq_show, + .show = wireless_dev_seq_show, }; static int seq_open_wireless(struct inode *inode, struct file *file) -- cgit v1.2.2