[PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Helmut Schaa]

Fix a possible NULL pointer dereference in ieee80211_compatible_rates
introduced in the patch "mac80211: fix association with some APs". If no bss
is available just use all supported rates in the association request.

Signed-off-by: Helmut Schaa <hschaa@suse.de>
---

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 76ad4ed..3f7f92a 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device *dev,
  			capab |= WLAN_CAPABILITY_PRIVACY;
  		if (bss->wmm_ie)
  			wmm = 1;
+
+		/* get all rates supported by the device and the AP as
+		 * some APs don't like getting a superset of their rates
+		 * in the association request (e.g. D-Link DAP 1353 in
+		 * b-only mode) */
+		rates_len = ieee80211_compatible_rates(bss, sband, &rates);
+
  		ieee80211_rx_bss_put(dev, bss);
+	} else {
+		rates = ~0;
+		rates_len = sband->n_bitrates;
  	}

  	mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
@@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device *dev,
  	*pos++ = ifsta->ssid_len;
  	memcpy(pos, ifsta->ssid, ifsta->ssid_len);

-	/* all supported rates should be added here but some APs
-	 * (e.g. D-Link DAP 1353 in b-only mode) don't like that
-	 * Therefore only add rates the AP supports */
-	rates_len = ieee80211_compatible_rates(bss, sband, &rates);
+	/* add all rates which were marked to be used above */
  	supp_rates_len = rates_len;
  	if (supp_rates_len > 8)
  		supp_rates_len = 8;

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Tue, May 20, 2008 at 10:56 AM, Helmut Schaa <hschaa@suse.de> wrote:
> Fix a possible NULL pointer dereference in ieee80211_compatible_rates
> introduced in the patch "mac80211: fix association with some APs". If no bss
> is available just use all supported rates in the association request.
>
> Signed-off-by: Helmut Schaa <hschaa@suse.de>
> ---
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 76ad4ed..3f7f92a 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device
> *dev,
>                        capab |= WLAN_CAPABILITY_PRIVACY;
>                if (bss->wmm_ie)
>                        wmm = 1;
> +
> +               /* get all rates supported by the device and the AP as
> +                * some APs don't like getting a superset of their rates
> +                * in the association request (e.g. D-Link DAP 1353 in
> +                * b-only mode) */
> +               rates_len = ieee80211_compatible_rates(bss, sband, &rates);
> +
>                ieee80211_rx_bss_put(dev, bss);
> +       } else {
> +               rates = ~0;
> +               rates_len = sband->n_bitrates;
>        }
>
>        mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
> @@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device
> *dev,
>        *pos++ = ifsta->ssid_len;
>        memcpy(pos, ifsta->ssid, ifsta->ssid_len);
>
> -       /* all supported rates should be added here but some APs
> -        * (e.g. D-Link DAP 1353 in b-only mode) don't like that
> -        * Therefore only add rates the AP supports */
> -       rates_len = ieee80211_compatible_rates(bss, sband, &rates);
> +       /* add all rates which were marked to be used above */
>        supp_rates_len = rates_len;
>        if (supp_rates_len > 8)
>                supp_rates_len = 8;
>
>

I found one  ieee80211_rx_bss_{get,put} imbalance   in
ieee80211_sta_join_ibss function
That may cause this problem yet it doesn't look like this is the case.
ieee80211_sta_join_ibss
calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
Keep searching.

I suggest to insert at least some WARN_ON(1) for the else case.

Tomas

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Johannes Berg]

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

