Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn

Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn

[Grumbach, Emmanuel]

T24gV2VkLCAyMDE1LTA2LTEwIGF0IDEyOjMzIC0wNDAwLCBOaWNob2xhcyBLcmF1c2Ugd3JvdGU6
DQo+IFRoaXMgbWFrZXMgdGhlIGZ1bmN0aW9uIGl3bF9yZXN1bWVfc3RhdHVzX2ZuIHJldHVybiBm
YWxzZSBub3cgaWYNCj4gdGhlIHJlY2VpdmVkIHBhY2tldCBvZiB0eXBlIGl3bF9yeF9wYWNrZXQg
aXMgbm90IHRoZSBzYW1lIHNpemUNCj4gYXMgdGhlIHN0cnVjdHVyZSBwb2ludGVyLCBpd2xfcmVz
dW1lX2RhdGEncyBjbWQgZWxlbWVudCBpbiBvcmRlcg0KPiB0byBzaWduYWwgY2FsbGVycyBhYm91
dCB0aGlzIGVycm9yIGFuZCBhbGxvdyB0aGVtIHRvIGhhbmRsZSBpdA0KPiBvY2N1cnJlbnRseS4N
Cj4gDQoNCkhtLi4uIERpZCB5b3UgYWN0dWFsbHkgaGl0IHRoaXMgaWY/DQpJIGFtIG5vdCBzdXJl
IEkgcmVhbGx5IHdhbnQgdG8gd2FpdCBoZXJlICh3aGljaCBpcyB3aGF0IHdpbGwgaGFwcGVuIGlm
DQp5b3UgcmV0dXJuIGZhbHNlKSB3aGVuIHdlIGdldCBhbiB1bmV4cGVjdGVkIGxlbmd0aD8gSSBk
byBub3QgZXhwZWN0DQphbnl0aGluZyBiZXNpZGVzIHRoZSByZXNwb25zZSBJIGFtIHdhaXRpbmcg
Zm9yIHNpbmNlIHRoZSBmaXJtd2FyZSBpcw0KaGFuZGxpbmcgdGhlIEdFVF9TVEFUVVMgKm9ubHkq
IC0gaXQganVzdCBjYW1lIGJhY2sgZnJvbSBXb1dMQU4uIEJvdHRvbQ0KbGluZSwgdGhpcyBpcyBy
ZWFsbHkgYW4gZXJyb3IgcGF0aCBhbmQgSSBwcmVmZXIgdG8gZXhpdCBhbmQgbm90IHdhaXQgZm9y
DQp0aGUgdGltZW91dCBpbiB0aGF0IGNhc2UuDQpCdXQgSSBtaWdodCBiZSBtaXNzaW5nIHNvbWV0
aGluZz8NCg0KPiBTaWduZWQtb2ZmLWJ5OiBOaWNob2xhcyBLcmF1c2UgPHhlcm9mb2lmeUBnbWFp
bC5jb20+DQo+IC0tLQ0KPiAgZHJpdmVycy9uZXQvd2lyZWxlc3MvaXdsd2lmaS9kdm0vbWFjODAy
MTEuYyB8IDIgKy0NCj4gIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKSwgMSBkZWxldGlv
bigtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbmV0L3dpcmVsZXNzL2l3bHdpZmkvZHZt
L21hYzgwMjExLmMgYi9kcml2ZXJzL25ldC93aXJlbGVzcy9pd2x3aWZpL2R2bS9tYWM4MDIxMS5j
DQo+IGluZGV4IDVhYmQ2MmUuLjIxZTgwOGMgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvbmV0L3dp
cmVsZXNzL2l3bHdpZmkvZHZtL21hYzgwMjExLmMNCj4gKysrIGIvZHJpdmVycy9uZXQvd2lyZWxl
c3MvaXdsd2lmaS9kdm0vbWFjODAyMTEuYw0KPiBAQCAtNDA5LDcgKzQwOSw3IEBAIHN0YXRpYyBi
b29sIGl3bF9yZXN1bWVfc3RhdHVzX2ZuKHN0cnVjdCBpd2xfbm90aWZfd2FpdF9kYXRhICpub3Rp
Zl93YWl0LA0KPiAgDQo+ICAJaWYgKGl3bF9yeF9wYWNrZXRfcGF5bG9hZF9sZW4ocGt0KSAhPSBz
aXplb2YoKnJlc3VtZV9kYXRhLT5jbWQpKSB7DQo+ICAJCUlXTF9FUlIocHJpdiwgInJ4IHdyb25n
IHNpemUgZGF0YVxuIik7DQo+IC0JCXJldHVybiB0cnVlOw0KPiArCQlyZXR1cm4gZmFsc2U7DQo+
ICAJfQ0KPiAgCW1lbWNweShyZXN1bWVfZGF0YS0+Y21kLCBwa3QtPmRhdGEsIHNpemVvZigqcmVz
dW1lX2RhdGEtPmNtZCkpOw0KPiAgCXJlc3VtZV9kYXRhLT52YWxpZCA9IHRydWU7DQoNCg==

Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn

