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

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Sun, Jan 3, 2016 at 11:05 PM, Michael van Elst <mlelstv%serpens.de@localhost> wrote:
> 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

I concur the change.

>  - fixes the destruction of the afdata lock

Heh... I'll commit this separately.

>  - makes the lock a _KERNEL only field to avoid
>    conflicts with kvm(3) users.

I remember that riastradh suggested me that we don't need the
ifdef for kvm(3). IIUC, the added variables aren't accessed
by kvm(3) so that we don't need to hide them to userland.

I'd remove it if nobody objects.

  ozaki-r

>
>  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))
>
>