Hello Klemens.
pppoe(4) input path and pppoe(4) config path (I mean clone/destroy)
is always different context. Your diff introduces the new lock which
protects `pppoe_softc_list’ list but what should protect `sc’ you got
from this list after you released `pppoe_lock’?
I mean this dereference is not safe because concurrent thread can
destroy this `sc’ (at least in theory).
> @@ -460,8 +463,10 @@ static void pppoe_dispatch_disc_pkt(stru
> err_msg = "TAG HUNIQUE ERROR";
> break;
> }
> + rw_enter_read(&pppoe_lock);
> sc = pppoe_find_softc_by_hunique(mtod(n, caddr_t) +
> noff,
> len, m->m_pkthdr.ph_ifidx);
> + rw_exit_read(&pppoe_lock);
> if (sc != NULL)
> devname = sc->sc_sppp.pp_if.if_xname;
This time we have multiple locks around input and config paths so I
guess this is the real reason you didn’t caught use after free issue.
Also witness allow us to debug lock order, but not concurrent access.
> On 13 Sep 2020, at 16:12, Klemens Nanni <k...@openbsd.org> wrote:
>
> This is my first try trading global locks for interface specific ones.
>
> pppoe(4) keeps a list of all its interfaces which is then obviously
> traversed during create and destroy.
>
> Currently, the net lock is grabbed for this, but there seems to be no
> justification other than reusing^Wabusing an existing lock.
>
> I run this diff with WITNESS and kern.witness=2 on my edgerouter 4
> providing my home uplink via pppoe0: the kernel runs stable, there's
> not witness log showing up and creating and destroying hundreds of
> additional pppoe(4) devices works without disruption.
>
> Is this the right direction?
>
> Index: if_pppoe.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppoe.c,v
> retrieving revision 1.73
> diff -u -p -r1.73 if_pppoe.c
> --- if_pppoe.c 13 Sep 2020 11:00:40 -0000 1.73
> +++ if_pppoe.c 13 Sep 2020 11:31:12 -0000
> @@ -114,15 +114,18 @@ struct pppoetag {
> #define PPPOE_DISC_MAXPADI 4 /* retry PADI four times
> (quickly) */
> #define PPPOE_DISC_MAXPADR 2 /* retry PADR twice */
>
> +struct rwlock pppoe_lock = RWLOCK_INITIALIZER("pppoe");
> +
> /*
> * Locks used to protect struct members and global data
> * I immutable after creation
> * N net lock
> + * p pppoe lock
> */
>
> struct pppoe_softc {
> struct sppp sc_sppp; /* contains a struct ifnet as first
> element */
> - LIST_ENTRY(pppoe_softc) sc_list;/* [N] */
> + LIST_ENTRY(pppoe_softc) sc_list;/* [p] */
> unsigned int sc_eth_ifidx; /* [N] */
>
> int sc_state; /* [N] discovery phase or session
> connected */
> @@ -233,7 +236,7 @@ pppoe_clone_create(struct if_clone *ifc,
> bpfattach(&sc->sc_sppp.pp_if.if_bpf, &sc->sc_sppp.pp_if, DLT_PPP_ETHER,
> 0);
> #endif
>
> - NET_LOCK();
> + rw_enter_write(&pppoe_lock);
> retry:
> unique = arc4random();
> LIST_FOREACH(tmpsc, &pppoe_softc_list, sc_list)
> @@ -241,7 +244,7 @@ retry:
> goto retry;
> sc->sc_unique = unique;
> LIST_INSERT_HEAD(&pppoe_softc_list, sc, sc_list);
> - NET_UNLOCK();
> + rw_exit_write(&pppoe_lock);
>
> return (0);
> }
> @@ -252,9 +255,9 @@ pppoe_clone_destroy(struct ifnet *ifp)
> {
> struct pppoe_softc *sc = ifp->if_softc;
>
> - NET_LOCK();
> + rw_enter_write(&pppoe_lock);
> LIST_REMOVE(sc, sc_list);
> - NET_UNLOCK();
> + rw_exit_write(&pppoe_lock);
>
> timeout_del(&sc->sc_timeout);
>
> @@ -460,8 +463,10 @@ static void pppoe_dispatch_disc_pkt(stru
> err_msg = "TAG HUNIQUE ERROR";
> break;
> }
> + rw_enter_read(&pppoe_lock);
> sc = pppoe_find_softc_by_hunique(mtod(n, caddr_t) +
> noff,
> len, m->m_pkthdr.ph_ifidx);
> + rw_exit_read(&pppoe_lock);
> if (sc != NULL)
> devname = sc->sc_sppp.pp_if.if_xname;
> break;
> @@ -668,8 +673,12 @@ pppoe_data_input(struct mbuf *m)
> #ifdef PPPOE_TERM_UNKNOWN_SESSIONS
> u_int8_t shost[ETHER_ADDR_LEN];
> #endif
> - if (LIST_EMPTY(&pppoe_softc_list))
> + rw_enter_read(&pppoe_lock);
> + if (LIST_EMPTY(&pppoe_softc_list)) {
> + rw_exit_read(&pppoe_lock);
> goto drop;
> + }
> + rw_exit_read(&pppoe_lock);
>
> KASSERT(m->m_flags & M_PKTHDR);
>
> @@ -699,7 +708,9 @@ pppoe_data_input(struct mbuf *m)
> goto drop;
>
> session = ntohs(ph->session);
> + rw_enter_read(&pppoe_lock);
> sc = pppoe_find_softc_by_session(session, m->m_pkthdr.ph_ifidx);
> + rw_exit_read(&pppoe_lock);
> if (sc == NULL) {
> #ifdef PPPOE_TERM_UNKNOWN_SESSIONS
> printf("pppoe (data): input for unknown session 0x%x, sending
> PADT\n",
>
