Message ID | 1376419513-31924-1-git-send-email-nsujir@broadcom.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: "Nithin Nayak Sujir" <nsujir@broadcom.com> Date: Tue, 13 Aug 2013 11:45:13 -0700 > From: Daniel Borkmann <dborkman@redhat.com> > > Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible > NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev) > condition is met and netdev is NULL. Then, the jump to the 'done' label > calls dev_close() with a netdevice that is NULL. Therefore, only call > dev_close() when we have a netdevice, but one that is not running. > > [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin > Nayak Sujir ] > > Reported-by: Dave Jones <davej@redhat.com> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > Cc: Gavin Shan <shangw@linux.vnet.ibm.com> > Cc: Michael Chan <mchan@broadcom.com> > Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com> Can I get some reviews from the Broadcom folks? Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 15, 2013 at 11:07 AM, David Miller <davem@davemloft.net> wrote: > > From: "Nithin Nayak Sujir" <nsujir@broadcom.com> > Date: Tue, 13 Aug 2013 11:45:13 -0700 > > > From: Daniel Borkmann <dborkman@redhat.com> > > > > Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible > > NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev) > > condition is met and netdev is NULL. Then, the jump to the 'done' label > > calls dev_close() with a netdevice that is NULL. Therefore, only call > > dev_close() when we have a netdevice, but one that is not running. > > > > [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin > > Nayak Sujir ] > > > > Reported-by: Dave Jones <davej@redhat.com> > > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > > Cc: Gavin Shan <shangw@linux.vnet.ibm.com> > > Cc: Michael Chan <mchan@broadcom.com> > > Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com> > > Can I get some reviews from the Broadcom folks? > > Thanks. You already have: Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-08-15 at 01:07 -0700, David Miller wrote: > From: "Nithin Nayak Sujir" <nsujir@broadcom.com> > Date: Tue, 13 Aug 2013 11:45:13 -0700 > > > From: Daniel Borkmann <dborkman@redhat.com> > > > > Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible > > NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev) > > condition is met and netdev is NULL. Then, the jump to the 'done' label > > calls dev_close() with a netdevice that is NULL. Therefore, only call > > dev_close() when we have a netdevice, but one that is not running. > > > > [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin > > Nayak Sujir ] > > > > Reported-by: Dave Jones <davej@redhat.com> > > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > > Cc: Gavin Shan <shangw@linux.vnet.ibm.com> > > Cc: Michael Chan <mchan@broadcom.com> > > Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com> > > Can I get some reviews from the Broadcom folks? > We worked with Daniel at Red Hat and Gavin at IBM on this patch. Thanks. Signed-off-by: Michael Chan <mchan@broadcom.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Cc: Gavin Shan <shangw@linux.vnet.ibm.com> >> Cc: Michael Chan <mchan@broadcom.com> >> Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com> > > Can I get some reviews from the Broadcom folks? > David, I made the additional changes to Daniel's patch and Michael Chan reviewed it before I submitted. Sorry, I forgot to add Michael's Signed-off-by. Do you want me to resend it with his Signed-off-by added? Thanks, Nithin. > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Nithin Nayak Sujir" <nsujir@broadcom.com> Date: Thu, 15 Aug 2013 10:32:58 -0700 > Sorry, I forgot to add Michael's Signed-off-by. Do you want me to > resend it with his Signed-off-by added? That's not necessary, it gets added automatically by patchwork since he replied and added it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Michael Chan" <mchan@broadcom.com> Date: Thu, 15 Aug 2013 08:15:20 -0700 > On Thu, 2013-08-15 at 01:07 -0700, David Miller wrote: >> From: "Nithin Nayak Sujir" <nsujir@broadcom.com> >> Date: Tue, 13 Aug 2013 11:45:13 -0700 >> >> > From: Daniel Borkmann <dborkman@redhat.com> >> > >> > Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible >> > NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev) >> > condition is met and netdev is NULL. Then, the jump to the 'done' label >> > calls dev_close() with a netdevice that is NULL. Therefore, only call >> > dev_close() when we have a netdevice, but one that is not running. >> > >> > [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin >> > Nayak Sujir ] >> > >> > Reported-by: Dave Jones <davej@redhat.com> >> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> >> > Cc: Gavin Shan <shangw@linux.vnet.ibm.com> >> > Cc: Michael Chan <mchan@broadcom.com> >> > Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com> >> >> Can I get some reviews from the Broadcom folks? >> > > We worked with Daniel at Red Hat and Gavin at IBM on this patch. > Thanks. > > Signed-off-by: Michael Chan <mchan@broadcom.com> Thanks for the info, applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index ddebc7a..0da2214 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -17796,8 +17796,10 @@ static pci_ers_result_t tg3_io_error_detected(struct pci_dev *pdev, done: if (state == pci_channel_io_perm_failure) { - tg3_napi_enable(tp); - dev_close(netdev); + if (netdev) { + tg3_napi_enable(tp); + dev_close(netdev); + } err = PCI_ERS_RESULT_DISCONNECT; } else { pci_disable_device(pdev); @@ -17827,7 +17829,8 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev) rtnl_lock(); if (pci_enable_device(pdev)) { - netdev_err(netdev, "Cannot re-enable PCI device after reset.\n"); + dev_err(&pdev->dev, + "Cannot re-enable PCI device after reset.\n"); goto done; } @@ -17835,7 +17838,7 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev) pci_restore_state(pdev); pci_save_state(pdev); - if (!netif_running(netdev)) { + if (!netdev || !netif_running(netdev)) { rc = PCI_ERS_RESULT_RECOVERED; goto done; } @@ -17847,7 +17850,7 @@ static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev) rc = PCI_ERS_RESULT_RECOVERED; done: - if (rc != PCI_ERS_RESULT_RECOVERED && netif_running(netdev)) { + if (rc != PCI_ERS_RESULT_RECOVERED && netdev && netif_running(netdev)) { tg3_napi_enable(tp); dev_close(netdev); }