tun/tap vs splnet()

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

tun/tap vs splnet()

Martin Pieuchot
tun(4) and tap(4) are pseudo-drivers, they don't need splnet().

The global lists of interfaces are only used inside the driver in code
path protected by the KERNEL_LOCK().

ok?

Index: net/if_tun.c
===================================================================
RCS file: /cvs/src/sys/net/if_tun.c,v
retrieving revision 1.173
diff -u -p -r1.173 if_tun.c
--- net/if_tun.c 24 Jan 2017 10:08:30 -0000 1.173
+++ net/if_tun.c 26 May 2017 14:01:35 -0000
@@ -192,7 +192,6 @@ tun_create(struct if_clone *ifc, int uni
 {
  struct tun_softc *tp;
  struct ifnet *ifp;
- int s;
 
  tp = malloc(sizeof(*tp), M_DEVBUF, M_NOWAIT|M_ZERO);
  if (tp == NULL)
@@ -226,9 +225,7 @@ tun_create(struct if_clone *ifc, int uni
 #if NBPFILTER > 0
  bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(u_int32_t));
 #endif
- s = splnet();
  LIST_INSERT_HEAD(&tun_softc_list, tp, entry);
- splx(s);
  } else {
  tp->tun_flags |= TUN_LAYER2;
  ether_fakeaddr(ifp);
@@ -239,9 +236,7 @@ tun_create(struct if_clone *ifc, int uni
  if_attach(ifp);
  ether_ifattach(ifp);
 
- s = splnet();
  LIST_INSERT_HEAD(&tap_softc_list, tp, entry);
- splx(s);
  }
 
 #ifdef PIPEX
@@ -269,9 +264,7 @@ tun_clone_destroy(struct ifnet *ifp)
  klist_invalidate(&tp->tun_wsel.si_note);
  splx(s);
 
- s = splnet();
  LIST_REMOVE(tp, entry);
- splx(s);
 
  if (tp->tun_flags & TUN_LAYER2)
  ether_ifdetach(ifp);
@@ -362,7 +355,6 @@ int
 tun_dev_open(struct tun_softc *tp, int flag, int mode, struct proc *p)
 {
  struct ifnet *ifp;
- int s;
 
  if (tp->tun_flags & TUN_OPEN)
  return (EBUSY);
@@ -373,10 +365,8 @@ tun_dev_open(struct tun_softc *tp, int f
  tp->tun_flags |= TUN_NBIO;
 
  /* automatically mark the interface running on open */
- s = splnet();
  ifp->if_flags |= IFF_RUNNING;
  tun_link_state(tp);
- splx(s);
 
  TUNDEBUG(("%s: open\n", ifp->if_xname));
  return (0);
@@ -418,11 +408,9 @@ tun_dev_close(struct tun_softc *tp, int
  /*
  * junk all pending output
  */
- s = splnet();
  ifp->if_flags &= ~IFF_RUNNING;
  tun_link_state(tp);
  IFQ_PURGE(&ifp->if_snd);
- splx(s);
 
  TUNDEBUG(("%s: closed\n", ifp->if_xname));
 
@@ -501,9 +489,7 @@ tun_ioctl(struct ifnet *ifp, u_long cmd,
 {
  struct tun_softc *tp = (struct tun_softc *)(ifp->if_softc);
  struct ifreq *ifr = (struct ifreq *)data;
- int error = 0, s;
-
- s = splnet();
+ int error = 0;
 
  switch (cmd) {
  case SIOCSIFADDR:
@@ -531,7 +517,6 @@ tun_ioctl(struct ifnet *ifp, u_long cmd,
  error = ENOTTY;
  }
 
- splx(s);
  return (error);
 }
 
@@ -569,8 +554,6 @@ tun_output(struct ifnet *ifp, struct mbu
  af = mtod(m0, u_int32_t *);
  *af = htonl(dst->sa_family);
 
- s = splnet();
-
 #if NBPFILTER > 0
  if (ifp->if_bpf)
  bpf_mtap(ifp->if_bpf, m0, BPF_DIRECTION_OUT);
@@ -584,7 +567,6 @@ tun_output(struct ifnet *ifp, struct mbu
 #endif
 
  error = if_enqueue(ifp, m0);
- splx(s);
 
  if (error) {
  ifp->if_collisions++;
@@ -635,18 +617,14 @@ int
 tun_dev_ioctl(struct tun_softc *tp, u_long cmd, caddr_t data, int flag,
     struct proc *p)
 {
- int s;
  struct tuninfo *tunp;
  struct mbuf *m;
 
- s = splnet();
  switch (cmd) {
  case TUNSIFINFO:
  tunp = (struct tuninfo *)data;
- if (tunp->mtu < ETHERMIN || tunp->mtu > TUNMRU) {
- splx(s);
+ if (tunp->mtu < ETHERMIN || tunp->mtu > TUNMRU)
  return (EINVAL);
- }
  tp->tun_if.if_mtu = tunp->mtu;
  tp->tun_if.if_type = tunp->type;
  tp->tun_if.if_flags =
@@ -677,7 +655,6 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
  tp->tun_if.if_flags |= *(int *)data & TUN_IFF_FLAGS;
  break;
  default:
- splx(s);
  return (EINVAL);
  }
  break;
@@ -711,19 +688,15 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
  *(int *)data = tp->tun_pgid;
  break;
  case SIOCGIFADDR:
- if (!(tp->tun_flags & TUN_LAYER2)) {
- splx(s);
+ if (!(tp->tun_flags & TUN_LAYER2))
  return (EINVAL);
- }
  bcopy(tp->arpcom.ac_enaddr, data,
     sizeof(tp->arpcom.ac_enaddr));
  break;
 
  case SIOCSIFADDR:
- if (!(tp->tun_flags & TUN_LAYER2)) {
- splx(s);
+ if (!(tp->tun_flags & TUN_LAYER2))
  return (EINVAL);
- }
  bcopy(data, tp->arpcom.ac_enaddr,
     sizeof(tp->arpcom.ac_enaddr));
  break;
@@ -732,14 +705,11 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
  if (!(tp->tun_flags & TUN_LAYER2)) {
  int ret;
  ret = pipex_ioctl(&tp->pipex_iface, cmd, data);
- splx(s);
  return (ret);
  }
 #endif
- splx(s);
  return (ENOTTY);
  }
- splx(s);
  return (0);
 }
 
@@ -782,7 +752,6 @@ tun_dev_read(struct tun_softc *tp, struc
  ifidx = ifp->if_index;
  tp->tun_flags &= ~TUN_RWAIT;
 
- s = splnet();
  do {
  struct ifnet *ifp1;
  int destroyed;
@@ -883,9 +852,7 @@ tun_dev_write(struct tun_softc *tp, stru
  struct mbuf *top, **mp, *m;
  int error = 0, tlen;
  size_t mlen;
-#if NBPFILTER > 0
- int s;
-#endif
+
  ifp = &tp->tun_if;
  TUNDEBUG(("%s: tunwrite\n", ifp->if_xname));
 
@@ -961,9 +928,7 @@ tun_dev_write(struct tun_softc *tp, stru
 
 #if NBPFILTER > 0
  if (ifp->if_bpf) {
- s = splnet();
  bpf_mtap(ifp->if_bpf, top, BPF_DIRECTION_IN);
- splx(s);
  }
 #endif
 
@@ -1028,13 +993,12 @@ tappoll(dev_t dev, int events, struct pr
 int
 tun_dev_poll(struct tun_softc *tp, int events, struct proc *p)
 {
- int revents, s;
+ int revents;
  struct ifnet *ifp;
  unsigned int len;
 
  ifp = &tp->tun_if;
  revents = 0;
- s = splnet();
  TUNDEBUG(("%s: tunpoll\n", ifp->if_xname));
 
  if (events & (POLLIN | POLLRDNORM)) {
@@ -1049,7 +1013,6 @@ tun_dev_poll(struct tun_softc *tp, int e
  }
  if (events & (POLLOUT | POLLWRNORM))
  revents |= events & (POLLOUT | POLLWRNORM);
- splx(s);
  return (revents);
 }
 
@@ -1143,7 +1106,6 @@ filt_tunread(struct knote *kn, long hint
  tp = (struct tun_softc *)kn->kn_hook;
  ifp = &tp->tun_if;
 
- s = splnet();
  len = IFQ_LEN(&ifp->if_snd);
  if (len > 0) {
  splx(s);
@@ -1153,7 +1115,6 @@ filt_tunread(struct knote *kn, long hint
     IFQ_LEN(&ifp->if_snd)));
  return (1);
  }
- splx(s);
  TUNDEBUG(("%s: tunkqread waiting\n", ifp->if_xname));
  return (0);
 }

Reply | Threaded
Open this post in threaded view
|

Re: tun/tap vs splnet()

Alexander Bluhm
On Fri, May 26, 2017 at 04:06:43PM +0200, Martin Pieuchot wrote:
> @@ -362,7 +355,6 @@ int
>  tun_dev_open(struct tun_softc *tp, int flag, int mode, struct proc *p)
>  {
>   struct ifnet *ifp;
> - int s;
>  
>   if (tp->tun_flags & TUN_OPEN)
>   return (EBUSY);

I wonder wether these device functions should grab the net lock.

> @@ -569,8 +554,6 @@ tun_output(struct ifnet *ifp, struct mbu
>   af = mtod(m0, u_int32_t *);
>   *af = htonl(dst->sa_family);
>  
> - s = splnet();
> -
>  #if NBPFILTER > 0
>   if (ifp->if_bpf)
>   bpf_mtap(ifp->if_bpf, m0, BPF_DIRECTION_OUT);
> @@ -584,7 +567,6 @@ tun_output(struct ifnet *ifp, struct mbu
>  #endif
>  
>   error = if_enqueue(ifp, m0);
> - splx(s);
>  
>   if (error) {
>   ifp->if_collisions++;

There is a forgotten splx(s) after pipex_output().  Is it safe to
call pipex_output() without splnet()?  There is a lot of splnet()
protection in pipex.

> @@ -677,7 +655,6 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
>   tp->tun_if.if_flags |= *(int *)data & TUN_IFF_FLAGS;
>   break;
>   default:
> - splx(s);
>   return (EINVAL);
>   }
>   break;

Is kernel lock enough to access if_flags?  Should there be netlock, too?

> @@ -732,14 +705,11 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
>   if (!(tp->tun_flags & TUN_LAYER2)) {
>   int ret;
>   ret = pipex_ioctl(&tp->pipex_iface, cmd, data);
> - splx(s);
>   return (ret);
>   }
>  #endif

pipex_ioctl() has a comment /* called from tunioctl() with splnet() */

> @@ -782,7 +752,6 @@ tun_dev_read(struct tun_softc *tp, struc
>   ifidx = ifp->if_index;
>   tp->tun_flags &= ~TUN_RWAIT;
>  
> - s = splnet();
>   do {
>   struct ifnet *ifp1;
>   int destroyed;

You forgot to remove the splx().

> @@ -1143,7 +1106,6 @@ filt_tunread(struct knote *kn, long hint
>   tp = (struct tun_softc *)kn->kn_hook;
>   ifp = &tp->tun_if;
>  
> - s = splnet();
>   len = IFQ_LEN(&ifp->if_snd);
>   if (len > 0) {
>   splx(s);

Remove the splx().

bluhm

Reply | Threaded
Open this post in threaded view
|

Re: tun/tap vs splnet()

Martin Pieuchot
On 26/05/17(Fri) 17:43, Alexander Bluhm wrote:

> On Fri, May 26, 2017 at 04:06:43PM +0200, Martin Pieuchot wrote:
> > @@ -362,7 +355,6 @@ int
> >  tun_dev_open(struct tun_softc *tp, int flag, int mode, struct proc *p)
> >  {
> >   struct ifnet *ifp;
> > - int s;
> >  
> >   if (tp->tun_flags & TUN_OPEN)
> >   return (EBUSY);
>
> I wonder wether these device functions should grab the net lock.

As long as the ioctl path is executed under KERNEL_LOCK() it's not
necessary.

> > @@ -569,8 +554,6 @@ tun_output(struct ifnet *ifp, struct mbu
> >   af = mtod(m0, u_int32_t *);
> >   *af = htonl(dst->sa_family);
> >  
> > - s = splnet();
> > -
> >  #if NBPFILTER > 0
> >   if (ifp->if_bpf)
> >   bpf_mtap(ifp->if_bpf, m0, BPF_DIRECTION_OUT);
> > @@ -584,7 +567,6 @@ tun_output(struct ifnet *ifp, struct mbu
> >  #endif
> >  
> >   error = if_enqueue(ifp, m0);
> > - splx(s);
> >  
> >   if (error) {
> >   ifp->if_collisions++;
>
> There is a forgotten splx(s) after pipex_output().  Is it safe to
> call pipex_output() without splnet()?  There is a lot of splnet()
> protection in pipex.

pipex data structures are not touched from an IPL_NET interrupt handler,
so splnet() is superfluous.

tun_output() is ran with the NET_LOCK() so that should be enough.

> > @@ -677,7 +655,6 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
> >   tp->tun_if.if_flags |= *(int *)data & TUN_IFF_FLAGS;
> >   break;
> >   default:
> > - splx(s);
> >   return (EINVAL);
> >   }
> >   break;
>
> Is kernel lock enough to access if_flags?  Should there be netlock, too?

It is enough as long as these flags aren't modified in the input/output
path or in an IPL_NET interrupt handler.  That's the case here.

> > @@ -732,14 +705,11 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
> >   if (!(tp->tun_flags & TUN_LAYER2)) {
> >   int ret;
> >   ret = pipex_ioctl(&tp->pipex_iface, cmd, data);
> > - splx(s);
> >   return (ret);
> >   }
> >  #endif
>
> pipex_ioctl() has a comment /* called from tunioctl() with splnet() */

I can kill the comment :)

Thanks for your review, it was a bad diff.  Updated version below.

Index: net/if_tun.c
===================================================================
RCS file: /cvs/src/sys/net/if_tun.c,v
retrieving revision 1.173
diff -u -p -r1.173 if_tun.c
--- net/if_tun.c 24 Jan 2017 10:08:30 -0000 1.173
+++ net/if_tun.c 26 May 2017 16:04:59 -0000
@@ -192,7 +192,6 @@ tun_create(struct if_clone *ifc, int uni
 {
  struct tun_softc *tp;
  struct ifnet *ifp;
- int s;
 
  tp = malloc(sizeof(*tp), M_DEVBUF, M_NOWAIT|M_ZERO);
  if (tp == NULL)
@@ -226,9 +225,7 @@ tun_create(struct if_clone *ifc, int uni
 #if NBPFILTER > 0
  bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(u_int32_t));
 #endif
- s = splnet();
  LIST_INSERT_HEAD(&tun_softc_list, tp, entry);
- splx(s);
  } else {
  tp->tun_flags |= TUN_LAYER2;
  ether_fakeaddr(ifp);
@@ -239,9 +236,7 @@ tun_create(struct if_clone *ifc, int uni
  if_attach(ifp);
  ether_ifattach(ifp);
 
- s = splnet();
  LIST_INSERT_HEAD(&tap_softc_list, tp, entry);
- splx(s);
  }
 
 #ifdef PIPEX
@@ -269,9 +264,7 @@ tun_clone_destroy(struct ifnet *ifp)
  klist_invalidate(&tp->tun_wsel.si_note);
  splx(s);
 
- s = splnet();
  LIST_REMOVE(tp, entry);
- splx(s);
 
  if (tp->tun_flags & TUN_LAYER2)
  ether_ifdetach(ifp);
@@ -362,7 +355,6 @@ int
 tun_dev_open(struct tun_softc *tp, int flag, int mode, struct proc *p)
 {
  struct ifnet *ifp;
- int s;
 
  if (tp->tun_flags & TUN_OPEN)
  return (EBUSY);
@@ -373,10 +365,8 @@ tun_dev_open(struct tun_softc *tp, int f
  tp->tun_flags |= TUN_NBIO;
 
  /* automatically mark the interface running on open */
- s = splnet();
  ifp->if_flags |= IFF_RUNNING;
  tun_link_state(tp);
- splx(s);
 
  TUNDEBUG(("%s: open\n", ifp->if_xname));
  return (0);
@@ -418,11 +408,9 @@ tun_dev_close(struct tun_softc *tp, int
  /*
  * junk all pending output
  */
- s = splnet();
  ifp->if_flags &= ~IFF_RUNNING;
  tun_link_state(tp);
  IFQ_PURGE(&ifp->if_snd);
- splx(s);
 
  TUNDEBUG(("%s: closed\n", ifp->if_xname));
 
@@ -501,9 +489,7 @@ tun_ioctl(struct ifnet *ifp, u_long cmd,
 {
  struct tun_softc *tp = (struct tun_softc *)(ifp->if_softc);
  struct ifreq *ifr = (struct ifreq *)data;
- int error = 0, s;
-
- s = splnet();
+ int error = 0;
 
  switch (cmd) {
  case SIOCSIFADDR:
@@ -531,7 +517,6 @@ tun_ioctl(struct ifnet *ifp, u_long cmd,
  error = ENOTTY;
  }
 
- splx(s);
  return (error);
 }
 
@@ -543,7 +528,7 @@ tun_output(struct ifnet *ifp, struct mbu
     struct rtentry *rt)
 {
  struct tun_softc *tp = ifp->if_softc;
- int s, error;
+ int error;
  u_int32_t *af;
 
  if ((ifp->if_flags & (IFF_UP|IFF_RUNNING)) != (IFF_UP|IFF_RUNNING)) {
@@ -569,8 +554,6 @@ tun_output(struct ifnet *ifp, struct mbu
  af = mtod(m0, u_int32_t *);
  *af = htonl(dst->sa_family);
 
- s = splnet();
-
 #if NBPFILTER > 0
  if (ifp->if_bpf)
  bpf_mtap(ifp->if_bpf, m0, BPF_DIRECTION_OUT);
@@ -578,13 +561,11 @@ tun_output(struct ifnet *ifp, struct mbu
 #ifdef PIPEX
  if (pipex_enable && (m0 = pipex_output(m0, dst->sa_family,
     sizeof(u_int32_t), &tp->pipex_iface)) == NULL) {
- splx(s);
  return (0);
  }
 #endif
 
  error = if_enqueue(ifp, m0);
- splx(s);
 
  if (error) {
  ifp->if_collisions++;
@@ -635,18 +616,14 @@ int
 tun_dev_ioctl(struct tun_softc *tp, u_long cmd, caddr_t data, int flag,
     struct proc *p)
 {
- int s;
  struct tuninfo *tunp;
  struct mbuf *m;
 
- s = splnet();
  switch (cmd) {
  case TUNSIFINFO:
  tunp = (struct tuninfo *)data;
- if (tunp->mtu < ETHERMIN || tunp->mtu > TUNMRU) {
- splx(s);
+ if (tunp->mtu < ETHERMIN || tunp->mtu > TUNMRU)
  return (EINVAL);
- }
  tp->tun_if.if_mtu = tunp->mtu;
  tp->tun_if.if_type = tunp->type;
  tp->tun_if.if_flags =
@@ -677,7 +654,6 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
  tp->tun_if.if_flags |= *(int *)data & TUN_IFF_FLAGS;
  break;
  default:
- splx(s);
  return (EINVAL);
  }
  break;
@@ -711,19 +687,15 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
  *(int *)data = tp->tun_pgid;
  break;
  case SIOCGIFADDR:
- if (!(tp->tun_flags & TUN_LAYER2)) {
- splx(s);
+ if (!(tp->tun_flags & TUN_LAYER2))
  return (EINVAL);
- }
  bcopy(tp->arpcom.ac_enaddr, data,
     sizeof(tp->arpcom.ac_enaddr));
  break;
 
  case SIOCSIFADDR:
- if (!(tp->tun_flags & TUN_LAYER2)) {
- splx(s);
+ if (!(tp->tun_flags & TUN_LAYER2))
  return (EINVAL);
- }
  bcopy(data, tp->arpcom.ac_enaddr,
     sizeof(tp->arpcom.ac_enaddr));
  break;
@@ -732,14 +704,11 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
  if (!(tp->tun_flags & TUN_LAYER2)) {
  int ret;
  ret = pipex_ioctl(&tp->pipex_iface, cmd, data);
- splx(s);
  return (ret);
  }
 #endif
- splx(s);
  return (ENOTTY);
  }
- splx(s);
  return (0);
 }
 
@@ -773,7 +742,7 @@ tun_dev_read(struct tun_softc *tp, struc
  struct ifnet *ifp = &tp->tun_if;
  struct mbuf *m, *m0;
  unsigned int ifidx;
- int error = 0, s;
+ int error = 0;
  size_t len;
 
  if ((tp->tun_flags & TUN_READY) != TUN_READY)
@@ -782,49 +751,37 @@ tun_dev_read(struct tun_softc *tp, struc
  ifidx = ifp->if_index;
  tp->tun_flags &= ~TUN_RWAIT;
 
- s = splnet();
  do {
  struct ifnet *ifp1;
  int destroyed;
 
  while ((tp->tun_flags & TUN_READY) != TUN_READY) {
  if ((error = tsleep((caddr_t)tp,
-    (PZERO + 1)|PCATCH, "tunread", 0)) != 0) {
- splx(s);
+    (PZERO + 1)|PCATCH, "tunread", 0)) != 0)
  return (error);
- }
  /* Make sure the interface still exists. */
  ifp1 = if_get(ifidx);
  destroyed = (ifp1 == NULL);
  if_put(ifp1);
- if (destroyed) {
- splx(s);
+ if (destroyed)
  return (ENXIO);
- }
  }
  IFQ_DEQUEUE(&ifp->if_snd, m0);
  if (m0 == NULL) {
- if (tp->tun_flags & TUN_NBIO && ioflag & IO_NDELAY) {
- splx(s);
+ if (tp->tun_flags & TUN_NBIO && ioflag & IO_NDELAY)
  return (EWOULDBLOCK);
- }
  tp->tun_flags |= TUN_RWAIT;
  if ((error = tsleep((caddr_t)tp,
-    (PZERO + 1)|PCATCH, "tunread", 0)) != 0) {
- splx(s);
+    (PZERO + 1)|PCATCH, "tunread", 0)) != 0)
  return (error);
- }
  /* Make sure the interface still exists. */
  ifp1 = if_get(ifidx);
  destroyed = (ifp1 == NULL);
  if_put(ifp1);
- if (destroyed) {
- splx(s);
+ if (destroyed)
  return (ENXIO);
- }
  }
  } while (m0 == NULL);
- splx(s);
 
  if (tp->tun_flags & TUN_LAYER2) {
 #if NBPFILTER > 0
@@ -883,9 +840,7 @@ tun_dev_write(struct tun_softc *tp, stru
  struct mbuf *top, **mp, *m;
  int error = 0, tlen;
  size_t mlen;
-#if NBPFILTER > 0
- int s;
-#endif
+
  ifp = &tp->tun_if;
  TUNDEBUG(("%s: tunwrite\n", ifp->if_xname));
 
@@ -961,9 +916,7 @@ tun_dev_write(struct tun_softc *tp, stru
 
 #if NBPFILTER > 0
  if (ifp->if_bpf) {
- s = splnet();
  bpf_mtap(ifp->if_bpf, top, BPF_DIRECTION_IN);
- splx(s);
  }
 #endif
 
@@ -1028,13 +981,12 @@ tappoll(dev_t dev, int events, struct pr
 int
 tun_dev_poll(struct tun_softc *tp, int events, struct proc *p)
 {
- int revents, s;
+ int revents;
  struct ifnet *ifp;
  unsigned int len;
 
  ifp = &tp->tun_if;
  revents = 0;
- s = splnet();
  TUNDEBUG(("%s: tunpoll\n", ifp->if_xname));
 
  if (events & (POLLIN | POLLRDNORM)) {
@@ -1049,7 +1001,6 @@ tun_dev_poll(struct tun_softc *tp, int e
  }
  if (events & (POLLOUT | POLLWRNORM))
  revents |= events & (POLLOUT | POLLWRNORM);
- splx(s);
  return (revents);
 }
 
@@ -1130,7 +1081,6 @@ filt_tunrdetach(struct knote *kn)
 int
 filt_tunread(struct knote *kn, long hint)
 {
- int s;
  struct tun_softc *tp;
  struct ifnet *ifp;
  unsigned int len;
@@ -1143,17 +1093,14 @@ filt_tunread(struct knote *kn, long hint
  tp = (struct tun_softc *)kn->kn_hook;
  ifp = &tp->tun_if;
 
- s = splnet();
  len = IFQ_LEN(&ifp->if_snd);
  if (len > 0) {
- splx(s);
  kn->kn_data = len;
 
  TUNDEBUG(("%s: tunkqread q=%d\n", ifp->if_xname,
     IFQ_LEN(&ifp->if_snd)));
  return (1);
  }
- splx(s);
  TUNDEBUG(("%s: tunkqread waiting\n", ifp->if_xname));
  return (0);
 }

Reply | Threaded
Open this post in threaded view
|

Re: tun/tap vs splnet()

Alexander Bluhm
On Fri, May 26, 2017 at 06:05:30PM +0200, Martin Pieuchot wrote:
> Thanks for your review, it was a bad diff.  Updated version below.

OK bluhm@

>
> Index: net/if_tun.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_tun.c,v
> retrieving revision 1.173
> diff -u -p -r1.173 if_tun.c
> --- net/if_tun.c 24 Jan 2017 10:08:30 -0000 1.173
> +++ net/if_tun.c 26 May 2017 16:04:59 -0000
> @@ -192,7 +192,6 @@ tun_create(struct if_clone *ifc, int uni
>  {
>   struct tun_softc *tp;
>   struct ifnet *ifp;
> - int s;
>  
>   tp = malloc(sizeof(*tp), M_DEVBUF, M_NOWAIT|M_ZERO);
>   if (tp == NULL)
> @@ -226,9 +225,7 @@ tun_create(struct if_clone *ifc, int uni
>  #if NBPFILTER > 0
>   bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(u_int32_t));
>  #endif
> - s = splnet();
>   LIST_INSERT_HEAD(&tun_softc_list, tp, entry);
> - splx(s);
>   } else {
>   tp->tun_flags |= TUN_LAYER2;
>   ether_fakeaddr(ifp);
> @@ -239,9 +236,7 @@ tun_create(struct if_clone *ifc, int uni
>   if_attach(ifp);
>   ether_ifattach(ifp);
>  
> - s = splnet();
>   LIST_INSERT_HEAD(&tap_softc_list, tp, entry);
> - splx(s);
>   }
>  
>  #ifdef PIPEX
> @@ -269,9 +264,7 @@ tun_clone_destroy(struct ifnet *ifp)
>   klist_invalidate(&tp->tun_wsel.si_note);
>   splx(s);
>  
> - s = splnet();
>   LIST_REMOVE(tp, entry);
> - splx(s);
>  
>   if (tp->tun_flags & TUN_LAYER2)
>   ether_ifdetach(ifp);
> @@ -362,7 +355,6 @@ int
>  tun_dev_open(struct tun_softc *tp, int flag, int mode, struct proc *p)
>  {
>   struct ifnet *ifp;
> - int s;
>  
>   if (tp->tun_flags & TUN_OPEN)
>   return (EBUSY);
> @@ -373,10 +365,8 @@ tun_dev_open(struct tun_softc *tp, int f
>   tp->tun_flags |= TUN_NBIO;
>  
>   /* automatically mark the interface running on open */
> - s = splnet();
>   ifp->if_flags |= IFF_RUNNING;
>   tun_link_state(tp);
> - splx(s);
>  
>   TUNDEBUG(("%s: open\n", ifp->if_xname));
>   return (0);
> @@ -418,11 +408,9 @@ tun_dev_close(struct tun_softc *tp, int
>   /*
>   * junk all pending output
>   */
> - s = splnet();
>   ifp->if_flags &= ~IFF_RUNNING;
>   tun_link_state(tp);
>   IFQ_PURGE(&ifp->if_snd);
> - splx(s);
>  
>   TUNDEBUG(("%s: closed\n", ifp->if_xname));
>  
> @@ -501,9 +489,7 @@ tun_ioctl(struct ifnet *ifp, u_long cmd,
>  {
>   struct tun_softc *tp = (struct tun_softc *)(ifp->if_softc);
>   struct ifreq *ifr = (struct ifreq *)data;
> - int error = 0, s;
> -
> - s = splnet();
> + int error = 0;
>  
>   switch (cmd) {
>   case SIOCSIFADDR:
> @@ -531,7 +517,6 @@ tun_ioctl(struct ifnet *ifp, u_long cmd,
>   error = ENOTTY;
>   }
>  
> - splx(s);
>   return (error);
>  }
>  
> @@ -543,7 +528,7 @@ tun_output(struct ifnet *ifp, struct mbu
>      struct rtentry *rt)
>  {
>   struct tun_softc *tp = ifp->if_softc;
> - int s, error;
> + int error;
>   u_int32_t *af;
>  
>   if ((ifp->if_flags & (IFF_UP|IFF_RUNNING)) != (IFF_UP|IFF_RUNNING)) {
> @@ -569,8 +554,6 @@ tun_output(struct ifnet *ifp, struct mbu
>   af = mtod(m0, u_int32_t *);
>   *af = htonl(dst->sa_family);
>  
> - s = splnet();
> -
>  #if NBPFILTER > 0
>   if (ifp->if_bpf)
>   bpf_mtap(ifp->if_bpf, m0, BPF_DIRECTION_OUT);
> @@ -578,13 +561,11 @@ tun_output(struct ifnet *ifp, struct mbu
>  #ifdef PIPEX
>   if (pipex_enable && (m0 = pipex_output(m0, dst->sa_family,
>      sizeof(u_int32_t), &tp->pipex_iface)) == NULL) {
> - splx(s);
>   return (0);
>   }
>  #endif
>  
>   error = if_enqueue(ifp, m0);
> - splx(s);
>  
>   if (error) {
>   ifp->if_collisions++;
> @@ -635,18 +616,14 @@ int
>  tun_dev_ioctl(struct tun_softc *tp, u_long cmd, caddr_t data, int flag,
>      struct proc *p)
>  {
> - int s;
>   struct tuninfo *tunp;
>   struct mbuf *m;
>  
> - s = splnet();
>   switch (cmd) {
>   case TUNSIFINFO:
>   tunp = (struct tuninfo *)data;
> - if (tunp->mtu < ETHERMIN || tunp->mtu > TUNMRU) {
> - splx(s);
> + if (tunp->mtu < ETHERMIN || tunp->mtu > TUNMRU)
>   return (EINVAL);
> - }
>   tp->tun_if.if_mtu = tunp->mtu;
>   tp->tun_if.if_type = tunp->type;
>   tp->tun_if.if_flags =
> @@ -677,7 +654,6 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
>   tp->tun_if.if_flags |= *(int *)data & TUN_IFF_FLAGS;
>   break;
>   default:
> - splx(s);
>   return (EINVAL);
>   }
>   break;
> @@ -711,19 +687,15 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
>   *(int *)data = tp->tun_pgid;
>   break;
>   case SIOCGIFADDR:
> - if (!(tp->tun_flags & TUN_LAYER2)) {
> - splx(s);
> + if (!(tp->tun_flags & TUN_LAYER2))
>   return (EINVAL);
> - }
>   bcopy(tp->arpcom.ac_enaddr, data,
>      sizeof(tp->arpcom.ac_enaddr));
>   break;
>  
>   case SIOCSIFADDR:
> - if (!(tp->tun_flags & TUN_LAYER2)) {
> - splx(s);
> + if (!(tp->tun_flags & TUN_LAYER2))
>   return (EINVAL);
> - }
>   bcopy(data, tp->arpcom.ac_enaddr,
>      sizeof(tp->arpcom.ac_enaddr));
>   break;
> @@ -732,14 +704,11 @@ tun_dev_ioctl(struct tun_softc *tp, u_lo
>   if (!(tp->tun_flags & TUN_LAYER2)) {
>   int ret;
>   ret = pipex_ioctl(&tp->pipex_iface, cmd, data);
> - splx(s);
>   return (ret);
>   }
>  #endif
> - splx(s);
>   return (ENOTTY);
>   }
> - splx(s);
>   return (0);
>  }
>  
> @@ -773,7 +742,7 @@ tun_dev_read(struct tun_softc *tp, struc
>   struct ifnet *ifp = &tp->tun_if;
>   struct mbuf *m, *m0;
>   unsigned int ifidx;
> - int error = 0, s;
> + int error = 0;
>   size_t len;
>  
>   if ((tp->tun_flags & TUN_READY) != TUN_READY)
> @@ -782,49 +751,37 @@ tun_dev_read(struct tun_softc *tp, struc
>   ifidx = ifp->if_index;
>   tp->tun_flags &= ~TUN_RWAIT;
>  
> - s = splnet();
>   do {
>   struct ifnet *ifp1;
>   int destroyed;
>  
>   while ((tp->tun_flags & TUN_READY) != TUN_READY) {
>   if ((error = tsleep((caddr_t)tp,
> -    (PZERO + 1)|PCATCH, "tunread", 0)) != 0) {
> - splx(s);
> +    (PZERO + 1)|PCATCH, "tunread", 0)) != 0)
>   return (error);
> - }
>   /* Make sure the interface still exists. */
>   ifp1 = if_get(ifidx);
>   destroyed = (ifp1 == NULL);
>   if_put(ifp1);
> - if (destroyed) {
> - splx(s);
> + if (destroyed)
>   return (ENXIO);
> - }
>   }
>   IFQ_DEQUEUE(&ifp->if_snd, m0);
>   if (m0 == NULL) {
> - if (tp->tun_flags & TUN_NBIO && ioflag & IO_NDELAY) {
> - splx(s);
> + if (tp->tun_flags & TUN_NBIO && ioflag & IO_NDELAY)
>   return (EWOULDBLOCK);
> - }
>   tp->tun_flags |= TUN_RWAIT;
>   if ((error = tsleep((caddr_t)tp,
> -    (PZERO + 1)|PCATCH, "tunread", 0)) != 0) {
> - splx(s);
> +    (PZERO + 1)|PCATCH, "tunread", 0)) != 0)
>   return (error);
> - }
>   /* Make sure the interface still exists. */
>   ifp1 = if_get(ifidx);
>   destroyed = (ifp1 == NULL);
>   if_put(ifp1);
> - if (destroyed) {
> - splx(s);
> + if (destroyed)
>   return (ENXIO);
> - }
>   }
>   } while (m0 == NULL);
> - splx(s);
>  
>   if (tp->tun_flags & TUN_LAYER2) {
>  #if NBPFILTER > 0
> @@ -883,9 +840,7 @@ tun_dev_write(struct tun_softc *tp, stru
>   struct mbuf *top, **mp, *m;
>   int error = 0, tlen;
>   size_t mlen;
> -#if NBPFILTER > 0
> - int s;
> -#endif
> +
>   ifp = &tp->tun_if;
>   TUNDEBUG(("%s: tunwrite\n", ifp->if_xname));
>  
> @@ -961,9 +916,7 @@ tun_dev_write(struct tun_softc *tp, stru
>  
>  #if NBPFILTER > 0
>   if (ifp->if_bpf) {
> - s = splnet();
>   bpf_mtap(ifp->if_bpf, top, BPF_DIRECTION_IN);
> - splx(s);
>   }
>  #endif
>  
> @@ -1028,13 +981,12 @@ tappoll(dev_t dev, int events, struct pr
>  int
>  tun_dev_poll(struct tun_softc *tp, int events, struct proc *p)
>  {
> - int revents, s;
> + int revents;
>   struct ifnet *ifp;
>   unsigned int len;
>  
>   ifp = &tp->tun_if;
>   revents = 0;
> - s = splnet();
>   TUNDEBUG(("%s: tunpoll\n", ifp->if_xname));
>  
>   if (events & (POLLIN | POLLRDNORM)) {
> @@ -1049,7 +1001,6 @@ tun_dev_poll(struct tun_softc *tp, int e
>   }
>   if (events & (POLLOUT | POLLWRNORM))
>   revents |= events & (POLLOUT | POLLWRNORM);
> - splx(s);
>   return (revents);
>  }
>  
> @@ -1130,7 +1081,6 @@ filt_tunrdetach(struct knote *kn)
>  int
>  filt_tunread(struct knote *kn, long hint)
>  {
> - int s;
>   struct tun_softc *tp;
>   struct ifnet *ifp;
>   unsigned int len;
> @@ -1143,17 +1093,14 @@ filt_tunread(struct knote *kn, long hint
>   tp = (struct tun_softc *)kn->kn_hook;
>   ifp = &tp->tun_if;
>  
> - s = splnet();
>   len = IFQ_LEN(&ifp->if_snd);
>   if (len > 0) {
> - splx(s);
>   kn->kn_data = len;
>  
>   TUNDEBUG(("%s: tunkqread q=%d\n", ifp->if_xname,
>      IFQ_LEN(&ifp->if_snd)));
>   return (1);
>   }
> - splx(s);
>   TUNDEBUG(("%s: tunkqread waiting\n", ifp->if_xname));
>   return (0);
>  }

Reply | Threaded
Open this post in threaded view
|

Re: tun/tap vs splnet()

YASUOKA Masahiko-4
In reply to this post by Martin Pieuchot
On Fri, 26 May 2017 18:05:30 +0200
Martin Pieuchot <[hidden email]> wrote:

> On 26/05/17(Fri) 17:43, Alexander Bluhm wrote:
>> > @@ -569,8 +554,6 @@ tun_output(struct ifnet *ifp, struct mbu
>> >   af = mtod(m0, u_int32_t *);
>> >   *af = htonl(dst->sa_family);
>> >  
>> > - s = splnet();
>> > -
>> >  #if NBPFILTER > 0
>> >   if (ifp->if_bpf)
>> >   bpf_mtap(ifp->if_bpf, m0, BPF_DIRECTION_OUT);
>> > @@ -584,7 +567,6 @@ tun_output(struct ifnet *ifp, struct mbu
>> >  #endif
>> >  
>> >   error = if_enqueue(ifp, m0);
>> > - splx(s);
>> >  
>> >   if (error) {
>> >   ifp->if_collisions++;
>>
>> There is a forgotten splx(s) after pipex_output().  Is it safe to
>> call pipex_output() without splnet()?  There is a lot of splnet()
>> protection in pipex.
>
> pipex data structures are not touched from an IPL_NET interrupt handler,
> so splnet() is superfluous.

Yes.  pipex is called from ether_input() which had been executed in
ethernet drivers' interrupt.  But now it is executed as a task, so
splnet is not neccesary for pipex.

--yasuoka