On Tue, 2008-05-20 at 15:54 +0300, Tomas Winkler wrote:
> On Tue, May 20, 2008 at 10:56 AM, Helmut Schaa <hschaa@suse.de> wrote:
> > Fix a possible NULL pointer dereference in ieee80211_compatible_rates
> > introduced in the patch "mac80211: fix association with some APs". If no bss
> > is available just use all supported rates in the association request.
> >
> > Signed-off-by: Helmut Schaa <hschaa@suse.de>
> > ---
> >
> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> > index 76ad4ed..3f7f92a 100644
> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device
> > *dev,
> >                        capab |= WLAN_CAPABILITY_PRIVACY;
> >                if (bss->wmm_ie)
> >                        wmm = 1;
> > +
> > +               /* get all rates supported by the device and the AP as
> > +                * some APs don't like getting a superset of their rates
> > +                * in the association request (e.g. D-Link DAP 1353 in
> > +                * b-only mode) */
> > +               rates_len = ieee80211_compatible_rates(bss, sband, &rates);
> > +
> >                ieee80211_rx_bss_put(dev, bss);
> > +       } else {
> > +               rates = ~0;
> > +               rates_len = sband->n_bitrates;
> >        }
> >
> >        mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
> > @@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device
> > *dev,
> >        *pos++ = ifsta->ssid_len;
> >        memcpy(pos, ifsta->ssid, ifsta->ssid_len);
> >
> > -       /* all supported rates should be added here but some APs
> > -        * (e.g. D-Link DAP 1353 in b-only mode) don't like that
> > -        * Therefore only add rates the AP supports */
> > -       rates_len = ieee80211_compatible_rates(bss, sband, &rates);
> > +       /* add all rates which were marked to be used above */
> >        supp_rates_len = rates_len;
> >        if (supp_rates_len > 8)
> >                supp_rates_len = 8;
> >
> >
> 
> I found one  ieee80211_rx_bss_{get,put} imbalance   in
> ieee80211_sta_join_ibss function
> That may cause this problem yet it doesn't look like this is the case.
> ieee80211_sta_join_ibss
> calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
> Keep searching.

Hm. Send a patch? :)

> I suggest to insert at least some WARN_ON(1) for the else case.

Disagree, not until somebody audits the code. We already know it can
happen and a WARN() won't help us track it down because it provides no
additional information (stack trace is useless)

johannes

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

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Tue, May 20, 2008 at 3:57 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2008-05-20 at 15:54 +0300, Tomas Winkler wrote:
>> On Tue, May 20, 2008 at 10:56 AM, Helmut Schaa <hschaa@suse.de> wrote:
>> > Fix a possible NULL pointer dereference in ieee80211_compatible_rates
>> > introduced in the patch "mac80211: fix association with some APs". If no bss
>> > is available just use all supported rates in the association request.
>> >
>> > Signed-off-by: Helmut Schaa <hschaa@suse.de>
>> > ---
>> >
>> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
>> > index 76ad4ed..3f7f92a 100644
>> > --- a/net/mac80211/mlme.c
>> > +++ b/net/mac80211/mlme.c
>> > @@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device
>> > *dev,
>> >                        capab |= WLAN_CAPABILITY_PRIVACY;
>> >                if (bss->wmm_ie)
>> >                        wmm = 1;
>> > +
>> > +               /* get all rates supported by the device and the AP as
>> > +                * some APs don't like getting a superset of their rates
>> > +                * in the association request (e.g. D-Link DAP 1353 in
>> > +                * b-only mode) */
>> > +               rates_len = ieee80211_compatible_rates(bss, sband, &rates);
>> > +
>> >                ieee80211_rx_bss_put(dev, bss);
>> > +       } else {
>> > +               rates = ~0;
>> > +               rates_len = sband->n_bitrates;
>> >        }
>> >
>> >        mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
>> > @@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device
>> > *dev,
>> >        *pos++ = ifsta->ssid_len;
>> >        memcpy(pos, ifsta->ssid, ifsta->ssid_len);
>> >
>> > -       /* all supported rates should be added here but some APs
>> > -        * (e.g. D-Link DAP 1353 in b-only mode) don't like that
>> > -        * Therefore only add rates the AP supports */
>> > -       rates_len = ieee80211_compatible_rates(bss, sband, &rates);
>> > +       /* add all rates which were marked to be used above */
>> >        supp_rates_len = rates_len;
>> >        if (supp_rates_len > 8)
>> >                supp_rates_len = 8;
>> >
>> >
>>
>> I found one  ieee80211_rx_bss_{get,put} imbalance   in
>> ieee80211_sta_join_ibss function
>> That may cause this problem yet it doesn't look like this is the case.
>> ieee80211_sta_join_ibss
>> calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
>> Keep searching.
>
> Hm. Send a patch? :)
>

