Re: kern/50602: removing ethernet cable panics the kernel

[Michael van Elst <mlelstv%serpens.de@localhost>]

The following reply was made to PR kern/50602; it has been noted by GNATS.

From: Michael van Elst <mlelstv%serpens.de@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: kern/50602: removing ethernet cable panics the kernel
Date: Sun, 3 Jan 2016 15:01:54 +0100

 Maybe this.
 
 The patch
 
 - defers link state handling to a softint
 - fixes the destruction of the afdata lock
 - makes the lock a _KERNEL only field to avoid
   conflicts with kvm(3) users.
 
 Index: sys/net/if.c
 ===================================================================
 RCS file: /cvsroot/src/sys/net/if.c,v
 retrieving revision 1.319
 diff -p -u -r1.319 if.c
 --- sys/net/if.c	20 Nov 2015 08:10:36 -0000	1.319
 +++ sys/net/if.c	3 Jan 2016 13:55:38 -0000
 @@ -198,6 +198,7 @@ static void if_attachdomain1(struct ifne
  static int ifconf(u_long, void *);
  static int if_clone_create(const char *);
  static int if_clone_destroy(const char *);
 +static void if_link_state_change_si(void *);
  
  #if defined(INET) || defined(INET6)
  static void sysctl_net_pktq_setup(struct sysctllog **, int);
 @@ -592,6 +593,7 @@ if_initialize(ifnet_t *ifp)
  	ifp->if_broadcastaddr = 0; /* reliably crash if used uninitialized */
  
  	ifp->if_link_state = LINK_STATE_UNKNOWN;
 +	ifp->if_old_link_state = LINK_STATE_UNKNOWN;
  
  	ifp->if_capenable = 0;
  	ifp->if_csum_flags_tx = 0;
 @@ -617,6 +619,10 @@ if_initialize(ifnet_t *ifp)
  
  	IF_AFDATA_LOCK_INIT(ifp);
  
 +	ifp->if_link_si = softint_establish(SOFTINT_NET, if_link_state_change_si, ifp);
 +	if (ifp->if_link_si == NULL)
 +		panic("%s: softint_establish() failed", __func__);
 +
  	if_getindex(ifp);
  }
  
 @@ -895,6 +901,11 @@ again:
  
  	ifioctl_detach(ifp);
  
 +	IF_AFDATA_LOCK_DESTROY(ifp);
 +
 +	softint_disestablish(ifp->if_link_si);
 +	ifp->if_link_si = NULL;
 +
  	/*
  	 * remove packets that came from ifp, from software interrupt queues.
  	 */
 @@ -1407,8 +1418,6 @@ void
  if_link_state_change(struct ifnet *ifp, int link_state)
  {
  	int s;
 -	int old_link_state;
 -	struct domain *dp;
  
  	s = splnet();
  	if (ifp->if_link_state == link_state) {
 @@ -1416,8 +1425,26 @@ if_link_state_change(struct ifnet *ifp, 
  		return;
  	}
  
 -	old_link_state = ifp->if_link_state;
  	ifp->if_link_state = link_state;
 +	softint_schedule(ifp->if_link_si);
 +
 +	splx(s);
 +}
 +
 +
 +void
 +if_link_state_change_si(void *arg)
 +{
 +	struct ifnet *ifp = arg;
 +	int s;
 +	int link_state, old_link_state;
 +	struct domain *dp;
 +
 +	s = splnet();
 +	link_state = ifp->if_link_state;
 +	old_link_state = ifp->if_old_link_state;
 +	ifp->if_old_link_state = ifp->if_link_state;
 +
  #ifdef DEBUG
  	log(LOG_DEBUG, "%s: link state %s (was %s)\n", ifp->if_xname,
  		link_state == LINK_STATE_UP ? "UP" :
 Index: sys/net/if.h
 ===================================================================
 RCS file: /cvsroot/src/sys/net/if.h,v
 retrieving revision 1.193
 diff -p -u -r1.193 if.h
 --- sys/net/if.h	2 Oct 2015 03:08:26 -0000	1.193
 +++ sys/net/if.h	3 Jan 2016 13:55:39 -0000
 @@ -351,12 +351,14 @@ typedef struct ifnet {
  	struct ifnet_lock *if_ioctl_lock;
  #ifdef _KERNEL /* XXX kvm(3) */
  	struct callout *if_slowtimo_ch;
 -#endif
  #ifdef GATEWAY
  	struct kmutex	*if_afdata_lock;
  #else
  	struct krwlock	*if_afdata_lock;
  #endif
 +	void *if_link_si;		/* softint to handle link state changes */
 +	int if_old_link_state;		/* previous link state */
 +#endif
  } ifnet_t;
   
  #define	if_mtu		if_data.ifi_mtu
 @@ -452,6 +454,8 @@ typedef struct ifnet {
  		(ifp)->if_afdata_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_NET); \
  	} while (0)
  
 +#define	IF_AFDATA_LOCK_DESTROY(ifp)	mutex_obj_free((ifp)->if_afdata_lock)
 +
  #define	IF_AFDATA_WLOCK(ifp)	mutex_enter((ifp)->if_afdata_lock)
  #define	IF_AFDATA_RLOCK(ifp)	mutex_enter((ifp)->if_afdata_lock)
  #define	IF_AFDATA_WUNLOCK(ifp)	mutex_exit((ifp)->if_afdata_lock)
 @@ -459,7 +463,6 @@ typedef struct ifnet {
  #define	IF_AFDATA_LOCK(ifp)	IF_AFDATA_WLOCK(ifp)
  #define	IF_AFDATA_UNLOCK(ifp)	IF_AFDATA_WUNLOCK(ifp)
  #define	IF_AFDATA_TRYLOCK(ifp)	mutex_tryenter((ifp)->if_afdata_lock)
 -#define	IF_AFDATA_DESTROY(ifp)	mutex_destroy((ifp)->if_afdata_lock)
  
  #define	IF_AFDATA_LOCK_ASSERT(ifp)	\
  	KASSERT(mutex_owned((ifp)->if_afdata_lock))
 @@ -474,6 +477,8 @@ typedef struct ifnet {
  #define	IF_AFDATA_LOCK_INIT(ifp)	\
  	do {(ifp)->if_afdata_lock = rw_obj_alloc();} while (0)
  
 +#define	IF_AFDATA_LOCK_DESTROY(ifp)	rw_obj_free((ifp)->if_afdata_lock)
 +
  #define	IF_AFDATA_WLOCK(ifp)	rw_enter((ifp)->if_afdata_lock, RW_WRITER)
  #define	IF_AFDATA_RLOCK(ifp)	rw_enter((ifp)->if_afdata_lock, RW_READER)
  #define	IF_AFDATA_WUNLOCK(ifp)	rw_exit((ifp)->if_afdata_lock)
 @@ -481,7 +486,6 @@ typedef struct ifnet {
  #define	IF_AFDATA_LOCK(ifp)	IF_AFDATA_WLOCK(ifp)
  #define	IF_AFDATA_UNLOCK(ifp)	IF_AFDATA_WUNLOCK(ifp)
  #define	IF_AFDATA_TRYLOCK(ifp)	rw_tryenter((ifp)->if_afdata_lock, RW_WRITER)
 -#define	IF_AFDATA_DESTROY(ifp)	rw_destroy((ifp)->if_afdata_lock)
  
  #define	IF_AFDATA_LOCK_ASSERT(ifp)	\
  	KASSERT(rw_lock_held((ifp)->if_afdata_lock))