The kthread_run() function returns either a valid task_struct or
ERR_PTR() value, check for NULL is invalid. The change fixes potential
oops, e.g. in OOM situation.
Signed-off-by: Vladimir Zapolskiy <[email protected]>
---
drivers/staging/wilc1000/linux_wlan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 54fe9d7..5077c30 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev)
PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
"K_TXQ_TASK");
- if (!wilc->txq_thread) {
+ if (IS_ERR(wilc->txq_thread)) {
PRINT_ER("couldn't create TXQ thread\n");
wilc->close = 0;
- return -ENOBUFS;
+ return PTR_ERR(wilc->txq_thread);
}
down(&wilc->txq_thread_started);
--
2.1.4
Hi Julian,
On 10.03.2016 02:24, Julian Calaby wrote:
> Hi Vladimir,
>
> On Thu, Mar 10, 2016 at 11:02 AM, Vladimir Zapolskiy <[email protected]> wrote:
>> Hi Julian,
>>
>> On 10.03.2016 01:42, Julian Calaby wrote:
>>> Hi Vladimir,
>>>
>>> On Thu, Mar 10, 2016 at 10:30 AM, Vladimir Zapolskiy <[email protected]> wrote:
>>>> Hi Julian,
>>>>
>>>> On 10.03.2016 01:27, Julian Calaby wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <[email protected]> wrote:
>>>>>> The kthread_run() function returns either a valid task_struct or
>>>>>> ERR_PTR() value, check for NULL is invalid. The change fixes potential
>>>>>> oops, e.g. in OOM situation.
>>>>>>
>>>>>> Signed-off-by: Vladimir Zapolskiy <[email protected]>
>>>>>> ---
>>>>>> drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
>>>>>> index 54fe9d7..5077c30 100644
>>>>>> --- a/drivers/staging/wilc1000/linux_wlan.c
>>>>>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>>>>>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev)
>>>>>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>>>>>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>>>>>> "K_TXQ_TASK");
>>>>>> - if (!wilc->txq_thread) {
>>>>>> + if (IS_ERR(wilc->txq_thread)) {
>>>>>> PRINT_ER("couldn't create TXQ thread\n");
>>>>>> wilc->close = 0;
>>>>>> - return -ENOBUFS;
>>>>>> + return PTR_ERR(wilc->txq_thread);
>>>>>
>>>>> Are you sure changing the error returned is correct? Do all the
>>>>> callers of wlan_initialize_threads() handle the full range of errors
>>>>> from kthread_run()?
>>>>
>>>> Have you checked the driver?
>>>
>>> I'm making sure you have. It's possible that there's a good reason why
>>> this returns -ENOBUFS I want to know that you've at least considered
>>> that possibility.
>>
>> You have my confirmation, I've checked the call stack before publishing
>> this fix.
>
> Awesome.
>
>>>> This function is called once on initialization, the check on the upper layer
>>>> has "if (ret < 0) goto exit_badly;" form.
>>>
>>> And practically everything in the chain up to net_device_ops uses the
>>> same error handling scheme so it's probably fine.
>>
>> dev_open()
>> __dev_open()
>> wilc_mac_open()
>> wilc1000_wlan_init()
>> wlan_initialize_threads()
>>
>> Oh, why kernel threads within a driver are init'ed/destroyed on
>> each device up/down state transition?
>
> You'll have to ask the driver developers. I believe this was a cross
> platform driver that is currently being Linux-ised, so I'm guessing
> this is some artefact of that.
>
>>> You should also document this change in the commit message.
>>
>> The change is documented in the commit message, take a look. But I didn't
>> add "I swear it does not break anything" ;)
>
> You
> 1. corrected the test in the if statement
> 2. changed the return value from -ENOBUFS
> in your patch, however you only documented the first part.
Agree, it makes sense.
> I would have expected a commit message along the lines of:
>
> ---->8----
>
> The kthread_run() function returns either a valid task_struct or
> ERR_PTR() value, so the check for NULL is invalid.
>
> Also return the error from kthread_run() instead of -ENOBUFS.
>
> ----8<----
>
Before publishing v2 let see if driver maintainers have something else
to add, or may be it is better to preserve -ENOBUFS, or may be they are
so kind to update the commit message on patch application.
Julian, thanks for review.
--
With best wishes,
Vladimir
Hi Julian,
On 10.03.2016 01:42, Julian Calaby wrote:
> Hi Vladimir,
>
> On Thu, Mar 10, 2016 at 10:30 AM, Vladimir Zapolskiy <[email protected]> wrote:
>> Hi Julian,
>>
>> On 10.03.2016 01:27, Julian Calaby wrote:
>>> Hi Vladimir,
>>>
>>> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <[email protected]> wrote:
>>>> The kthread_run() function returns either a valid task_struct or
>>>> ERR_PTR() value, check for NULL is invalid. The change fixes potential
>>>> oops, e.g. in OOM situation.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <[email protected]>
>>>> ---
>>>> drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
>>>> index 54fe9d7..5077c30 100644
>>>> --- a/drivers/staging/wilc1000/linux_wlan.c
>>>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>>>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev)
>>>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>>>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>>>> "K_TXQ_TASK");
>>>> - if (!wilc->txq_thread) {
>>>> + if (IS_ERR(wilc->txq_thread)) {
>>>> PRINT_ER("couldn't create TXQ thread\n");
>>>> wilc->close = 0;
>>>> - return -ENOBUFS;
>>>> + return PTR_ERR(wilc->txq_thread);
>>>
>>> Are you sure changing the error returned is correct? Do all the
>>> callers of wlan_initialize_threads() handle the full range of errors
>>> from kthread_run()?
>>
>> Have you checked the driver?
>
> I'm making sure you have. It's possible that there's a good reason why
> this returns -ENOBUFS I want to know that you've at least considered
> that possibility.
You have my confirmation, I've checked the call stack before publishing
this fix.
>> This function is called once on initialization, the check on the upper layer
>> has "if (ret < 0) goto exit_badly;" form.
>
> And practically everything in the chain up to net_device_ops uses the
> same error handling scheme so it's probably fine.
dev_open()
__dev_open()
wilc_mac_open()
wilc1000_wlan_init()
wlan_initialize_threads()
Oh, why kernel threads within a driver are init'ed/destroyed on
each device up/down state transition?
> You should also document this change in the commit message.
The change is documented in the commit message, take a look. But I didn't
add "I swear it does not break anything" ;)
--
With best wishes,
Vladimir
Hi Vladimir,
On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <[email protected]> wrote:
> The kthread_run() function returns either a valid task_struct or
> ERR_PTR() value, check for NULL is invalid. The change fixes potential
> oops, e.g. in OOM situation.
>
> Signed-off-by: Vladimir Zapolskiy <[email protected]>
> ---
> drivers/staging/wilc1000/linux_wlan.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
> index 54fe9d7..5077c30 100644
> --- a/drivers/staging/wilc1000/linux_wlan.c
> +++ b/drivers/staging/wilc1000/linux_wlan.c
> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev)
> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
> "K_TXQ_TASK");
> - if (!wilc->txq_thread) {
> + if (IS_ERR(wilc->txq_thread)) {
> PRINT_ER("couldn't create TXQ thread\n");
> wilc->close = 0;
> - return -ENOBUFS;
> + return PTR_ERR(wilc->txq_thread);
Are you sure changing the error returned is correct? Do all the
callers of wlan_initialize_threads() handle the full range of errors
from kthread_run()?
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
SGkgVmxhZGltaXIgJiBKdWxpYW4NCg0KVGhlIGRyaXZlciBpcyBzdGlsbCBtb3ZpbmcgdG8gdGhl
IExpbnV4IGRyaXZlciBhbmQgYWxzbyBpdHMgYmVpbmcgY2hhbmdlZC4NCllvdXIgcGF0Y2ggaXMg
ZXZlcnl0aGluZydzIG9rLg0KSSBhbHNvIGFncmVlIHRvIEp1bGlhbiByZXZpZXcuDQoNCsKgVGhh
bmtzLCBCUg0KwqBMZW8NCg0KLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCkZyb206IFZsYWRp
bWlyIFphcG9sc2tpeSBbbWFpbHRvOnZ6QG1sZWlhLmNvbV0gDQpTZW50OiBUaHVyc2RheSwgTWFy
Y2ggMTAsIDIwMTYgOTozNiBBTQ0KVG86IEp1bGlhbiBDYWxhYnkgPGp1bGlhbi5jYWxhYnlAZ21h
aWwuY29tPg0KQ2M6IExlZSwgR2xlbiA8R2xlbi5MZWVAYXRtZWwuY29tPjsgR3JlZyBLcm9haC1I
YXJ0bWFuIDxncmVna2hAbGludXhmb3VuZGF0aW9uLm9yZz47IEtpbSwgSm9obm55IDxqb2hubnku
a2ltQGF0bWVsLmNvbT47IFNoaW4sIEF1c3RpbiA8QXVzdGluLlNoaW5AYXRtZWwuY29tPjsgUGFy
aywgQ2hyaXMgPENocmlzLlBhcmtAYXRtZWwuY29tPjsgQ2hvLCBUb255IDxUb255LkNob0BhdG1l
bC5jb20+OyBLaW0sIExlbyA8TGVvLktpbUBhdG1lbC5jb20+OyBsaW51eC13aXJlbGVzcyA8bGlu
dXgtd2lyZWxlc3NAdmdlci5rZXJuZWwub3JnPjsgZGV2ZWxAZHJpdmVyZGV2Lm9zdW9zbC5vcmcN
ClN1YmplY3Q6IFJlOiBbUEFUQ0hdIHN0YWdpbmc6IHdpbGMxMDAwOiBmaXggY2hlY2sgb2Yga3Ro
cmVhZF9ydW4oKSByZXR1cm4gdmFsdWUNCg0KSGkgSnVsaWFuLA0KDQpPbiAxMC4wMy4yMDE2IDAy
OjI0LCBKdWxpYW4gQ2FsYWJ5IHdyb3RlOg0KPiBIaSBWbGFkaW1pciwNCj4gDQo+IE9uIFRodSwg
TWFyIDEwLCAyMDE2IGF0IDExOjAyIEFNLCBWbGFkaW1pciBaYXBvbHNraXkgPHZ6QG1sZWlhLmNv
bT4gd3JvdGU6DQo+PiBIaSBKdWxpYW4sDQo+Pg0KPj4gT24gMTAuMDMuMjAxNiAwMTo0MiwgSnVs
aWFuIENhbGFieSB3cm90ZToNCj4+PiBIaSBWbGFkaW1pciwNCj4+Pg0KPj4+IE9uIFRodSwgTWFy
IDEwLCAyMDE2IGF0IDEwOjMwIEFNLCBWbGFkaW1pciBaYXBvbHNraXkgPHZ6QG1sZWlhLmNvbT4g
d3JvdGU6DQo+Pj4+IEhpIEp1bGlhbiwNCj4+Pj4NCj4+Pj4gT24gMTAuMDMuMjAxNiAwMToyNywg
SnVsaWFuIENhbGFieSB3cm90ZToNCj4+Pj4+IEhpIFZsYWRpbWlyLA0KPj4+Pj4NCj4+Pj4+IE9u
IFRodSwgTWFyIDEwLCAyMDE2IGF0IDEwOjEzIEFNLCBWbGFkaW1pciBaYXBvbHNraXkgPHZ6QG1s
ZWlhLmNvbT4gd3JvdGU6DQo+Pj4+Pj4gVGhlIGt0aHJlYWRfcnVuKCkgZnVuY3Rpb24gcmV0dXJu
cyBlaXRoZXIgYSB2YWxpZCB0YXNrX3N0cnVjdCBvcg0KPj4+Pj4+IEVSUl9QVFIoKSB2YWx1ZSwg
Y2hlY2sgZm9yIE5VTEwgaXMgaW52YWxpZC4gVGhlIGNoYW5nZSBmaXhlcyANCj4+Pj4+PiBwb3Rl
bnRpYWwgb29wcywgZS5nLiBpbiBPT00gc2l0dWF0aW9uLg0KPj4+Pj4+DQo+Pj4+Pj4gU2lnbmVk
LW9mZi1ieTogVmxhZGltaXIgWmFwb2xza2l5IDx2ekBtbGVpYS5jb20+DQo+Pj4+Pj4gLS0tDQo+
Pj4+Pj4gIGRyaXZlcnMvc3RhZ2luZy93aWxjMTAwMC9saW51eF93bGFuLmMgfCA0ICsrLS0NCj4+
Pj4+PiAgMSBmaWxlIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkNCj4+
Pj4+Pg0KPj4+Pj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3N0YWdpbmcvd2lsYzEwMDAvbGludXhf
d2xhbi5jIA0KPj4+Pj4+IGIvZHJpdmVycy9zdGFnaW5nL3dpbGMxMDAwL2xpbnV4X3dsYW4uYw0K
Pj4+Pj4+IGluZGV4IDU0ZmU5ZDcuLjUwNzdjMzAgMTAwNjQ0DQo+Pj4+Pj4gLS0tIGEvZHJpdmVy
cy9zdGFnaW5nL3dpbGMxMDAwL2xpbnV4X3dsYW4uYw0KPj4+Pj4+ICsrKyBiL2RyaXZlcnMvc3Rh
Z2luZy93aWxjMTAwMC9saW51eF93bGFuLmMNCj4+Pj4+PiBAQCAtODQ5LDEwICs4NDksMTAgQEAg
c3RhdGljIGludCB3bGFuX2luaXRpYWxpemVfdGhyZWFkcyhzdHJ1Y3QgbmV0X2RldmljZSAqZGV2
KQ0KPj4+Pj4+ICAgICAgICAgUFJJTlRfRChJTklUX0RCRywgIkNyZWF0aW5nIGt0aHJlYWQgZm9y
IHRyYW5zbWlzc2lvblxuIik7DQo+Pj4+Pj4gICAgICAgICB3aWxjLT50eHFfdGhyZWFkID0ga3Ro
cmVhZF9ydW4obGludXhfd2xhbl90eHFfdGFzaywgKHZvaWQgKilkZXYsDQo+Pj4+Pj4gICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICJLX1RYUV9UQVNLIik7DQo+Pj4+Pj4gLSAg
ICAgICBpZiAoIXdpbGMtPnR4cV90aHJlYWQpIHsNCj4+Pj4+PiArICAgICAgIGlmIChJU19FUlIo
d2lsYy0+dHhxX3RocmVhZCkpIHsNCj4+Pj4+PiAgICAgICAgICAgICAgICAgUFJJTlRfRVIoImNv
dWxkbid0IGNyZWF0ZSBUWFEgdGhyZWFkXG4iKTsNCj4+Pj4+PiAgICAgICAgICAgICAgICAgd2ls
Yy0+Y2xvc2UgPSAwOw0KPj4+Pj4+IC0gICAgICAgICAgICAgICByZXR1cm4gLUVOT0JVRlM7DQo+
Pj4+Pj4gKyAgICAgICAgICAgICAgIHJldHVybiBQVFJfRVJSKHdpbGMtPnR4cV90aHJlYWQpOw0K
Pj4+Pj4NCj4+Pj4+IEFyZSB5b3Ugc3VyZSBjaGFuZ2luZyB0aGUgZXJyb3IgcmV0dXJuZWQgaXMg
Y29ycmVjdD8gRG8gYWxsIHRoZSANCj4+Pj4+IGNhbGxlcnMgb2Ygd2xhbl9pbml0aWFsaXplX3Ro
cmVhZHMoKSBoYW5kbGUgdGhlIGZ1bGwgcmFuZ2Ugb2YgDQo+Pj4+PiBlcnJvcnMgZnJvbSBrdGhy
ZWFkX3J1bigpPw0KPj4+Pg0KPj4+PiBIYXZlIHlvdSBjaGVja2VkIHRoZSBkcml2ZXI/DQo+Pj4N
Cj4+PiBJJ20gbWFraW5nIHN1cmUgeW91IGhhdmUuIEl0J3MgcG9zc2libGUgdGhhdCB0aGVyZSdz
IGEgZ29vZCByZWFzb24gDQo+Pj4gd2h5IHRoaXMgcmV0dXJucyAtRU5PQlVGUyBJIHdhbnQgdG8g
a25vdyB0aGF0IHlvdSd2ZSBhdCBsZWFzdCANCj4+PiBjb25zaWRlcmVkIHRoYXQgcG9zc2liaWxp
dHkuDQo+Pg0KPj4gWW91IGhhdmUgbXkgY29uZmlybWF0aW9uLCBJJ3ZlIGNoZWNrZWQgdGhlIGNh
bGwgc3RhY2sgYmVmb3JlIA0KPj4gcHVibGlzaGluZyB0aGlzIGZpeC4NCj4gDQo+IEF3ZXNvbWUu
DQo+IA0KPj4+PiBUaGlzIGZ1bmN0aW9uIGlzIGNhbGxlZCBvbmNlIG9uIGluaXRpYWxpemF0aW9u
LCB0aGUgY2hlY2sgb24gdGhlIA0KPj4+PiB1cHBlciBsYXllciBoYXMgImlmIChyZXQgPCAwKSBn
b3RvIGV4aXRfYmFkbHk7IiBmb3JtLg0KPj4+DQo+Pj4gQW5kIHByYWN0aWNhbGx5IGV2ZXJ5dGhp
bmcgaW4gdGhlIGNoYWluIHVwIHRvIG5ldF9kZXZpY2Vfb3BzIHVzZXMgDQo+Pj4gdGhlIHNhbWUg
ZXJyb3IgaGFuZGxpbmcgc2NoZW1lIHNvIGl0J3MgcHJvYmFibHkgZmluZS4NCj4+DQo+PiBkZXZf
b3BlbigpDQo+PiAgIF9fZGV2X29wZW4oKQ0KPj4gICAgIHdpbGNfbWFjX29wZW4oKQ0KPj4gICAg
ICAgd2lsYzEwMDBfd2xhbl9pbml0KCkNCj4+ICAgICAgICAgd2xhbl9pbml0aWFsaXplX3RocmVh
ZHMoKQ0KPj4NCj4+IE9oLCB3aHkga2VybmVsIHRocmVhZHMgd2l0aGluIGEgZHJpdmVyIGFyZSBp
bml0J2VkL2Rlc3Ryb3llZCBvbiBlYWNoIA0KPj4gZGV2aWNlIHVwL2Rvd24gc3RhdGUgdHJhbnNp
dGlvbj8NCj4gDQo+IFlvdSdsbCBoYXZlIHRvIGFzayB0aGUgZHJpdmVyIGRldmVsb3BlcnMuIEkg
YmVsaWV2ZSB0aGlzIHdhcyBhIGNyb3NzIA0KPiBwbGF0Zm9ybSBkcml2ZXIgdGhhdCBpcyBjdXJy
ZW50bHkgYmVpbmcgTGludXgtaXNlZCwgc28gSSdtIGd1ZXNzaW5nIA0KPiB0aGlzIGlzIHNvbWUg
YXJ0ZWZhY3Qgb2YgdGhhdC4NCj4gDQo+Pj4gWW91IHNob3VsZCBhbHNvIGRvY3VtZW50IHRoaXMg
Y2hhbmdlIGluIHRoZSBjb21taXQgbWVzc2FnZS4NCj4+DQo+PiBUaGUgY2hhbmdlIGlzIGRvY3Vt
ZW50ZWQgaW4gdGhlIGNvbW1pdCBtZXNzYWdlLCB0YWtlIGEgbG9vay4gQnV0IEkgDQo+PiBkaWRu
J3QgYWRkICJJIHN3ZWFyIGl0IGRvZXMgbm90IGJyZWFrIGFueXRoaW5nIiA7KQ0KPiANCj4gWW91
DQo+IDEuIGNvcnJlY3RlZCB0aGUgdGVzdCBpbiB0aGUgaWYgc3RhdGVtZW50IDIuIGNoYW5nZWQg
dGhlIHJldHVybiB2YWx1ZSANCj4gZnJvbSAtRU5PQlVGUyBpbiB5b3VyIHBhdGNoLCBob3dldmVy
IHlvdSBvbmx5IGRvY3VtZW50ZWQgdGhlIGZpcnN0IA0KPiBwYXJ0Lg0KDQpBZ3JlZSwgaXQgbWFr
ZXMgc2Vuc2UuDQoNCj4gSSB3b3VsZCBoYXZlIGV4cGVjdGVkIGEgY29tbWl0IG1lc3NhZ2UgYWxv
bmcgdGhlIGxpbmVzIG9mOg0KPiANCj4gLS0tLT44LS0tLQ0KPiANCj4gVGhlIGt0aHJlYWRfcnVu
KCkgZnVuY3Rpb24gcmV0dXJucyBlaXRoZXIgYSB2YWxpZCB0YXNrX3N0cnVjdCBvcg0KPiBFUlJf
UFRSKCkgdmFsdWUsIHNvIHRoZSBjaGVjayBmb3IgTlVMTCBpcyBpbnZhbGlkLg0KPiANCj4gQWxz
byByZXR1cm4gdGhlIGVycm9yIGZyb20ga3RocmVhZF9ydW4oKSBpbnN0ZWFkIG9mIC1FTk9CVUZT
Lg0KPiANCj4gLS0tLTg8LS0tLQ0KPiANCg0KQmVmb3JlIHB1Ymxpc2hpbmcgdjIgbGV0IHNlZSBp
ZiBkcml2ZXIgbWFpbnRhaW5lcnMgaGF2ZSBzb21ldGhpbmcgZWxzZSB0byBhZGQsIG9yIG1heSBi
ZSBpdCBpcyBiZXR0ZXIgdG8gcHJlc2VydmUgLUVOT0JVRlMsIG9yIG1heSBiZSB0aGV5IGFyZSBz
byBraW5kIHRvIHVwZGF0ZSB0aGUgY29tbWl0IG1lc3NhZ2Ugb24gcGF0Y2ggYXBwbGljYXRpb24u
DQoNCkp1bGlhbiwgdGhhbmtzIGZvciByZXZpZXcuDQoNCi0tDQpXaXRoIGJlc3Qgd2lzaGVzLA0K
VmxhZGltaXINCg==
Hi Julian,
On 10.03.2016 01:27, Julian Calaby wrote:
> Hi Vladimir,
>
> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <[email protected]> wrote:
>> The kthread_run() function returns either a valid task_struct or
>> ERR_PTR() value, check for NULL is invalid. The change fixes potential
>> oops, e.g. in OOM situation.
>>
>> Signed-off-by: Vladimir Zapolskiy <[email protected]>
>> ---
>> drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
>> index 54fe9d7..5077c30 100644
>> --- a/drivers/staging/wilc1000/linux_wlan.c
>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev)
>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>> "K_TXQ_TASK");
>> - if (!wilc->txq_thread) {
>> + if (IS_ERR(wilc->txq_thread)) {
>> PRINT_ER("couldn't create TXQ thread\n");
>> wilc->close = 0;
>> - return -ENOBUFS;
>> + return PTR_ERR(wilc->txq_thread);
>
> Are you sure changing the error returned is correct? Do all the
> callers of wlan_initialize_threads() handle the full range of errors
> from kthread_run()?
Have you checked the driver?
This function is called once on initialization, the check on the upper layer
has "if (ret < 0) goto exit_badly;" form.
--
With best wishes,
Vladimir
Hi Vladimir,
On Thu, Mar 10, 2016 at 11:02 AM, Vladimir Zapolskiy <[email protected]> wrote:
> Hi Julian,
>
> On 10.03.2016 01:42, Julian Calaby wrote:
>> Hi Vladimir,
>>
>> On Thu, Mar 10, 2016 at 10:30 AM, Vladimir Zapolskiy <[email protected]> wrote:
>>> Hi Julian,
>>>
>>> On 10.03.2016 01:27, Julian Calaby wrote:
>>>> Hi Vladimir,
>>>>
>>>> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <[email protected]> wrote:
>>>>> The kthread_run() function returns either a valid task_struct or
>>>>> ERR_PTR() value, check for NULL is invalid. The change fixes potential
>>>>> oops, e.g. in OOM situation.
>>>>>
>>>>> Signed-off-by: Vladimir Zapolskiy <[email protected]>
>>>>> ---
>>>>> drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
>>>>> index 54fe9d7..5077c30 100644
>>>>> --- a/drivers/staging/wilc1000/linux_wlan.c
>>>>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>>>>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev)
>>>>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>>>>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>>>>> "K_TXQ_TASK");
>>>>> - if (!wilc->txq_thread) {
>>>>> + if (IS_ERR(wilc->txq_thread)) {
>>>>> PRINT_ER("couldn't create TXQ thread\n");
>>>>> wilc->close = 0;
>>>>> - return -ENOBUFS;
>>>>> + return PTR_ERR(wilc->txq_thread);
>>>>
>>>> Are you sure changing the error returned is correct? Do all the
>>>> callers of wlan_initialize_threads() handle the full range of errors
>>>> from kthread_run()?
>>>
>>> Have you checked the driver?
>>
>> I'm making sure you have. It's possible that there's a good reason why
>> this returns -ENOBUFS I want to know that you've at least considered
>> that possibility.
>
> You have my confirmation, I've checked the call stack before publishing
> this fix.
Awesome.
>>> This function is called once on initialization, the check on the upper layer
>>> has "if (ret < 0) goto exit_badly;" form.
>>
>> And practically everything in the chain up to net_device_ops uses the
>> same error handling scheme so it's probably fine.
>
> dev_open()
> __dev_open()
> wilc_mac_open()
> wilc1000_wlan_init()
> wlan_initialize_threads()
>
> Oh, why kernel threads within a driver are init'ed/destroyed on
> each device up/down state transition?
You'll have to ask the driver developers. I believe this was a cross
platform driver that is currently being Linux-ised, so I'm guessing
this is some artefact of that.
>> You should also document this change in the commit message.
>
> The change is documented in the commit message, take a look. But I didn't
> add "I swear it does not break anything" ;)
You
1. corrected the test in the if statement
2. changed the return value from -ENOBUFS
in your patch, however you only documented the first part.
I would have expected a commit message along the lines of:
---->8----
The kthread_run() function returns either a valid task_struct or
ERR_PTR() value, so the check for NULL is invalid.
Also return the error from kthread_run() instead of -ENOBUFS.
----8<----
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi Vladimir,
On Thu, Mar 10, 2016 at 10:30 AM, Vladimir Zapolskiy <[email protected]> wrote:
> Hi Julian,
>
> On 10.03.2016 01:27, Julian Calaby wrote:
>> Hi Vladimir,
>>
>> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy <[email protected]> wrote:
>>> The kthread_run() function returns either a valid task_struct or
>>> ERR_PTR() value, check for NULL is invalid. The change fixes potential
>>> oops, e.g. in OOM situation.
>>>
>>> Signed-off-by: Vladimir Zapolskiy <[email protected]>
>>> ---
>>> drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
>>> index 54fe9d7..5077c30 100644
>>> --- a/drivers/staging/wilc1000/linux_wlan.c
>>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct net_device *dev)
>>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>>> "K_TXQ_TASK");
>>> - if (!wilc->txq_thread) {
>>> + if (IS_ERR(wilc->txq_thread)) {
>>> PRINT_ER("couldn't create TXQ thread\n");
>>> wilc->close = 0;
>>> - return -ENOBUFS;
>>> + return PTR_ERR(wilc->txq_thread);
>>
>> Are you sure changing the error returned is correct? Do all the
>> callers of wlan_initialize_threads() handle the full range of errors
>> from kthread_run()?
>
> Have you checked the driver?
I'm making sure you have. It's possible that there's a good reason why
this returns -ENOBUFS I want to know that you've at least considered
that possibility.
> This function is called once on initialization, the check on the upper layer
> has "if (ret < 0) goto exit_badly;" form.
And practically everything in the chain up to net_device_ops uses the
same error handling scheme so it's probably fine.
You should also document this change in the commit message.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/