On Wed, Apr 06, 2016 at 12:15 +0200, Mike Belopuhov wrote:
> Hi,
>
> The EBUSY hack that I'm removing imposes an order on the ifconfig
> commands issued against the pppoe interface used to configure the
> sppp layer below. Right now ifconfig will fail with "device busy"
> after the authentication parameters have been set up because it
> kickstarts the LCP logic down in the sppp. To workaround that we
> have to bring the interface down first and then submit commands in
> a correct order or do the reconfiguration if that's the goal.
>
> As far as I know, this doesn't follow the behavior of other pseudo
> interfaces and forces us to implement kludges and workarounds for
> these arguably broken semantics. We'd rather avoid that.
>
> To counter this we use the ENETRESET trick that other drivers use
> to tell the pppoe layer that sppp has requested a stop/init reset
> sequence to proceed which we oblige with in case pppoe is UP and
> RUNNING.
>
> Now I must admit the code under "if (error == ENETRESET)" is a
> bit scary, but apart from calling if_down, if_up and SIOCSIFFLAGS
> directly it's copied from the pppoe/SIOCSIFFLAGS handling.
> sppp/SIOCSIFFLAGS handler does not use the ifreq argument so it's
> passed down as a NULL. Theoretically I can factor out code doing
> SIOCSIFFLAG in the sppp_ioctl to two exported functions sppp_up
> and sppp_down and call them from the pppoe_ioctl, but I don't
> think this will be a huge improvement in readability or otherwise.
>
> This has been already tested by a few, so I'm fairly confident
> that there are no operational issues with this diff, but I'm not
> using this myself (-;
>
> Opinions? OK?
>
> (I've stashed a small cleanup diff for the sppp_set_params that
> I'll be posting shortly in case you notice that it can lose some
> weight.)
>
I take it nobody's got a better idea. Any objections for this to
go in?
>
> diff --git sys/net/if_pppoe.c sys/net/if_pppoe.c
> index 15b2b8a..8e5f713 100644
> --- sys/net/if_pppoe.c
> +++ sys/net/if_pppoe.c
> @@ -896,11 +896,11 @@ static int
> pppoe_ioctl(struct ifnet *ifp, unsigned long cmd, caddr_t data)
> {
> struct proc *p = curproc; /* XXX */
> struct pppoe_softc *sc = (struct pppoe_softc *)ifp;
> struct ifnet *eth_if;
> - int error = 0;
> + int s, error = 0;
>
> switch (cmd) {
> case PPPOESETPARMS:
> {
> struct pppoediscparms *parms = (struct pppoediscparms *)data;
> @@ -1027,11 +1027,38 @@ pppoe_ioctl(struct ifnet *ifp, unsigned long cmd,
> caddr_t data)
> if_put(eth_if);
>
> return (sppp_ioctl(ifp, cmd, data));
> }
> default:
> - return (sppp_ioctl(ifp, cmd, data));
> + error = sppp_ioctl(ifp, cmd, data);
> + if (error == ENETRESET) {
> + error = 0;
> + if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) ==
> + (IFF_UP | IFF_RUNNING)) {
> + s = splnet();
> + if_down(ifp);
> + splx(s);
> + if (sc->sc_state >= PPPOE_STATE_PADI_SENT &&
> + sc->sc_state < PPPOE_STATE_SESSION) {
> + timeout_del(&sc->sc_timeout);
> + sc->sc_state = PPPOE_STATE_INITIAL;
> + sc->sc_padi_retried = 0;
> + sc->sc_padr_retried = 0;
> + memcpy(&sc->sc_dest,
> + etherbroadcastaddr,
> + sizeof(sc->sc_dest));
> + }
> + error = sppp_ioctl(ifp, SIOCSIFFLAGS, NULL);
> + if (error)
> + return (error);
> + s = splnet();
> + if_up(ifp);
> + splx(s);
> + return (sppp_ioctl(ifp, SIOCSIFFLAGS, NULL));
> + }
> + }
> + return (error);
> }
> return (0);
> }
>
> /*
> diff --git sys/net/if_spppsubr.c sys/net/if_spppsubr.c
> index ae8b589..7ff1156 100644
> --- sys/net/if_spppsubr.c
> +++ sys/net/if_spppsubr.c
> @@ -4527,23 +4527,10 @@ sppp_set_params(struct sppp *sp, struct ifreq *ifr)
> int cmd;
>
> if (copyin((caddr_t)ifr->ifr_data, &cmd, sizeof cmd) != 0)
> return EFAULT;
>
> - /*
> - * We have a very specific idea of which fields we allow
> - * being passed back from userland, so to not clobber our
> - * current state. For one, we only allow setting
> - * anything if LCP is in dead phase. Once the LCP
> - * negotiations started, the authentication settings must
> - * not be changed again. (The administrator can force an
> - * ifconfig down in order to get LCP back into dead
> - * phase.)
> - */
> - if (sp->pp_phase != PHASE_DEAD)
> - return EBUSY;
> -
> switch (cmd) {
> case SPPPIOSDEFS:
> {
> struct spppreq *spr;
>
> @@ -4636,11 +4623,11 @@ sppp_set_params(struct sppp *sp, struct ifreq *ifr)
> }
> default:
> return EINVAL;
> }
>
> - return 0;
> + return (ENETRESET);
> }
>
> void
> sppp_phase_network(struct sppp *sp)
> {
