* Re: [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference
@ 2013-12-05 8:41 ` Eugene Krasnikov
0 siblings, 0 replies; 27+ messages in thread
From: Eugene Krasnikov @ 2013-12-05 8:41 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
> + if (msg_ind)
> + break;
> + /* FIXME: Do something smarter then just printing an error. */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
I think code will look neater if error case will be handled in "else"
statement something like:
if (msg_ind) {
...
} else {
/*print message*/
}
/*unlock and break;*/
How about that?
On Mon, Dec 2, 2013 at 1:09 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
>
> If kmalloc fails wcn36xx_smd_rsp_process will attempt to dereference
> a NULL pointer. There might be a better error recovery then just
> printing an error, but printing an error message is better then the
> current behaviour.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> On Sun, Dec 01 2013, Peter Stuge wrote:
>> Michal Nazarewicz wrote:
>>> + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
>>> + msg_header->msg_type);
>>
>> Typo hnadling.
>
> Fixed.
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index de9eb2c..3663394 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2041,13 +2041,20 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (msg_ind) {
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + memcpy(msg_ind->msg, buf, len);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + }
> mutex_unlock(&wcn->hal_ind_mutex);
> + if (msg_ind)
> + break;
> + /* FIXME: Do something smarter then just printing an error. */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4.1
>
> --
> Best regards, _ _
> .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
> ..o | Computer Science, Michał “mina86” Nazarewicz (o o)
> ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
--
Best regards,
Eugene
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference
@ 2013-12-05 8:41 ` Eugene Krasnikov
0 siblings, 0 replies; 27+ messages in thread
From: Eugene Krasnikov @ 2013-12-05 8:41 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
> + if (msg_ind)
> + break;
> + /* FIXME: Do something smarter then just printing an error. */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
I think code will look neater if error case will be handled in "else"
statement something like:
if (msg_ind) {
...
} else {
/*print message*/
}
/*unlock and break;*/
How about that?
On Mon, Dec 2, 2013 at 1:09 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
>
> If kmalloc fails wcn36xx_smd_rsp_process will attempt to dereference
> a NULL pointer. There might be a better error recovery then just
> printing an error, but printing an error message is better then the
> current behaviour.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> On Sun, Dec 01 2013, Peter Stuge wrote:
>> Michal Nazarewicz wrote:
>>> + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
>>> + msg_header->msg_type);
>>
>> Typo hnadling.
>
> Fixed.
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index de9eb2c..3663394 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2041,13 +2041,20 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (msg_ind) {
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + memcpy(msg_ind->msg, buf, len);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + }
> mutex_unlock(&wcn->hal_ind_mutex);
> + if (msg_ind)
> + break;
> + /* FIXME: Do something smarter then just printing an error. */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4.1
>
> --
> Best regards, _ _
> .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
> ..o | Computer Science, Michał “mina86” Nazarewicz (o o)
> ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
--
Best regards,
Eugene
^ permalink raw reply [flat|nested] 27+ messages in thread* [ath9k-devel] [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference
2013-12-05 8:41 ` Eugene Krasnikov
@ 2013-12-06 16:12 ` John W. Linville
-1 siblings, 0 replies; 27+ messages in thread
From: John W. Linville @ 2013-12-06 16:12 UTC (permalink / raw)
To: ath9k-devel
FWIW, I merged the earlier version. Please send any changes as a
follow-up patch, probably against wireless-next.
John
On Thu, Dec 05, 2013 at 08:41:29AM +0000, Eugene Krasnikov wrote:
> > + if (msg_ind)
> > + break;
> > + /* FIXME: Do something smarter then just printing an error. */
> > + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
>
> I think code will look neater if error case will be handled in "else"
> statement something like:
> if (msg_ind) {
> ...
> } else {
> /*print message*/
> }
> /*unlock and break;*/
>
> How about that?
>
>
> On Mon, Dec 2, 2013 at 1:09 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> >
> > If kmalloc fails wcn36xx_smd_rsp_process will attempt to dereference
> > a NULL pointer. There might be a better error recovery then just
> > printing an error, but printing an error message is better then the
> > current behaviour.
> >
> > Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> > ---
> > drivers/net/wireless/ath/wcn36xx/smd.c | 19 +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > On Sun, Dec 01 2013, Peter Stuge wrote:
> >> Michal Nazarewicz wrote:
> >>> + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
> >>> + msg_header->msg_type);
> >>
> >> Typo hnadling.
> >
> > Fixed.
> >
> > diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> > index de9eb2c..3663394 100644
> > --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> > @@ -2041,13 +2041,20 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> > case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> > mutex_lock(&wcn->hal_ind_mutex);
> > msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> > - msg_ind->msg_len = len;
> > - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> > - memcpy(msg_ind->msg, buf, len);
> > - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> > - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> > - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> > + if (msg_ind) {
> > + msg_ind->msg_len = len;
> > + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> > + memcpy(msg_ind->msg, buf, len);
> > + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> > + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> > + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> > + }
> > mutex_unlock(&wcn->hal_ind_mutex);
> > + if (msg_ind)
> > + break;
> > + /* FIXME: Do something smarter then just printing an error. */
> > + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> > + msg_header->msg_type);
> > break;
> > default:
> > wcn36xx_err("SMD_EVENT (%d) not supported\n",
> > --
> > 1.8.4.1
> >
> > --
> > Best regards, _ _
> > .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
> > ..o | Computer Science, Micha? ?mina86? Nazarewicz (o o)
> > ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
>
>
>
> --
> Best regards,
> Eugene
>
--
John W. Linville Someday the world will need a hero, and you
linville at tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference
@ 2013-12-06 16:12 ` John W. Linville
0 siblings, 0 replies; 27+ messages in thread
From: John W. Linville @ 2013-12-06 16:12 UTC (permalink / raw)
To: Eugene Krasnikov
Cc: Michal Nazarewicz, Peter Stuge, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
FWIW, I merged the earlier version. Please send any changes as a
follow-up patch, probably against wireless-next.
John
On Thu, Dec 05, 2013 at 08:41:29AM +0000, Eugene Krasnikov wrote:
> > + if (msg_ind)
> > + break;
> > + /* FIXME: Do something smarter then just printing an error. */
> > + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
>
> I think code will look neater if error case will be handled in "else"
> statement something like:
> if (msg_ind) {
> ...
> } else {
> /*print message*/
> }
> /*unlock and break;*/
>
> How about that?
>
>
> On Mon, Dec 2, 2013 at 1:09 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> >
> > If kmalloc fails wcn36xx_smd_rsp_process will attempt to dereference
> > a NULL pointer. There might be a better error recovery then just
> > printing an error, but printing an error message is better then the
> > current behaviour.
> >
> > Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> > ---
> > drivers/net/wireless/ath/wcn36xx/smd.c | 19 +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > On Sun, Dec 01 2013, Peter Stuge wrote:
> >> Michal Nazarewicz wrote:
> >>> + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
> >>> + msg_header->msg_type);
> >>
> >> Typo hnadling.
> >
> > Fixed.
> >
> > diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> > index de9eb2c..3663394 100644
> > --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> > @@ -2041,13 +2041,20 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> > case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> > mutex_lock(&wcn->hal_ind_mutex);
> > msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> > - msg_ind->msg_len = len;
> > - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> > - memcpy(msg_ind->msg, buf, len);
> > - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> > - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> > - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> > + if (msg_ind) {
> > + msg_ind->msg_len = len;
> > + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> > + memcpy(msg_ind->msg, buf, len);
> > + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> > + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> > + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> > + }
> > mutex_unlock(&wcn->hal_ind_mutex);
> > + if (msg_ind)
> > + break;
> > + /* FIXME: Do something smarter then just printing an error. */
> > + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> > + msg_header->msg_type);
> > break;
> > default:
> > wcn36xx_err("SMD_EVENT (%d) not supported\n",
> > --
> > 1.8.4.1
> >
> > --
> > Best regards, _ _
> > .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
> > ..o | Computer Science, Michał “mina86” Nazarewicz (o o)
> > ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--
>
>
>
> --
> Best regards,
> Eugene
>
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [ath9k-devel] [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section
2013-12-05 8:41 ` Eugene Krasnikov
(?)
@ 2013-12-07 17:13 ` Michal Nazarewicz
-1 siblings, 0 replies; 27+ messages in thread
From: Michal Nazarewicz @ 2013-12-07 17:13 UTC (permalink / raw)
To: ath9k-devel
This also simplifies flow-controll as there is now only one if
condition with a single branch.
---
drivers/net/wireless/ath/wcn36xx/smd.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
On Thu, Dec 05 2013, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
> I think code will look neater if error case will be handled in "else"
> statement something like: [...]
How about the following:
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..ebab2db 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,24 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind) {
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ mutex_lock(&wcn->hal_ind_mutex);
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ memcpy(msg_ind->msg, buf, len);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section
@ 2013-12-07 17:13 ` Michal Nazarewicz
0 siblings, 0 replies; 27+ messages in thread
From: Michal Nazarewicz @ 2013-12-07 17:13 UTC (permalink / raw)
To: Eugene Krasnikov
Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
This also simplifies flow-controll as there is now only one if
condition with a single branch.
---
drivers/net/wireless/ath/wcn36xx/smd.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
On Thu, Dec 05 2013, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
> I think code will look neater if error case will be handled in "else"
> statement something like: [...]
How about the following:
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..ebab2db 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,24 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind) {
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ mutex_lock(&wcn->hal_ind_mutex);
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ memcpy(msg_ind->msg, buf, len);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section
@ 2013-12-07 17:13 ` Michal Nazarewicz
0 siblings, 0 replies; 27+ messages in thread
From: Michal Nazarewicz @ 2013-12-07 17:13 UTC (permalink / raw)
To: Eugene Krasnikov
Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
This also simplifies flow-controll as there is now only one if
condition with a single branch.
---
drivers/net/wireless/ath/wcn36xx/smd.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
On Thu, Dec 05 2013, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
> I think code will look neater if error case will be handled in "else"
> statement something like: [...]
How about the following:
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..ebab2db 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,24 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind) {
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ mutex_lock(&wcn->hal_ind_mutex);
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ memcpy(msg_ind->msg, buf, len);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [ath9k-devel] [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section
2013-12-07 17:13 ` Michal Nazarewicz
(?)
@ 2013-12-09 7:27 ` Eugene Krasnikov
-1 siblings, 0 replies; 27+ messages in thread
From: Eugene Krasnikov @ 2013-12-09 7:27 UTC (permalink / raw)
To: ath9k-devel
hal_ind_mutex suppose to protect msg_ind but with this patch
allocation will be done outside the critical section.
On Sat, Dec 7, 2013 at 5:13 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
>
> This also simplifies flow-controll as there is now only one if
> condition with a single branch.
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> On Thu, Dec 05 2013, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>> I think code will look neater if error case will be handled in "else"
>> statement something like: [...]
>
> How about the following:
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 823631c..ebab2db 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2056,22 +2056,24 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_OTA_TX_COMPL_IND:
> case WCN36XX_HAL_MISSED_BEACON_IND:
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - if (msg_ind) {
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (!msg_ind) {
> + /*
> + * FIXME: Do something smarter then just
> + * printing an error.
> + */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> + break;
> }
> + mutex_lock(&wcn->hal_ind_mutex);
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + memcpy(msg_ind->msg, buf, len);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> mutex_unlock(&wcn->hal_ind_mutex);
> - if (msg_ind)
> - break;
> - /* FIXME: Do something smarter then just printing an error. */
> - wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> - msg_header->msg_type);
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4
>
--
Best regards,
Eugene
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section
@ 2013-12-09 7:27 ` Eugene Krasnikov
0 siblings, 0 replies; 27+ messages in thread
From: Eugene Krasnikov @ 2013-12-09 7:27 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
hal_ind_mutex suppose to protect msg_ind but with this patch
allocation will be done outside the critical section.
On Sat, Dec 7, 2013 at 5:13 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
>
> This also simplifies flow-controll as there is now only one if
> condition with a single branch.
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> On Thu, Dec 05 2013, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>> I think code will look neater if error case will be handled in "else"
>> statement something like: [...]
>
> How about the following:
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 823631c..ebab2db 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2056,22 +2056,24 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_OTA_TX_COMPL_IND:
> case WCN36XX_HAL_MISSED_BEACON_IND:
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - if (msg_ind) {
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (!msg_ind) {
> + /*
> + * FIXME: Do something smarter then just
> + * printing an error.
> + */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> + break;
> }
> + mutex_lock(&wcn->hal_ind_mutex);
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + memcpy(msg_ind->msg, buf, len);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> mutex_unlock(&wcn->hal_ind_mutex);
> - if (msg_ind)
> - break;
> - /* FIXME: Do something smarter then just printing an error. */
> - wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> - msg_header->msg_type);
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4
>
--
Best regards,
Eugene
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section
@ 2013-12-09 7:27 ` Eugene Krasnikov
0 siblings, 0 replies; 27+ messages in thread
From: Eugene Krasnikov @ 2013-12-09 7:27 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
hal_ind_mutex suppose to protect msg_ind but with this patch
allocation will be done outside the critical section.
On Sat, Dec 7, 2013 at 5:13 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
>
> This also simplifies flow-controll as there is now only one if
> condition with a single branch.
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> On Thu, Dec 05 2013, Eugene Krasnikov <k.eugene.e@gmail.com> wrote:
>> I think code will look neater if error case will be handled in "else"
>> statement something like: [...]
>
> How about the following:
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 823631c..ebab2db 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2056,22 +2056,24 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_OTA_TX_COMPL_IND:
> case WCN36XX_HAL_MISSED_BEACON_IND:
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - if (msg_ind) {
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (!msg_ind) {
> + /*
> + * FIXME: Do something smarter then just
> + * printing an error.
> + */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> + break;
> }
> + mutex_lock(&wcn->hal_ind_mutex);
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + memcpy(msg_ind->msg, buf, len);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> mutex_unlock(&wcn->hal_ind_mutex);
> - if (msg_ind)
> - break;
> - /* FIXME: Do something smarter then just printing an error. */
> - wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> - msg_header->msg_type);
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4
>
--
Best regards,
Eugene
^ permalink raw reply [flat|nested] 27+ messages in thread* [ath9k-devel] [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section
2013-12-09 7:27 ` Eugene Krasnikov
(?)
@ 2013-12-09 11:34 ` Michal Nazarewicz
-1 siblings, 0 replies; 27+ messages in thread
From: Michal Nazarewicz @ 2013-12-09 11:34 UTC (permalink / raw)
To: ath9k-devel
Commit [3469adb3: fix potential NULL pointer dereference] introduced
a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
Moreover, it introduced two if statements, which looked a bit clunky.
This commit moves allocation code outside of the critical section so
there's no need to dance around mutex_unlock, and adds the missing
allocation check.
---
drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
On Mon, Dec 09 2013, Eugene Krasnikov wrote:
> hal_ind_mutex suppose to protect msg_ind but with this patch
> allocation will be done outside the critical section.
msg_ind is a local variable which is not shared outside of the function
until it's added to a list. I don't see how it matters if it's
allocation and initialisation is done under mutex.
In fact, there's another kmalloc call that should be checked and can be
done outside of the critical section, so disregard my previous patch,
here's a new one.
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..713e3ce 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind)
+ goto nomem;
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ if (!msg_ind->msg) {
+ kfree(msg_ind);
+nomem:
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ memcpy(msg_ind->msg, buf, len);
+ mutex_lock(&wcn->hal_ind_mutex);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
Url : http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20131209/eade3c6a/attachment.pgp
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section
@ 2013-12-09 11:34 ` Michal Nazarewicz
0 siblings, 0 replies; 27+ messages in thread
From: Michal Nazarewicz @ 2013-12-09 11:34 UTC (permalink / raw)
To: Eugene Krasnikov
Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2809 bytes --]
Commit [3469adb3: fix potential NULL pointer dereference] introduced
a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
Moreover, it introduced two if statements, which looked a bit clunky.
This commit moves allocation code outside of the critical section so
there's no need to dance around mutex_unlock, and adds the missing
allocation check.
---
drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
On Mon, Dec 09 2013, Eugene Krasnikov wrote:
> hal_ind_mutex suppose to protect msg_ind but with this patch
> allocation will be done outside the critical section.
msg_ind is a local variable which is not shared outside of the function
until it's added to a list. I don't see how it matters if it's
allocation and initialisation is done under mutex.
In fact, there's another kmalloc call that should be checked and can be
done outside of the critical section, so disregard my previous patch,
here's a new one.
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..713e3ce 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind)
+ goto nomem;
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ if (!msg_ind->msg) {
+ kfree(msg_ind);
+nomem:
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ memcpy(msg_ind->msg, buf, len);
+ mutex_lock(&wcn->hal_ind_mutex);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section
@ 2013-12-09 11:34 ` Michal Nazarewicz
0 siblings, 0 replies; 27+ messages in thread
From: Michal Nazarewicz @ 2013-12-09 11:34 UTC (permalink / raw)
To: Eugene Krasnikov
Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2809 bytes --]
Commit [3469adb3: fix potential NULL pointer dereference] introduced
a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
Moreover, it introduced two if statements, which looked a bit clunky.
This commit moves allocation code outside of the critical section so
there's no need to dance around mutex_unlock, and adds the missing
allocation check.
---
drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
On Mon, Dec 09 2013, Eugene Krasnikov wrote:
> hal_ind_mutex suppose to protect msg_ind but with this patch
> allocation will be done outside the critical section.
msg_ind is a local variable which is not shared outside of the function
until it's added to a list. I don't see how it matters if it's
allocation and initialisation is done under mutex.
In fact, there's another kmalloc call that should be checked and can be
done outside of the critical section, so disregard my previous patch,
here's a new one.
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..713e3ce 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind)
+ goto nomem;
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ if (!msg_ind->msg) {
+ kfree(msg_ind);
+nomem:
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ memcpy(msg_ind->msg, buf, len);
+ mutex_lock(&wcn->hal_ind_mutex);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply related [flat|nested] 27+ messages in thread* [ath9k-devel] [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section
2013-12-09 11:34 ` Michal Nazarewicz
@ 2013-12-11 15:52 ` John W. Linville
-1 siblings, 0 replies; 27+ messages in thread
From: John W. Linville @ 2013-12-11 15:52 UTC (permalink / raw)
To: ath9k-devel
Missing Signed-off-by...
On Mon, Dec 09, 2013 at 12:34:04PM +0100, Michal Nazarewicz wrote:
> Commit [3469adb3: fix potential NULL pointer dereference] introduced
> a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
> Moreover, it introduced two if statements, which looked a bit clunky.
>
> This commit moves allocation code outside of the critical section so
> there's no need to dance around mutex_unlock, and adds the missing
> allocation check.
>
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> On Mon, Dec 09 2013, Eugene Krasnikov wrote:
> > hal_ind_mutex suppose to protect msg_ind but with this patch
> > allocation will be done outside the critical section.
>
> msg_ind is a local variable which is not shared outside of the function
> until it's added to a list. I don't see how it matters if it's
> allocation and initialisation is done under mutex.
>
> In fact, there's another kmalloc call that should be checked and can be
> done outside of the critical section, so disregard my previous patch,
> here's a new one.
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 823631c..713e3ce 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2056,22 +2056,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_OTA_TX_COMPL_IND:
> case WCN36XX_HAL_MISSED_BEACON_IND:
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - if (msg_ind) {
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (!msg_ind)
> + goto nomem;
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + if (!msg_ind->msg) {
> + kfree(msg_ind);
> +nomem:
> + /*
> + * FIXME: Do something smarter then just
> + * printing an error.
> + */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> + break;
> }
> + memcpy(msg_ind->msg, buf, len);
> + mutex_lock(&wcn->hal_ind_mutex);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> mutex_unlock(&wcn->hal_ind_mutex);
> - if (msg_ind)
> - break;
> - /* FIXME: Do something smarter then just printing an error. */
> - wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> - msg_header->msg_type);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4
>
--
John W. Linville Someday the world will need a hero, and you
linville at tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section
@ 2013-12-11 15:52 ` John W. Linville
0 siblings, 0 replies; 27+ messages in thread
From: John W. Linville @ 2013-12-11 15:52 UTC (permalink / raw)
To: Michal Nazarewicz
Cc: Eugene Krasnikov, Peter Stuge, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
Missing Signed-off-by...
On Mon, Dec 09, 2013 at 12:34:04PM +0100, Michal Nazarewicz wrote:
> Commit [3469adb3: fix potential NULL pointer dereference] introduced
> a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
> Moreover, it introduced two if statements, which looked a bit clunky.
>
> This commit moves allocation code outside of the critical section so
> there's no need to dance around mutex_unlock, and adds the missing
> allocation check.
>
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> On Mon, Dec 09 2013, Eugene Krasnikov wrote:
> > hal_ind_mutex suppose to protect msg_ind but with this patch
> > allocation will be done outside the critical section.
>
> msg_ind is a local variable which is not shared outside of the function
> until it's added to a list. I don't see how it matters if it's
> allocation and initialisation is done under mutex.
>
> In fact, there's another kmalloc call that should be checked and can be
> done outside of the critical section, so disregard my previous patch,
> here's a new one.
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 823631c..713e3ce 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2056,22 +2056,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_OTA_TX_COMPL_IND:
> case WCN36XX_HAL_MISSED_BEACON_IND:
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - if (msg_ind) {
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (!msg_ind)
> + goto nomem;
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + if (!msg_ind->msg) {
> + kfree(msg_ind);
> +nomem:
> + /*
> + * FIXME: Do something smarter then just
> + * printing an error.
> + */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> + break;
> }
> + memcpy(msg_ind->msg, buf, len);
> + mutex_lock(&wcn->hal_ind_mutex);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> mutex_unlock(&wcn->hal_ind_mutex);
> - if (msg_ind)
> - break;
> - /* FIXME: Do something smarter then just printing an error. */
> - wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> - msg_header->msg_type);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4
>
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 27+ messages in thread* [ath9k-devel] [PATCH RESEND] net: wirelesse: wcn36xx: pull allocation outside of critical section
2013-12-11 15:52 ` John W. Linville
@ 2013-12-11 16:43 ` Michal Nazarewicz
-1 siblings, 0 replies; 27+ messages in thread
From: Michal Nazarewicz @ 2013-12-11 16:43 UTC (permalink / raw)
To: ath9k-devel
Commit [3469adb3: fix potential NULL pointer dereference] introduced
a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
Moreover, it introduced two if statements, which looked a bit clunky.
This commit moves allocation code outside of the critical section so
there's no need to dance around mutex_unlock, and adds the missing
allocation check.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
On Wed, Dec 11 2013, John W. Linville wrote:
> Missing Signed-off-by...
Sorry about that.
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 8f37562..750626b 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2060,22 +2060,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind)
+ goto nomem;
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ if (!msg_ind->msg) {
+ kfree(msg_ind);
+nomem:
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ memcpy(msg_ind->msg, buf, len);
+ mutex_lock(&wcn->hal_ind_mutex);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH RESEND] net: wirelesse: wcn36xx: pull allocation outside of critical section
@ 2013-12-11 16:43 ` Michal Nazarewicz
0 siblings, 0 replies; 27+ messages in thread
From: Michal Nazarewicz @ 2013-12-11 16:43 UTC (permalink / raw)
To: John W. Linville
Cc: Eugene Krasnikov, Peter Stuge, ath9k-devel, linux-wireless,
linux-kernel@vger.kernel.org
Commit [3469adb3: fix potential NULL pointer dereference] introduced
a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
Moreover, it introduced two if statements, which looked a bit clunky.
This commit moves allocation code outside of the critical section so
there's no need to dance around mutex_unlock, and adds the missing
allocation check.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
On Wed, Dec 11 2013, John W. Linville wrote:
> Missing Signed-off-by...
Sorry about that.
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 8f37562..750626b 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2060,22 +2060,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind)
+ goto nomem;
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ if (!msg_ind->msg) {
+ kfree(msg_ind);
+nomem:
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ memcpy(msg_ind->msg, buf, len);
+ mutex_lock(&wcn->hal_ind_mutex);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4
^ permalink raw reply related [flat|nested] 27+ messages in thread