Skip to contentSkip to conversation

d80211: reduce mdev usage, fix ieee80211_rx_mgmt

PATCH David Kimdon 3 participants 4 messages Nov 19, 2006 – Nov 20, 2006

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

David Kimdon

author
#0 lore raw

Reply-To:
In-Reply-To: <[email protected]>

Quoted 6 lines
net/d80211/ieee80211.c +0 -0
@@ -210,9 +210,16 @@ static void ieee80211_key_threshold_noti
 					   struct ieee80211_key *key,
 					   struct sta_info *sta)

net/d80211/ieee80211.c static void ieee80211_key_threshold_noti · line 221

Quoted 10 lines
net/d80211/ieee80211.c +6 -0
 {
+	struct ieee80211_local *local = dev->ieee80211_ptr;
 	struct sk_buff *skb;
 	struct ieee80211_msg_key_notification *msg;
 
+	/* if no one will get it anyway, don't even allocate it.
+	 * unlikely because this is only relevant for APs
+	 * where the device must be open... */
+	if (unlikely(!local->apdev))
+		return;

Why not just let it Oops and show the bug? In what cases is it ok for apdev to not be set?

net/d80211/ieee80211.c static void ieee80211_key_threshold_noti · line 224

Quoted 3 lines
net/d80211/ieee80211.c +0 -0
 	skb = dev_alloc_skb(sizeof(struct ieee80211_frame_info) +
 			    sizeof(struct ieee80211_msg_key_notification));
 	if (!skb)

Can we get whitespace changes like this in a separate patch?

Quoted 28 lines
net/d80211/ieee80211.c +11 -11
-                jiffies_to_timespec(status->hosttime, &ts);
+		jiffies_to_timespec(status->hosttime, &ts);
 		fi->hosttime = cpu_to_be64((u64) ts.tv_sec * 1000000 +
 					   ts.tv_nsec / 1000);
 		fi->mactime = cpu_to_be64(status->mactime);
 		switch (status->phymode) {
-                case MODE_IEEE80211A:
-                        fi->phytype = htonl(ieee80211_phytype_ofdm_dot11_a);
-                        break;
-                case MODE_IEEE80211B:
-                        fi->phytype = htonl(ieee80211_phytype_dsss_dot11_b);
-                        break;
-                case MODE_IEEE80211G:
-                        fi->phytype = htonl(ieee80211_phytype_pbcc_dot11_g);
-                        break;
-                case MODE_ATHEROS_TURBO:
+		case MODE_IEEE80211A:
+			fi->phytype = htonl(ieee80211_phytype_ofdm_dot11_a);
+			break;
+		case MODE_IEEE80211B:
+			fi->phytype = htonl(ieee80211_phytype_dsss_dot11_b);
+			break;
+		case MODE_IEEE80211G:
+			fi->phytype = htonl(ieee80211_phytype_pbcc_dot11_g);
+			break;
+		case MODE_ATHEROS_TURBO:
 			fi->phytype =

net/d80211/ieee80211.c static void ieee80211_key_threshold_noti

Quoted 10 lines
net/d80211/ieee80211.c +3 -3
 				htonl(ieee80211_phytype_dsss_dot11_turbo);
                         break;
                 default:
-                        fi->phytype = 0xAAAAAAAA;
+			fi->phytype = 0xAAAAAAAA;
 			break;
-                }
-                fi->channel = htonl(status->channel);
+		}
+		fi->channel = htonl(status->channel);

. . .

net/d80211/ieee80211.c

Quoted 11 lines

int ieee80211_set_aid_for_sta(struct net_device *dev, u8 *peer_address,

net/d80211/ieee80211.c +5 -0
 			      u16 aid)
 {
+	struct ieee80211_local *local = dev->ieee80211_ptr;
 	struct sk_buff *skb;
 	struct ieee80211_msg_set_aid_for_sta *msg;
 
+	/* unlikely because if this event only happens for APs,
+	 * which require an open ap device. */
+	if (unlikely(!local->apdev))
+		return 0;

again, so should we just let the Oops happen?

-David

#1 lore raw
Quoted 11 lines

Review

Quoted 6 lines
(quoted diff) +5 -0
+	/* if no one will get it anyway, don't even allocate it.
+	 * unlikely because this is only relevant for APs
+	 * where the device must be open... */
+	if (unlikely(!local->apdev))
+		return;

Why not just let it Oops and show the bug? In what cases is it ok for apdev to not be set?

Can we get whitespace changes like this in a separate patch?

Sure. I'll just remove it from this patch for now.

Quoted 1 line

again, so should we just let the Oops happen?

I'm thinking there might be race conditions where userspace does something, then drops the ap device but after that some operations complete and would lead to the oops too. Wasn't really sure.

johannes

#2 lore raw
Quoted 3 lines · Johannes Berg

I'm thinking there might be race conditions where userspace does something, then drops the ap device but after that some operations complete and would lead to the oops too. Wasn't really sure.

Yes, that's possible. But your check doesn't solve it.

Anyway, it's not worse than today, so ACK.

Thanks,

Jiri

Signature · 2 lines
Jiri Benc
SUSE Labs
#3 lore raw
Quoted 6 lines · Jiri Benc
Quoted 3 lines · Johannes Berg

I'm thinking there might be race conditions where userspace does something, then drops the ap device but after that some operations complete and would lead to the oops too. Wasn't really sure.

Yes, that's possible. But your check doesn't solve it.

Oh. Right, there are no locks involved :)

We want to get rid of the management interface so I guess we shouldn't try to fix the locking with this since we get it for free when we correctly implement it in cfg80211.

johannes