[Grumbach, Emmanuel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1715 bytes --]

On Wed, 2015-06-10 at 12:33 -0400, Nicholas Krause wrote:
> This makes the function iwl_resume_status_fn return false now if
> the received packet of type iwl_rx_packet is not the same size
> as the structure pointer, iwl_resume_data's cmd element in order
> to signal callers about this error and allow them to handle it
> occurrently.
> 

Hm... Did you actually hit this if?
I am not sure I really want to wait here (which is what will happen if
you return false) when we get an unexpected length? I do not expect
anything besides the response I am waiting for since the firmware is
handling the GET_STATUS *only* - it just came back from WoWLAN. Bottom
line, this is really an error path and I prefer to exit and not wait for
the timeout in that case.
But I might be missing something?

> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/net/wireless/iwlwifi/dvm/mac80211.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> index 5abd62e..21e808c 100644
> --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> @@ -409,7 +409,7 @@ static bool iwl_resume_status_fn(struct iwl_notif_wait_data *notif_wait,
>  
>  	if (iwl_rx_packet_payload_len(pkt) != sizeof(*resume_data->cmd)) {
>  		IWL_ERR(priv, "rx wrong size data\n");
> -		return true;
> +		return false;
>  	}
>  	memcpy(resume_data->cmd, pkt->data, sizeof(*resume_data->cmd));
>  	resume_data->valid = true;

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn

[Grumbach, Emmanuel]

On Wed, 2015-06-10 at 12:33 -0400, Nicholas Krause wrote:
> This makes the function iwl_resume_status_fn return false now if
> the received packet of type iwl_rx_packet is not the same size
> as the structure pointer, iwl_resume_data's cmd element in order
> to signal callers about this error and allow them to handle it
> occurrently.
> 

Hm... Did you actually hit this if?
I am not sure I really want to wait here (which is what will happen if
you return false) when we get an unexpected length? I do not expect
anything besides the response I am waiting for since the firmware is
handling the GET_STATUS *only* - it just came back from WoWLAN. Bottom
line, this is really an error path and I prefer to exit and not wait for
the timeout in that case.
But I might be missing something?

> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/net/wireless/iwlwifi/dvm/mac80211.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> index 5abd62e..21e808c 100644
> --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> @@ -409,7 +409,7 @@ static bool iwl_resume_status_fn(struct iwl_notif_wait_data *notif_wait,
>  
>  	if (iwl_rx_packet_payload_len(pkt) != sizeof(*resume_data->cmd)) {
>  		IWL_ERR(priv, "rx wrong size data\n");
> -		return true;
> +		return false;
>  	}
>  	memcpy(resume_data->cmd, pkt->data, sizeof(*resume_data->cmd));
>  	resume_data->valid = true;

Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn

[Grumbach, Emmanuel]

T24gV2VkLCAyMDE1LTA2LTEwIGF0IDEyOjU4IC0wNDAwLCBOaWNob2xhcyBLcmF1c2Ugd3JvdGU6
DQo+IA0KPiBPbiBKdW5lIDEwLCAyMDE1IDEyOjUwOjQ1IFBNIEVEVCwgIkdydW1iYWNoLCBFbW1h
bnVlbCIgPGVtbWFudWVsLmdydW1iYWNoQGludGVsLmNvbT4gd3JvdGU6DQo+ID5PbiBXZWQsIDIw
MTUtMDYtMTAgYXQgMTI6MzMgLTA0MDAsIE5pY2hvbGFzIEtyYXVzZSB3cm90ZToNCj4gPj4gVGhp
cyBtYWtlcyB0aGUgZnVuY3Rpb24gaXdsX3Jlc3VtZV9zdGF0dXNfZm4gcmV0dXJuIGZhbHNlIG5v
dyBpZg0KPiA+PiB0aGUgcmVjZWl2ZWQgcGFja2V0IG9mIHR5cGUgaXdsX3J4X3BhY2tldCBpcyBu
b3QgdGhlIHNhbWUgc2l6ZQ0KPiA+PiBhcyB0aGUgc3RydWN0dXJlIHBvaW50ZXIsIGl3bF9yZXN1
bWVfZGF0YSdzIGNtZCBlbGVtZW50IGluIG9yZGVyDQo+ID4+IHRvIHNpZ25hbCBjYWxsZXJzIGFi
b3V0IHRoaXMgZXJyb3IgYW5kIGFsbG93IHRoZW0gdG8gaGFuZGxlIGl0DQo+ID4+IG9jY3VycmVu
dGx5Lg0KPiA+PiANCj4gPg0KPiA+SG0uLi4gRGlkIHlvdSBhY3R1YWxseSBoaXQgdGhpcyBpZj8N
Cj4gPkkgYW0gbm90IHN1cmUgSSByZWFsbHkgd2FudCB0byB3YWl0IGhlcmUgKHdoaWNoIGlzIHdo
YXQgd2lsbCBoYXBwZW4gaWYNCj4gPnlvdSByZXR1cm4gZmFsc2UpIHdoZW4gd2UgZ2V0IGFuIHVu
ZXhwZWN0ZWQgbGVuZ3RoPyBJIGRvIG5vdCBleHBlY3QNCj4gPmFueXRoaW5nIGJlc2lkZXMgdGhl
IHJlc3BvbnNlIEkgYW0gd2FpdGluZyBmb3Igc2luY2UgdGhlIGZpcm13YXJlIGlzDQo+ID5oYW5k
bGluZyB0aGUgR0VUX1NUQVRVUyAqb25seSogLSBpdCBqdXN0IGNhbWUgYmFjayBmcm9tIFdvV0xB
Ti4gQm90dG9tDQo+ID5saW5lLCB0aGlzIGlzIHJlYWxseSBhbiBlcnJvciBwYXRoIGFuZCBJIHBy
ZWZlciB0byBleGl0IGFuZCBub3Qgd2FpdA0KPiA+Zm9yDQo+ID50aGUgdGltZW91dCBpbiB0aGF0
IGNhc2UuDQo+ID5CdXQgSSBtaWdodCBiZSBtaXNzaW5nIHNvbWV0aGluZz8NCj4gPg0KPiBXaHkg
bm90IHdhaXQgZm9yIHRoZSB0aW1lIG91dD8gIFNlZW1zIHRoZXJlIGlzIG5vIHJlYXNvbiBub3Qg
dG8gYW5kIGluIHRoYXQgY2FzZQ0KPiBpZiB0aGUgZmlybXdhcmUgaGFuZGxlcyB0aGlzDQoNCkkg
ZG91YnQgaXQgd2lsbC4gVGhpcyBnb2VzIGJhY2sgdG8gbXkgb3JpZ2luYWwgcXVlc3Rpb246IGRp
ZCB5b3UgcmVhbGx5DQpoaXQgdGhpcyBwYXRoPyBEb2VzIHRoZSBwYXRjaCBzb2x2ZXMgYSByZWFs
IGJ1ZyB5b3UgZmFjZWQ/DQoNCj4gd2h5IGlzIHRoZSBpZiBzdGF0ZW1lbnQgc3RpbGwgaGVyZS4g
DQoNClRvIGdldCBhIGRlYnVnIHByaW50PyA6KQ0KDQo+IE5pY2sgDQo+ID4+IFNpZ25lZC1vZmYt
Ynk6IE5pY2hvbGFzIEtyYXVzZSA8eGVyb2ZvaWZ5QGdtYWlsLmNvbT4NCj4gPj4gLS0tDQo+ID4+
ICBkcml2ZXJzL25ldC93aXJlbGVzcy9pd2x3aWZpL2R2bS9tYWM4MDIxMS5jIHwgMiArLQ0KPiA+
PiAgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pDQo+ID4+IA0K
PiA+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9uZXQvd2lyZWxlc3MvaXdsd2lmaS9kdm0vbWFjODAy
MTEuYw0KPiA+Yi9kcml2ZXJzL25ldC93aXJlbGVzcy9pd2x3aWZpL2R2bS9tYWM4MDIxMS5jDQo+
ID4+IGluZGV4IDVhYmQ2MmUuLjIxZTgwOGMgMTAwNjQ0DQo+ID4+IC0tLSBhL2RyaXZlcnMvbmV0
L3dpcmVsZXNzL2l3bHdpZmkvZHZtL21hYzgwMjExLmMNCj4gPj4gKysrIGIvZHJpdmVycy9uZXQv
d2lyZWxlc3MvaXdsd2lmaS9kdm0vbWFjODAyMTEuYw0KPiA+PiBAQCAtNDA5LDcgKzQwOSw3IEBA
IHN0YXRpYyBib29sIGl3bF9yZXN1bWVfc3RhdHVzX2ZuKHN0cnVjdA0KPiA+aXdsX25vdGlmX3dh
aXRfZGF0YSAqbm90aWZfd2FpdCwNCj4gPj4gIA0KPiA+PiAgCWlmIChpd2xfcnhfcGFja2V0X3Bh
eWxvYWRfbGVuKHBrdCkgIT0gc2l6ZW9mKCpyZXN1bWVfZGF0YS0+Y21kKSkgew0KPiA+PiAgCQlJ
V0xfRVJSKHByaXYsICJyeCB3cm9uZyBzaXplIGRhdGFcbiIpOw0KPiA+PiAtCQlyZXR1cm4gdHJ1
ZTsNCj4gPj4gKwkJcmV0dXJuIGZhbHNlOw0KPiA+PiAgCX0NCj4gPj4gIAltZW1jcHkocmVzdW1l
X2RhdGEtPmNtZCwgcGt0LT5kYXRhLCBzaXplb2YoKnJlc3VtZV9kYXRhLT5jbWQpKTsNCj4gPj4g
IAlyZXN1bWVfZGF0YS0+dmFsaWQgPSB0cnVlOw0KPiANCg0K

Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn

[Grumbach, Emmanuel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2308 bytes --]

On Wed, 2015-06-10 at 12:58 -0400, Nicholas Krause wrote:
> 
> On June 10, 2015 12:50:45 PM EDT, "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com> wrote:
> >On Wed, 2015-06-10 at 12:33 -0400, Nicholas Krause wrote:
> >> This makes the function iwl_resume_status_fn return false now if
> >> the received packet of type iwl_rx_packet is not the same size
> >> as the structure pointer, iwl_resume_data's cmd element in order
> >> to signal callers about this error and allow them to handle it
> >> occurrently.
> >> 
> >
> >Hm... Did you actually hit this if?
> >I am not sure I really want to wait here (which is what will happen if
> >you return false) when we get an unexpected length? I do not expect
> >anything besides the response I am waiting for since the firmware is
> >handling the GET_STATUS *only* - it just came back from WoWLAN. Bottom
> >line, this is really an error path and I prefer to exit and not wait
> >for
> >the timeout in that case.
> >But I might be missing something?
> >
> Why not wait for the time out?  Seems there is no reason not to and in that case
> if the firmware handles this

I doubt it will. This goes back to my original question: did you really
hit this path? Does the patch solves a real bug you faced?

> why is the if statement still here. 

To get a debug print? :)

> Nick 
> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> >> ---
> >>  drivers/net/wireless/iwlwifi/dvm/mac80211.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> index 5abd62e..21e808c 100644
> >> --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> @@ -409,7 +409,7 @@ static bool iwl_resume_status_fn(struct
> >iwl_notif_wait_data *notif_wait,
> >>  
> >>  	if (iwl_rx_packet_payload_len(pkt) != sizeof(*resume_data->cmd)) {
> >>  		IWL_ERR(priv, "rx wrong size data\n");
> >> -		return true;
> >> +		return false;
> >>  	}
> >>  	memcpy(resume_data->cmd, pkt->data, sizeof(*resume_data->cmd));
> >>  	resume_data->valid = true;
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] iwlwifi:dvm:Return false if resume command data is not same size as received packet for the function iwl_resume_status_fn

[Grumbach, Emmanuel]

On Wed, 2015-06-10 at 12:58 -0400, Nicholas Krause wrote:
> 
> On June 10, 2015 12:50:45 PM EDT, "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com> wrote:
> >On Wed, 2015-06-10 at 12:33 -0400, Nicholas Krause wrote:
> >> This makes the function iwl_resume_status_fn return false now if
> >> the received packet of type iwl_rx_packet is not the same size
> >> as the structure pointer, iwl_resume_data's cmd element in order
> >> to signal callers about this error and allow them to handle it
> >> occurrently.
> >> 
> >
> >Hm... Did you actually hit this if?
> >I am not sure I really want to wait here (which is what will happen if
> >you return false) when we get an unexpected length? I do not expect
> >anything besides the response I am waiting for since the firmware is
> >handling the GET_STATUS *only* - it just came back from WoWLAN. Bottom
> >line, this is really an error path and I prefer to exit and not wait
> >for
> >the timeout in that case.
> >But I might be missing something?
> >
> Why not wait for the time out?  Seems there is no reason not to and in that case
> if the firmware handles this

I doubt it will. This goes back to my original question: did you really
hit this path? Does the patch solves a real bug you faced?

> why is the if statement still here. 

To get a debug print? :)

> Nick 
> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> >> ---
> >>  drivers/net/wireless/iwlwifi/dvm/mac80211.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> index 5abd62e..21e808c 100644
> >> --- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> +++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
> >> @@ -409,7 +409,7 @@ static bool iwl_resume_status_fn(struct
> >iwl_notif_wait_data *notif_wait,
> >>  
> >>  	if (iwl_rx_packet_payload_len(pkt) != sizeof(*resume_data->cmd)) {
> >>  		IWL_ERR(priv, "rx wrong size data\n");
> >> -		return true;
> >> +		return false;
> >>  	}
> >>  	memcpy(resume_data->cmd, pkt->data, sizeof(*resume_data->cmd));
> >>  	resume_data->valid = true;
>