Searching for the real problem ...

>> I suggest to insert at least some WARN_ON(1) for the else case.
>
> Disagree, not until somebody audits the code. We already know it can
> happen and a WARN() won't help us track it down because it provides no
> additional information (stack trace is useless)

What about printk(KERN_WARN ), The else statement actually means that
something wrong happened.


Tomas

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Johannes Berg]

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


> >> I suggest to insert at least some WARN_ON(1) for the else case.
> >
> > Disagree, not until somebody audits the code. We already know it can
> > happen and a WARN() won't help us track it down because it provides no
> > additional information (stack trace is useless)
> 
> What about printk(KERN_WARN ), The else statement actually means that
> something wrong happened.

Thing is, I'm not totally convinced it is wrong to the code while it may
or may not be wrong... I think this patch should go in first as it
actually fixes the oops, and then we can discuss the merits of adding a
warning there separately. Maybe after we look a bit at the code and try
to figure out whether it can still happen after that patch from
Abhijeet.

johannes

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

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Tue, May 20, 2008 at 4:22 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> >> I suggest to insert at least some WARN_ON(1) for the else case.
>> >
>> > Disagree, not until somebody audits the code. We already know it can
>> > happen and a WARN() won't help us track it down because it provides no
>> > additional information (stack trace is useless)
>>
>> What about printk(KERN_WARN ), The else statement actually means that
>> something wrong happened.
>
> Thing is, I'm not totally convinced it is wrong to the code while it may
> or may not be wrong...

Doesn't should be bss pinned int he bss list if you are associating to
it. If it's not there you don't have access to it's info It looks very
wrong to me.

I think this patch should go in first as it
> actually fixes the oops, and then we can discuss the merits of adding a
> warning there separately. Maybe after we look a bit at the code and try
> to figure out whether it can still happen after that patch from
> Abhijeet.

I'm not sure if this patch is complete without this warning. What is
in the else statement is a hack and it should be obvious.

Tomas

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Johannes Berg]

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


> > Thing is, I'm not totally convinced it is wrong to the code while it may
> > or may not be wrong...
> 
> Doesn't should be bss pinned int he bss list if you are associating to
> it. If it's not there you don't have access to it's info It looks very
> wrong to me.

Well, yes, it is a bit odd.

> > I think this patch should go in first as it
> > actually fixes the oops, and then we can discuss the merits of adding a
> > warning there separately. Maybe after we look a bit at the code and try
> > to figure out whether it can still happen after that patch from
> > Abhijeet.
> 
> I'm not sure if this patch is complete without this warning. What is
> in the else statement is a hack and it should be obvious.

Considering that the message won't help us at all, why bother? We know
it's triggering, we know this might be a problem, and we know we can
only solve it by auditing the code. So why add a message that will get
us countless emails/complaints from people we cannot do anything about
anyway without doing the audit?

johannes

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

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Tue, May 20, 2008 at 4:38 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> > Thing is, I'm not totally convinced it is wrong to the code while it may
>> > or may not be wrong...
>>
>> Doesn't should be bss pinned int he bss list if you are associating to
>> it. If it's not there you don't have access to it's info It looks very
>> wrong to me.
>
> Well, yes, it is a bit odd.
>
>> > I think this patch should go in first as it
>> > actually fixes the oops, and then we can discuss the merits of adding a
>> > warning there separately. Maybe after we look a bit at the code and try
>> > to figure out whether it can still happen after that patch from
>> > Abhijeet.
>>
>> I'm not sure if this patch is complete without this warning. What is
>> in the else statement is a hack and it should be obvious.
>
> Considering that the message won't help us at all, why bother? We know
> it's triggering, we know this might be a problem, and we know we can
> only solve it by auditing the code. So why add a message that will get
> us countless emails/complaints from people we cannot do anything about
> anyway without doing the audit?


