| Message ID | 20190815052116.23430-2-po-hsu.lin@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | p54: fix crash during initialization | expand |
On 8/14/19 10:21 PM, Po-Hsu Lin wrote: > From: Christian Lamparter <chunkeey@gmail.com> > > BugLink: https://bugs.launchpad.net/bugs/1839693 > > This patch fixes a crash that got introduced when the > mentioned patch replaced the direct list_head access > with skb_peek_tail(). When the device is starting up, > there are no entries in the queue, so previously to > "Use skb_peek_tail() instead..." the target_skb would > end up as the tail and head pointer which then could > be used by __skb_queue_after to fill the empty queue. > > With skb_peek_tail() in its place will instead just > return NULL which then causes a crash in the > __skb_queue_after(). > > | BUG: unable to handle kernel NULL pointer dereference at 000000 > | #PF error: [normal kernel read fault] > | PGD 0 P4D 0 > | Oops: 0000 [#1] SMP PTI > | CPU: 0 PID: 12 Comm: kworker/0:1 Tainted: GO 5.1.0-rc7-wt+ #218 > | Hardware name: MSI MS-7816/Z87-G43 (MS-7816), BIOS V1.11 05/09/2015 > | Workqueue: events request_firmware_work_func > | RIP: 0010:p54_tx_pending+0x10f/0x1b0 [p54common] > | Code: 78 06 80 78 28 00 74 6d <48> 8b 07 49 89 7c 24 08 49 89 04 24 4 > | RSP: 0018:ffffa81c81927d90 EFLAGS: 00010086 > | RAX: ffff9bbaaf131048 RBX: 0000000000020670 RCX: 0000000000020264 > | RDX: ffff9bbaa976d660 RSI: 0000000000000202 RDI: 0000000000000000 > | RBP: ffff9bbaa976d620 R08: 00000000000006c0 R09: ffff9bbaa976d660 > | R10: 0000000000000000 R11: ffffe8480dbc5900 R12: ffff9bbb45e87700 > | R13: ffff9bbaa976d648 R14: ffff9bbaa976d674 R15: ffff9bbaaf131048 > | FS: 0000000000000000(0000) GS:ffff9bbb5ec00000(0000) knlGS:00000 > | CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > | CR2: 0000000000000000 CR3: 00000003695fc003 CR4: 00000000001606f0 > | Call Trace: > | p54_download_eeprom+0xbe/0x120 [p54common] > | p54_read_eeprom+0x7f/0xc0 [p54common] > | p54u_load_firmware_cb+0xe0/0x160 [p54usb] > | request_firmware_work_func+0x42/0x80 > | process_one_work+0x1f5/0x3f0 > | worker_thread+0x28/0x3c0 > > Cc: stable@vger.kernel.org > Fixes: e3554197fc8f ("p54: Use skb_peek_tail() instead of direct head pointer accesses.") > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > (cherry picked from commit 1645ab931998b39aed5761f095956f0b10a6362f) > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com> > --- Clean cherry pick & positive test results. Acked-by: Connor Kuehl <connor.kuehl@canonical.com> > drivers/net/wireless/intersil/p54/txrx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c > index 79078456..5bf1c19 100644 > --- a/drivers/net/wireless/intersil/p54/txrx.c > +++ b/drivers/net/wireless/intersil/p54/txrx.c > @@ -142,7 +142,10 @@ static int p54_assign_address(struct p54_common *priv, struct sk_buff *skb) > unlikely(GET_HW_QUEUE(skb) == P54_QUEUE_BEACON)) > priv->beacon_req_id = data->req_id; > > - __skb_queue_after(&priv->tx_queue, target_skb, skb); > + if (target_skb) > + __skb_queue_after(&priv->tx_queue, target_skb, skb); > + else > + __skb_queue_head(&priv->tx_queue, skb); > spin_unlock_irqrestore(&priv->tx_queue.lock, flags); > return 0; > } >
On 15.08.19 07:21, Po-Hsu Lin wrote: > From: Christian Lamparter <chunkeey@gmail.com> > > BugLink: https://bugs.launchpad.net/bugs/1839693 > > This patch fixes a crash that got introduced when the > mentioned patch replaced the direct list_head access > with skb_peek_tail(). When the device is starting up, > there are no entries in the queue, so previously to > "Use skb_peek_tail() instead..." the target_skb would > end up as the tail and head pointer which then could > be used by __skb_queue_after to fill the empty queue. > > With skb_peek_tail() in its place will instead just > return NULL which then causes a crash in the > __skb_queue_after(). > > | BUG: unable to handle kernel NULL pointer dereference at 000000 > | #PF error: [normal kernel read fault] > | PGD 0 P4D 0 > | Oops: 0000 [#1] SMP PTI > | CPU: 0 PID: 12 Comm: kworker/0:1 Tainted: GO 5.1.0-rc7-wt+ #218 > | Hardware name: MSI MS-7816/Z87-G43 (MS-7816), BIOS V1.11 05/09/2015 > | Workqueue: events request_firmware_work_func > | RIP: 0010:p54_tx_pending+0x10f/0x1b0 [p54common] > | Code: 78 06 80 78 28 00 74 6d <48> 8b 07 49 89 7c 24 08 49 89 04 24 4 > | RSP: 0018:ffffa81c81927d90 EFLAGS: 00010086 > | RAX: ffff9bbaaf131048 RBX: 0000000000020670 RCX: 0000000000020264 > | RDX: ffff9bbaa976d660 RSI: 0000000000000202 RDI: 0000000000000000 > | RBP: ffff9bbaa976d620 R08: 00000000000006c0 R09: ffff9bbaa976d660 > | R10: 0000000000000000 R11: ffffe8480dbc5900 R12: ffff9bbb45e87700 > | R13: ffff9bbaa976d648 R14: ffff9bbaa976d674 R15: ffff9bbaaf131048 > | FS: 0000000000000000(0000) GS:ffff9bbb5ec00000(0000) knlGS:00000 > | CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > | CR2: 0000000000000000 CR3: 00000003695fc003 CR4: 00000000001606f0 > | Call Trace: > | p54_download_eeprom+0xbe/0x120 [p54common] > | p54_read_eeprom+0x7f/0xc0 [p54common] > | p54u_load_firmware_cb+0xe0/0x160 [p54usb] > | request_firmware_work_func+0x42/0x80 > | process_one_work+0x1f5/0x3f0 > | worker_thread+0x28/0x3c0 > > Cc: stable@vger.kernel.org > Fixes: e3554197fc8f ("p54: Use skb_peek_tail() instead of direct head pointer accesses.") > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > (cherry picked from commit 1645ab931998b39aed5761f095956f0b10a6362f) > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/net/wireless/intersil/p54/txrx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c > index 79078456..5bf1c19 100644 > --- a/drivers/net/wireless/intersil/p54/txrx.c > +++ b/drivers/net/wireless/intersil/p54/txrx.c > @@ -142,7 +142,10 @@ static int p54_assign_address(struct p54_common *priv, struct sk_buff *skb) > unlikely(GET_HW_QUEUE(skb) == P54_QUEUE_BEACON)) > priv->beacon_req_id = data->req_id; > > - __skb_queue_after(&priv->tx_queue, target_skb, skb); > + if (target_skb) > + __skb_queue_after(&priv->tx_queue, target_skb, skb); > + else > + __skb_queue_head(&priv->tx_queue, skb); > spin_unlock_irqrestore(&priv->tx_queue.lock, flags); > return 0; > } >
On 8/15/19 7:21 AM, Po-Hsu Lin wrote: > From: Christian Lamparter <chunkeey@gmail.com> > > BugLink: https://bugs.launchpad.net/bugs/1839693 > > This patch fixes a crash that got introduced when the > mentioned patch replaced the direct list_head access > with skb_peek_tail(). When the device is starting up, > there are no entries in the queue, so previously to > "Use skb_peek_tail() instead..." the target_skb would > end up as the tail and head pointer which then could > be used by __skb_queue_after to fill the empty queue. > > With skb_peek_tail() in its place will instead just > return NULL which then causes a crash in the > __skb_queue_after(). > > | BUG: unable to handle kernel NULL pointer dereference at 000000 > | #PF error: [normal kernel read fault] > | PGD 0 P4D 0 > | Oops: 0000 [#1] SMP PTI > | CPU: 0 PID: 12 Comm: kworker/0:1 Tainted: GO 5.1.0-rc7-wt+ #218 > | Hardware name: MSI MS-7816/Z87-G43 (MS-7816), BIOS V1.11 05/09/2015 > | Workqueue: events request_firmware_work_func > | RIP: 0010:p54_tx_pending+0x10f/0x1b0 [p54common] > | Code: 78 06 80 78 28 00 74 6d <48> 8b 07 49 89 7c 24 08 49 89 04 24 4 > | RSP: 0018:ffffa81c81927d90 EFLAGS: 00010086 > | RAX: ffff9bbaaf131048 RBX: 0000000000020670 RCX: 0000000000020264 > | RDX: ffff9bbaa976d660 RSI: 0000000000000202 RDI: 0000000000000000 > | RBP: ffff9bbaa976d620 R08: 00000000000006c0 R09: ffff9bbaa976d660 > | R10: 0000000000000000 R11: ffffe8480dbc5900 R12: ffff9bbb45e87700 > | R13: ffff9bbaa976d648 R14: ffff9bbaa976d674 R15: ffff9bbaaf131048 > | FS: 0000000000000000(0000) GS:ffff9bbb5ec00000(0000) knlGS:00000 > | CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > | CR2: 0000000000000000 CR3: 00000003695fc003 CR4: 00000000001606f0 > | Call Trace: > | p54_download_eeprom+0xbe/0x120 [p54common] > | p54_read_eeprom+0x7f/0xc0 [p54common] > | p54u_load_firmware_cb+0xe0/0x160 [p54usb] > | request_firmware_work_func+0x42/0x80 > | process_one_work+0x1f5/0x3f0 > | worker_thread+0x28/0x3c0 > > Cc: stable@vger.kernel.org > Fixes: e3554197fc8f ("p54: Use skb_peek_tail() instead of direct head pointer accesses.") > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > (cherry picked from commit 1645ab931998b39aed5761f095956f0b10a6362f) > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com> > --- > drivers/net/wireless/intersil/p54/txrx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c > index 79078456..5bf1c19 100644 > --- a/drivers/net/wireless/intersil/p54/txrx.c > +++ b/drivers/net/wireless/intersil/p54/txrx.c > @@ -142,7 +142,10 @@ static int p54_assign_address(struct p54_common *priv, struct sk_buff *skb) > unlikely(GET_HW_QUEUE(skb) == P54_QUEUE_BEACON)) > priv->beacon_req_id = data->req_id; > > - __skb_queue_after(&priv->tx_queue, target_skb, skb); > + if (target_skb) > + __skb_queue_after(&priv->tx_queue, target_skb, skb); > + else > + __skb_queue_head(&priv->tx_queue, skb); > spin_unlock_irqrestore(&priv->tx_queue.lock, flags); > return 0; > } > Applied to disco/master-next branch. Thanks, Kleber
diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c index 79078456..5bf1c19 100644 --- a/drivers/net/wireless/intersil/p54/txrx.c +++ b/drivers/net/wireless/intersil/p54/txrx.c @@ -142,7 +142,10 @@ static int p54_assign_address(struct p54_common *priv, struct sk_buff *skb) unlikely(GET_HW_QUEUE(skb) == P54_QUEUE_BEACON)) priv->beacon_req_id = data->req_id; - __skb_queue_after(&priv->tx_queue, target_skb, skb); + if (target_skb) + __skb_queue_after(&priv->tx_queue, target_skb, skb); + else + __skb_queue_head(&priv->tx_queue, skb); spin_unlock_irqrestore(&priv->tx_queue.lock, flags); return 0; }