Bug 197139 - Double cleanup in igb_attach
Summary: Double cleanup in igb_attach
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: i386 Any
: --- Affects Some People
Assignee: Eric Joyner
URL:
Keywords: IntelNetworking, patch
Depends on:
Blocks:
 
Reported: 2015-01-27 21:02 UTC by Sreekanth Rupavatharam
Modified: 2017-08-24 14:04 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sreekanth Rupavatharam 2015-01-27 21:02:22 UTC
igb_attach has this code 

err_late:
        igb_detach(dev);
        igb_free_transmit_structures(adapter);
        igb_free_receive_structures(adapter);
        igb_release_hw_control(adapter);
err_pci:
        igb_free_pci_resources(adapter);
        if (adapter->ifp != NULL)
                if_free(adapter->ifp);
        free(adapter->mta, M_DEVBUF);
        IGB_CORE_LOCK_DESTROY(adapter);

        return (error);

However, I see that igb_detach does all this cleanup when it's successfully completed. Only exception I see is that it return EBUSY if vlantrunk is in use
        /* Make sure VLANS are not using driver */
        if (if_vlantrunkinuse(ifp)) {
                device_printf(dev,"Vlan in use, detach first\n");
                return (EBUSY);
        }

This results in duplicate cleanup in igb_attach and can cause crashes. 
The fix would be the following. 
Index: if_igb.c
===================================================================
--- if_igb.c	(revision 298053)
+++ if_igb.c	(working copy)
@@ -723,7 +723,8 @@ igb_attach(device_t dev)
 	return (0);
 
 err_late:
-	igb_detach(dev);
+	if(igb_detach(dev) == 0) /* igb_detach did the cleanup */
+		return(error); 
 	igb_free_transmit_structures(adapter);
 	igb_free_receive_structures(adapter);
 	igb_release_hw_control(adapter);


The issue exists in stable/10 as well as current.
Comment 1 Sean Bruno freebsd_committer 2015-08-04 15:25:35 UTC
This is still applicable.
Comment 2 commit-hook freebsd_committer 2016-05-06 17:01:00 UTC
A commit references this bug:

Author: sbruno
Date: Fri May  6 17:00:45 UTC 2016
New revision: 299188
URL: https://svnweb.freebsd.org/changeset/base/299188

Log:
  Since igb_detach() cleans up all the data structures that will be
  free'd by the functions following its call, we can simply return instead
  of crashing and burning in the event of igb_detach() failing.

  PR:		197139
  Submitted by:	rupavath@juniper.net
  MFC after:	2 weeks

Changes:
  head/sys/dev/e1000/if_igb.c
Comment 3 commit-hook freebsd_committer 2016-07-22 03:20:04 UTC
A commit references this bug:

Author: sbruno
Date: Fri Jul 22 03:19:50 UTC 2016
New revision: 303175
URL: https://svnweb.freebsd.org/changeset/base/303175

Log:
  MFC r299188
  Since igb_detach() cleans up all the data structures that will be
  free'd by the functions following its call, we can simply return instead
  of crashing and burning in the event of igb_detach() failing.

  PR:		197139

Changes:
  stable/10/sys/dev/e1000/if_igb.c
Comment 4 Mark Linimon freebsd_committer freebsd_triage 2017-08-24 14:04:09 UTC
Committed back in 2016.