As I understand it's not easily reproducible so you need reference
point in the trace
when it happens. I'm not sure what your debug techniques are, though.

Tomas



> johannes
>

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Larry Finger]

Johannes Berg wrote:
>>> Thing is, I'm not totally convinced it is wrong to the code while it may
>>> or may not be wrong...
>> Doesn't should be bss pinned int he bss list if you are associating to
>> it. If it's not there you don't have access to it's info It looks very
>> wrong to me.
> 
> Well, yes, it is a bit odd.
> 
>>> I think this patch should go in first as it
>>> actually fixes the oops, and then we can discuss the merits of adding a
>>> warning there separately. Maybe after we look a bit at the code and try
>>> to figure out whether it can still happen after that patch from
>>> Abhijeet.
>> I'm not sure if this patch is complete without this warning. What is
>> in the else statement is a hack and it should be obvious.
> 
> Considering that the message won't help us at all, why bother? We know
> it's triggering, we know this might be a problem, and we know we can
> only solve it by auditing the code. So why add a message that will get
> us countless emails/complaints from people we cannot do anything about
> anyway without doing the audit?

This argument could go on endlessly; however, it is clear that we need to 
settle on a patch and get it upstream ASAP! Now that mainline is broken, the 
urgency is _MUCH_ greater.

Larry

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[John W. Linville]

On Tue, May 20, 2008 at 08:47:13AM -0500, Larry Finger wrote:
> Johannes Berg wrote:
>>>> Thing is, I'm not totally convinced it is wrong to the code while it may
>>>> or may not be wrong...
>>> Doesn't should be bss pinned int he bss list if you are associating to
>>> it. If it's not there you don't have access to it's info It looks very
>>> wrong to me.
>>
>> Well, yes, it is a bit odd.
>>
>>>> I think this patch should go in first as it
>>>> actually fixes the oops, and then we can discuss the merits of adding a
>>>> warning there separately. Maybe after we look a bit at the code and try
>>>> to figure out whether it can still happen after that patch from
>>>> Abhijeet.
>>> I'm not sure if this patch is complete without this warning. What is
>>> in the else statement is a hack and it should be obvious.
>>
>> Considering that the message won't help us at all, why bother? We know
>> it's triggering, we know this might be a problem, and we know we can
>> only solve it by auditing the code. So why add a message that will get
>> us countless emails/complaints from people we cannot do anything about
>> anyway without doing the audit?
>
> This argument could go on endlessly; however, it is clear that we need to 
> settle on a patch and get it upstream ASAP! Now that mainline is broken, 
> the urgency is _MUCH_ greater.

I agree.  I'll take this one unless someone finds a problem with it.

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Tue, May 20, 2008 at 4:47 PM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> Johannes Berg wrote:
>>>>
>>>> Thing is, I'm not totally convinced it is wrong to the code while it may
>>>> or may not be wrong...
>>>
>>> Doesn't should be bss pinned int he bss list if you are associating to
>>> it. If it's not there you don't have access to it's info It looks very
>>> wrong to me.
>>
>> Well, yes, it is a bit odd.
>>
>>>> I think this patch should go in first as it
>>>> actually fixes the oops, and then we can discuss the merits of adding a
>>>> warning there separately. Maybe after we look a bit at the code and try
>>>> to figure out whether it can still happen after that patch from
>>>> Abhijeet.
>>>
>>> I'm not sure if this patch is complete without this warning. What is
>>> in the else statement is a hack and it should be obvious.
>>
>> Considering that the message won't help us at all, why bother? We know
>> it's triggering, we know this might be a problem, and we know we can
>> only solve it by auditing the code. So why add a message that will get
>> us countless emails/complaints from people we cannot do anything about
>> anyway without doing the audit?
>
> This argument could go on endlessly; however, it is clear that we need to
> settle on a patch and get it upstream ASAP! Now that mainline is broken, the
> urgency is _MUCH_ greater.
>

