On 10/07/20(Fri) 14:07, Vitaliy Makkoveev wrote:
> We have some races in pppac(4)
> 1. malloc(9) can sleep so we must check `sc' presence after malloc(9)
Makes sense.
> 2. we can sleep between `sc' insertion to `sc_entry' list and
> `sc_pipex_iface' initialization. Concurrent pppacioctl() can touch
> this incomplete `sc'.
Why not insert the descriptor at the end? Shouldn't the order of
operations be:
pipex_iface_init();
if_attach();
LIST_INSERT_HEAD()
This way there's no need for a `ready' flag since the descriptor is only
added to global data structures once it is completely initialized.
Using a `sc_ready' or `sc_dead' approach is something that require
touching all drivers whereas serializing insertions to global data
structures can be done at once for all the kernel.
> Index: sys/net/if_pppx.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.91
> diff -u -p -r1.91 if_pppx.c
> --- sys/net/if_pppx.c 6 Jul 2020 20:37:51 -0000 1.91
> +++ sys/net/if_pppx.c 10 Jul 2020 11:04:53 -0000
> @@ -1019,7 +1019,7 @@ RBT_GENERATE(pppx_ifs, pppx_if, pxi_entr
>
> struct pppac_softc {
> struct ifnet sc_if;
> - unsigned int sc_dead;
> + unsigned int sc_ready;
> dev_t sc_dev;
> LIST_ENTRY(pppac_softc)
> sc_entry;
> @@ -1072,8 +1072,12 @@ pppac_lookup(dev_t dev)
> struct pppac_softc *sc;
>
> LIST_FOREACH(sc, &pppac_devs, sc_entry) {
> - if (sc->sc_dev == dev)
> - return (sc);
> + if (sc->sc_dev == dev) {
> + if (sc->sc_ready)
> + return (sc);
> + else
> + break;
> + }
> }
>
> return (NULL);
> @@ -1088,22 +1092,25 @@ pppacattach(int n)
> int
> pppacopen(dev_t dev, int flags, int mode, struct proc *p)
> {
> - struct pppac_softc *sc;
> + struct pppac_softc *sc, *sc_tmp;
> struct ifnet *ifp;
>
> - sc = pppac_lookup(dev);
> - if (sc != NULL)
> - return (EBUSY);
> -
> sc = malloc(sizeof(*sc), M_DEVBUF, M_WAITOK|M_ZERO);
> +
> + LIST_FOREACH(sc_tmp, &pppac_devs, sc_entry) {
> + if (sc_tmp->sc_dev == dev) {
> + free(sc, M_DEVBUF, sizeof(*sc));
> + return (EBUSY);
> + }
> + }
> +
> sc->sc_dev = dev;
> + LIST_INSERT_HEAD(&pppac_devs, sc, sc_entry);
>
> mtx_init(&sc->sc_rsel_mtx, IPL_SOFTNET);
> mtx_init(&sc->sc_wsel_mtx, IPL_SOFTNET);
> mq_init(&sc->sc_mq, IFQ_MAXLEN, IPL_SOFTNET);
>
> - LIST_INSERT_HEAD(&pppac_devs, sc, sc_entry);
> -
> ifp = &sc->sc_if;
> snprintf(ifp->if_xname, sizeof(ifp->if_xname), "pppac%u", minor(dev));
>
> @@ -1129,6 +1136,7 @@ pppacopen(dev_t dev, int flags, int mode
> #endif
>
> pipex_iface_init(&sc->sc_pipex_iface, ifp);
> + sc->sc_ready = 1;
>
> return (0);
> }
> @@ -1136,12 +1144,14 @@ pppacopen(dev_t dev, int flags, int mode
> int
> pppacread(dev_t dev, struct uio *uio, int ioflag)
> {
> - struct pppac_softc *sc = pppac_lookup(dev);
> + struct pppac_softc *sc;
> struct ifnet *ifp = &sc->sc_if;
> struct mbuf *m0, *m;
> int error = 0;
> size_t len;
>
> + if ((sc = pppac_lookup(dev)) == NULL)
> + return (EBADF);
> if (!ISSET(ifp->if_flags, IFF_RUNNING))
> return (EHOSTDOWN);
>
> @@ -1181,12 +1191,14 @@ pppacread(dev_t dev, struct uio *uio, in
> int
> pppacwrite(dev_t dev, struct uio *uio, int ioflag)
> {
> - struct pppac_softc *sc = pppac_lookup(dev);
> + struct pppac_softc *sc;
> struct ifnet *ifp = &sc->sc_if;
> uint32_t proto;
> int error;
> struct mbuf *m;
>
> + if ((sc = pppac_lookup(dev)) == NULL)
> + return (EBADF);
> if (!ISSET(ifp->if_flags, IFF_RUNNING))
> return (EHOSTDOWN);
>
> @@ -1258,9 +1270,12 @@ pppacwrite(dev_t dev, struct uio *uio, i
> int
> pppacioctl(dev_t dev, u_long cmd, caddr_t data, int flags, struct proc *p)
> {
> - struct pppac_softc *sc = pppac_lookup(dev);
> + struct pppac_softc *sc;
> int error = 0;
>
> + if ((sc = pppac_lookup(dev)) == NULL)
> + return (EBADF);
> +
> switch (cmd) {
> case TUNSIFMODE: /* make npppd happy */
> break;
> @@ -1282,9 +1297,12 @@ pppacioctl(dev_t dev, u_long cmd, caddr_
> int
> pppacpoll(dev_t dev, int events, struct proc *p)
> {
> - struct pppac_softc *sc = pppac_lookup(dev);
> + struct pppac_softc *sc;
> int revents = 0;
>
> + if ((sc = pppac_lookup(dev)) == NULL)
> + goto out;
> +
> if (events & (POLLIN | POLLRDNORM)) {
> if (!mq_empty(&sc->sc_mq))
> revents |= events & (POLLIN | POLLRDNORM);
> @@ -1296,17 +1314,20 @@ pppacpoll(dev_t dev, int events, struct
> if (events & (POLLIN | POLLRDNORM))
> selrecord(p, &sc->sc_rsel);
> }
> -
> +out:
> return (revents);
> }
>
> int
> pppackqfilter(dev_t dev, struct knote *kn)
> {
> - struct pppac_softc *sc = pppac_lookup(dev);
> + struct pppac_softc *sc;
> struct mutex *mtx;
> struct klist *klist;
>
> + if ((sc = pppac_lookup(dev)) == NULL)
> + return (EBADF);
> +
> switch (kn->kn_filter) {
> case EVFILT_READ:
> mtx = &sc->sc_rsel_mtx;
> @@ -1373,12 +1394,15 @@ filt_pppac_write(struct knote *kn, long
> int
> pppacclose(dev_t dev, int flags, int mode, struct proc *p)
> {
> - struct pppac_softc *sc = pppac_lookup(dev);
> + struct pppac_softc *sc;
> struct ifnet *ifp = &sc->sc_if;
> int s;
>
> + if ((sc = pppac_lookup(dev)) == NULL)
> + return (EBADF);
> +
> + sc->sc_ready = 0;
> NET_LOCK();
> - sc->sc_dead = 1;
> CLR(ifp->if_flags, IFF_RUNNING);
> NET_UNLOCK();
>
> @@ -1404,7 +1428,7 @@ pppac_ioctl(struct ifnet *ifp, u_long cm
> /* struct ifreq *ifr = (struct ifreq *)data; */
> int error = 0;
>
> - if (sc->sc_dead)
> + if (sc->sc_ready == 0)
> return (ENXIO);
>
> switch (cmd) {
>
