On Tue, Oct 28, 2025 at 08:56:00AM +0300, Vitaliy Makkoveev wrote:
> On Tue, Oct 28, 2025 at 02:26:57PM +1000, David Gwynne wrote:
> > On Mon, Oct 27, 2025 at 09:43:04PM +0100, Alexander Bluhm wrote:
> > > On Mon, Oct 27, 2025 at 08:54:02PM +0300, Vitaliy Makkoveev wrote:
> > > > Seems the whole sppp_output() should take kernel lock. I have no ability
> > > > to test this diff.
> > >
> > > I also came to the conclusion that some big kernel lock is missing.
> > > Just wondering why this bug appears now. We have removed kernel
> > > lock from network stack a long time ago.
> >
> > this is because of src/sys/net/if_pppoe.c r1.87, which moved pppoe away
> > from queueing outgoing packets on the ifq to direct dispatch. because
> > pppoe wasn't marked mpsafe, sppp_output was called by the ifq machinery
> > with the kernel lock held.
> >
> > the testing i (and semarie) did was without the link1 flag, but that's
> > being used here. IFF_LINK1 is aliased to IFF_AUTO flag in the code and
> > is used to implement autodial functionality in the sppp code. the
> > kernel lock is only really needed to start moving the interface
> > through the connection state machine, which can be done in parallel
> > to the data path.
> >
> > also, fiddling with ifp->if_flags like this should be done while
> > holding NET_LOCK, which isn't guaranteed to be held here. the diff
> > below moves the autodial stuff into a task run from systq, which
> > holds the kernel lock like the ppp state machine requires.
> >
> > this diff compiles. i have no idea if it works.
> >
>
> According the trace, sppp_output() running with shared netlock. Is it
> still enough? I especially concern about if_enqueue() and the following
> bridge(4) codepaths.
hrm. i was confusing pppoe output and enqueue. if_pppoe.c r1.87 only
changed when enqueue is called, the pppoe output calls (which is
sppp_output) is called the same way now as it has been for a while. it
must have been a change to how the stack is called that caused this?
anyway, to answer your questions (i think): interface output and
enqueue should not need the caller to be holding any locks for them.
the bridge stuff is pretty gross, but the worst i can see is that it
can lose counter updates, otherwise it doesnt appear to rely on the
kernel or net locks.
>
> > Index: if_sppp.h
> > ===================================================================
> > RCS file: /cvs/src/sys/net/if_sppp.h,v
> > diff -u -p -r1.31 if_sppp.h
> > --- if_sppp.h 15 Jan 2025 06:15:44 -0000 1.31
> > +++ if_sppp.h 28 Oct 2025 04:25:22 -0000
> > @@ -174,6 +174,7 @@ struct sppp {
> > time_t pp_last_receive; /* peer's last "sign of life" */
> > time_t pp_last_activity; /* second of last payload data s/r */
> > enum ppp_phase pp_phase; /* phase we're currently in */
> > + struct task pp_autodial;
> > int state[IDX_COUNT]; /* state machine */
> > u_char confid[IDX_COUNT]; /* id of last configuration request */
> > int rst_counter[IDX_COUNT]; /* restart counter */
> > Index: if_spppsubr.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/if_spppsubr.c,v
> > diff -u -p -r1.198 if_spppsubr.c
> > --- if_spppsubr.c 19 Aug 2025 12:34:15 -0000 1.198
> > +++ if_spppsubr.c 28 Oct 2025 04:25:22 -0000
> > @@ -227,6 +227,7 @@ static struct timeout keepalive_ch;
> > struct ifnet *ifp = &sp->pp_if; \
> > int debug = ifp->if_flags & IFF_DEBUG
> >
> > +void sppp_autodial(void *);
> > int sppp_output(struct ifnet *ifp, struct mbuf *m,
> > struct sockaddr *dst, struct rtentry *rt);
> >
> > @@ -570,6 +571,20 @@ sppp_input(struct ifnet *ifp, struct mbu
> > goto drop;
> > }
> >
> > +void
> > +sppp_autodial(void *arg)
> > +{
> > + struct sppp *sp = arg;
> > + struct ifnet *ifp = &sp->pp_if;
> > +
> > + NET_LOCK();
> > + if ((ifp->if_flags & (IFF_RUNNING | IFF_AUTO)) == IFF_AUTO) {
> > + ifp->if_flags |= IFF_RUNNING;
> > + lcp.Open(sp);
> > + }
> > + NET_UNLOCK();
> > +}
> > +
> > /*
> > * Enqueue transmit packet.
> > */
> > @@ -581,6 +596,7 @@ sppp_output(struct ifnet *ifp, struct mb
> > struct timeval tv;
> > int s, rv = 0;
> > u_int16_t protocol;
> > + int if_flags;
> >
> > #ifdef DIAGNOSTIC
> > if (ifp->if_rdomain != rtable_l2(m->m_pkthdr.ph_rtableid)) {
> > @@ -596,22 +612,20 @@ sppp_output(struct ifnet *ifp, struct mb
> > getmicrouptime(&tv);
> > sp->pp_last_activity = tv.tv_sec;
> >
> > - if ((ifp->if_flags & IFF_UP) == 0 ||
> > - (ifp->if_flags & (IFF_RUNNING | IFF_AUTO)) == 0) {
> > + if_flags = ifp->if_flags;
> > + if ((if_flags & IFF_UP) == 0 ||
> > + (if_flags & (IFF_RUNNING | IFF_AUTO)) == 0) {
> > m_freem (m);
> > splx (s);
> > return (ENETDOWN);
> > }
> >
> > - if ((ifp->if_flags & (IFF_RUNNING | IFF_AUTO)) == IFF_AUTO) {
> > + if ((if_flags & (IFF_RUNNING | IFF_AUTO)) == IFF_AUTO) {
> > /*
> > * Interface is not yet running, but auto-dial. Need
> > * to start LCP for it.
> > */
> > - ifp->if_flags |= IFF_RUNNING;
> > - splx(s);
> > - lcp.Open(sp);
> > - s = splnet();
> > + task_add(systq, &sp->pp_autodial);
> > }
> >
> > if (dst->sa_family == AF_INET) {
> > @@ -746,6 +760,8 @@ sppp_attach(struct ifnet *ifp)
> > sp->pp_up = lcp.Up;
> > sp->pp_down = lcp.Down;
> >
> > + task_set(&sp->pp_autodial, sppp_autodial, sp);
> > +
> > for (i = 0; i < IDX_COUNT; i++)
> > timeout_set(&sp->ch[i], (cps[i])->TO, (void *)sp);
> > timeout_set(&sp->pap_my_to_ch, sppp_pap_my_TO, (void *)sp);
> > @@ -762,6 +778,8 @@ sppp_detach(struct ifnet *ifp)
> > {
> > struct sppp **q, *p, *sp = (struct sppp*) ifp;
> > int i;
> > +
> > + taskq_del_barrier(systq, &sp->pp_autodial);
> >
> > sppp_ipcp_destroy(sp);
> > sppp_ipv6cp_destroy(sp);