We've just arguing about warning message so I won't consider it blocking.
Tomas

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Tue, May 20, 2008 at 3:54 PM, Tomas Winkler <tomasw@gmail.com> wrote:
> On Tue, May 20, 2008 at 10:56 AM, Helmut Schaa <hschaa@suse.de> wrote:
>> Fix a possible NULL pointer dereference in ieee80211_compatible_rates
>> introduced in the patch "mac80211: fix association with some APs". If no bss
>> is available just use all supported rates in the association request.
>>
>> Signed-off-by: Helmut Schaa <hschaa@suse.de>
>> ---
>>
>> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
>> index 76ad4ed..3f7f92a 100644
>> --- a/net/mac80211/mlme.c
>> +++ b/net/mac80211/mlme.c
>> @@ -721,7 +721,17 @@ static void ieee80211_send_assoc(struct net_device
>> *dev,
>>                        capab |= WLAN_CAPABILITY_PRIVACY;
>>                if (bss->wmm_ie)
>>                        wmm = 1;
>> +
>> +               /* get all rates supported by the device and the AP as
>> +                * some APs don't like getting a superset of their rates
>> +                * in the association request (e.g. D-Link DAP 1353 in
>> +                * b-only mode) */
>> +               rates_len = ieee80211_compatible_rates(bss, sband, &rates);
>> +
>>                ieee80211_rx_bss_put(dev, bss);
>> +       } else {
>> +               rates = ~0;
>> +               rates_len = sband->n_bitrates;
>>        }
>>
>>        mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
>> @@ -752,10 +762,7 @@ static void ieee80211_send_assoc(struct net_device
>> *dev,
>>        *pos++ = ifsta->ssid_len;
>>        memcpy(pos, ifsta->ssid, ifsta->ssid_len);
>>
>> -       /* all supported rates should be added here but some APs
>> -        * (e.g. D-Link DAP 1353 in b-only mode) don't like that
>> -        * Therefore only add rates the AP supports */
>> -       rates_len = ieee80211_compatible_rates(bss, sband, &rates);
>> +       /* add all rates which were marked to be used above */
>>        supp_rates_len = rates_len;
>>        if (supp_rates_len > 8)
>>                supp_rates_len = 8;
>>
>>
>
> I found one  ieee80211_rx_bss_{get,put} imbalance   in
> ieee80211_sta_join_ibss function
> That may cause this problem yet it doesn't look like this is the case.
> ieee80211_sta_join_ibss
> calls ieee80211_rx_bss_put on 'bss' that it receives as an argument

The patch below introduced  _get/_put imbalance. ieee80211_rx_bss_info
_put bss back at the end. Other callers of the ieee80211_sta_join_ibss
function don't use put.
I will post a patch that takes out the _put out of
ieee80211_rx_bss_info, I think it's more readable.

commit 9d9bf77d16ba527f6f63846ca18cf20ae6e8d697
Author: Bruno Randolf <bruno@thinktube.com>
Date:   Mon Feb 18 11:21:36 2008 +0900

    mac80211: enable IBSS merging

Tomas

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[John W. Linville]

On Wed, May 21, 2008 at 01:47:04PM +0300, Tomas Winkler wrote:
> On Tue, May 20, 2008 at 3:54 PM, Tomas Winkler <tomasw@gmail.com> wrote:

> > I found one  ieee80211_rx_bss_{get,put} imbalance   in
> > ieee80211_sta_join_ibss function
> > That may cause this problem yet it doesn't look like this is the case.
> > ieee80211_sta_join_ibss
> > calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
> 
> The patch below introduced  _get/_put imbalance. ieee80211_rx_bss_info
> _put bss back at the end. Other callers of the ieee80211_sta_join_ibss
> function don't use put.
> I will post a patch that takes out the _put out of
> ieee80211_rx_bss_info, I think it's more readable.

Since you are doing _get and _add in ieee80211_rx_bss_info, it makes
sense to me to do _put at the end of it.  Perhaps we should remove
the _put from the end of ieee80211_sta_join_ibss and change it's
callers instead?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Wed, May 21, 2008 at 4:54 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Wed, May 21, 2008 at 01:47:04PM +0300, Tomas Winkler wrote:
>> On Tue, May 20, 2008 at 3:54 PM, Tomas Winkler <tomasw@gmail.com> wrote:
>
>> > I found one  ieee80211_rx_bss_{get,put} imbalance   in
>> > ieee80211_sta_join_ibss function
>> > That may cause this problem yet it doesn't look like this is the case.
>> > ieee80211_sta_join_ibss
>> > calls ieee80211_rx_bss_put on 'bss' that it receives as an argument
>>
>> The patch below introduced  _get/_put imbalance. ieee80211_rx_bss_info
>> _put bss back at the end. Other callers of the ieee80211_sta_join_ibss
>> function don't use put.
>> I will post a patch that takes out the _put out of
>> ieee80211_rx_bss_info, I think it's more readable.
>
> Since you are doing _get and _add in ieee80211_rx_bss_info, it makes
> sense to me to do _put at the end of it.  Perhaps we should remove
> the _put from the end of ieee80211_sta_join_ibss and change it's
> callers instead?

That what I meant I've just pasted wrong function name into the mail
(was lazy typing)

Maybe someone can answer this


static void ieee80211_rx_bss_put(struct net_device *dev,
                                 struct ieee80211_sta_bss *bss)
{
        struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);

        local_bh_disable();
        if (!atomic_dec_and_lock(&bss->users, &local->sta_bss_lock)) {
                local_bh_enable();
                return;
        }

---- don't we miss  local_bh_enable(); here or spin_unlock_bh takes
care of this ---


        __ieee80211_rx_bss_hash_del(dev, bss);
        list_del(&bss->list);
        spin_unlock_bh(&local->sta_bss_lock);
        ieee80211_rx_bss_free(bss);
}

