Re: [PATCH 4/10] d80211: reduce mdev usage, fix ieee80211_rx_mgmt

Re: [PATCH 4/10] d80211: reduce mdev usage, fix ieee80211_rx_mgmt

[David Kimdon]

Reply-To: 
In-Reply-To: <1163802439.3392.47.camel@johannes.berg>


> 
> --- wireless-dev.orig/net/d80211/ieee80211.c	2006-11-17 20:01:54.999703408 +0100
> +++ wireless-dev/net/d80211/ieee80211.c	2006-11-17 20:01:55.659703408 +0100
> @@ -210,9 +210,16 @@ static void ieee80211_key_threshold_noti
>  					   struct ieee80211_key *key,
>  					   struct sta_info *sta)
>  {
> +	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?

>  	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?

>  
> -                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 =
>  				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);


. . . 


>  int ieee80211_set_aid_for_sta(struct net_device *dev, u8 *peer_address,
>  			      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

Re: [PATCH 4/10] d80211: reduce mdev usage, fix ieee80211_rx_mgmt

[Johannes Berg]

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


> > +	/* 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.

> 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

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

Re: [PATCH 4/10] d80211: reduce mdev usage, fix ieee80211_rx_mgmt

[Jiri Benc]

On Sun, 19 Nov 2006 17:37:56 +0100, Johannes Berg wrote:
> 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

-- 
Jiri Benc
SUSE Labs

Re: [PATCH 4/10] d80211: reduce mdev usage, fix ieee80211_rx_mgmt

[Johannes Berg]

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

On Mon, 2006-11-20 at 18:44 +0100, Jiri Benc wrote:
> On Sun, 19 Nov 2006 17:37:56 +0100, Johannes Berg wrote:
> > 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

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