Thanks
Tomas


> John
> --
> John W. Linville
> linville@tuxdriver.com
>

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Johannes Berg]

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


> static void ieee80211_rx_bss_put(struct net_device *dev,
>                                  struct ieee80211_sta_bss *bss)
> {
>         struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> 
>         local_bh_disable();
>         if (!atomic_dec_and_lock(&bss->users, &local->sta_bss_lock)) {
>                 local_bh_enable();
>                 return;
>         }
> 
> ---- don't we miss  local_bh_enable(); here or spin_unlock_bh takes
> care of this ---
> 
> 
>         __ieee80211_rx_bss_hash_del(dev, bss);
>         list_del(&bss->list);
>         spin_unlock_bh(&local->sta_bss_lock);

spin_unlock_bh takes care of it. The local_bh_disable() +
atomic_dec_and_lock() is like spin_lock_bh() + atomic_dec() just with
different atomicity guarantees.

johannes

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

Re: [PATCHv5] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Wed, May 21, 2008 at 6:08 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> static void ieee80211_rx_bss_put(struct net_device *dev,
>>                                  struct ieee80211_sta_bss *bss)
>> {
>>         struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
>>
>>         local_bh_disable();
>>         if (!atomic_dec_and_lock(&bss->users, &local->sta_bss_lock)) {
>>                 local_bh_enable();
>>                 return;
>>         }
>>
>> ---- don't we miss  local_bh_enable(); here or spin_unlock_bh takes
>> care of this ---
>>
>>
>>         __ieee80211_rx_bss_hash_del(dev, bss);
>>         list_del(&bss->list);
>>         spin_unlock_bh(&local->sta_bss_lock);
>
> spin_unlock_bh takes care of it. The local_bh_disable() +
> atomic_dec_and_lock() is like spin_lock_bh() + atomic_dec() just with
> different atomicity guarantees.
>
> johannes
>
OK, thanks for explanation
Tomas