Re: RFC 4638

[Matthias Scheler <tron%zhadum.org.uk@localhost>]

On Thu, Sep 01, 2011 at 10:44:37PM +0100, Robert Swindells wrote:
> Could somebody review the following patches to add support for RFC 4638.
> 
> The change to if_spppsubr.c just moves the code to work out whether
> lcp should request an mru change until after the pppoe device has
> picked up the mtu of the underlying ethernet device.
> 
> To use this change:
> 
>  pick an ethernet card that supports jumbo frames.
>  set the mtu on the ethernet card to 1508.
>  start the pppoe device as normal. 
>
> This is working for me connected to a BT VDSL modem.

Cool. Thanks a lot for coming up with this patch.

> There probably needs to be an extra test for the server refusing the
> max payload request.

In which case it should revert to the previously negoiated MTU?

> Index: if_spppsubr.c
> ===================================================================
> RCS file: /cvsroot/src/sys/net/if_spppsubr.c,v
> retrieving revision 1.121
> diff -u -r1.121 if_spppsubr.c
> --- if_spppsubr.c     17 Jul 2011 20:54:52 -0000      1.121
> +++ if_spppsubr.c     1 Sep 2011 21:28:39 -0000
> @@ -2000,12 +2000,6 @@
>       sp->pp_seq[IDX_LCP] = 0;
>       sp->pp_rseq[IDX_LCP] = 0;
>       sp->lcp.protos = 0;
> -     if (sp->pp_if.if_mtu < PP_MTU) {
> -             sp->lcp.mru = sp->pp_if.if_mtu;
> -             sp->lcp.opts |= (1 << LCP_OPT_MRU);
> -     } else
> -             sp->lcp.mru = PP_MTU;
> -     sp->lcp.their_mru = PP_MTU;

I think it would be better to keep the initialise the fields to save
values here.

>       /*
>        * Initialize counters and timeout values.  Note that we don't
> @@ -2090,6 +2084,13 @@
>  static void
>  sppp_lcp_open(struct sppp *sp)
>  {
> +     if (sp->pp_if.if_mtu < PP_MTU) {
> +             sp->lcp.mru = sp->pp_if.if_mtu;
> +             sp->lcp.opts |= (1 << LCP_OPT_MRU);
> +     } else
> +             sp->lcp.mru = PP_MTU;
> +     sp->lcp.their_mru = PP_MTU;
> +
>       /*
>        * If we are authenticator, negotiate LCP_AUTH
>        */
> Index: if_pppoe.c
> ===================================================================
> RCS file: /cvsroot/src/sys/net/if_pppoe.c,v
> retrieving revision 1.97
> diff -u -r1.97 if_pppoe.c
> --- if_pppoe.c        30 Aug 2011 22:23:06 -0000      1.97
> +++ if_pppoe.c        1 Sep 2011 21:28:39 -0000
> @@ -85,6 +85,7 @@
>  #define      PPPOE_TAG_ACCOOKIE      0x0104          /* AC cookie */
>  #define      PPPOE_TAG_VENDOR        0x0105          /* vendor specific */
>  #define      PPPOE_TAG_RELAYSID      0x0110          /* relay session id */
> +#define      PPPOE_TAG_MAX_PAYLOAD   0x0120          /* max payload */

Mention RFC 4638 here?

>  #define      PPPOE_TAG_SNAME_ERR     0x0201          /* service name error */
>  #define      PPPOE_TAG_ACSYS_ERR     0x0202          /* AC system error */
>  #define      PPPOE_TAG_GENERIC_ERR   0x0203          /* generic error */
> @@ -895,7 +896,7 @@
>                               return ENXIO;
>                       }
>  
> -                     if (sc->sc_sppp.pp_if.if_mtu >
> +                     if (sc->sc_sppp.pp_if.if_mtu !=
>                           eth_if->if_mtu - PPPOE_OVERHEAD) {
>                               sc->sc_sppp.pp_if.if_mtu = eth_if->if_mtu -
>                                   PPPOE_OVERHEAD;
> @@ -1041,6 +1042,9 @@
>               l2 = strlen(sc->sc_concentrator_name);
>               len += 2 + 2 + l2;
>       }
> +     if (sc->sc_sppp.pp_if.if_mtu > PPPOE_MAXMTU) {
> +             len += 2 + 2 + 2;
> +     }
>  
>       /* allocate a buffer */
>       m0 = pppoe_get_mbuf(len + PPPOE_HEADERLEN);     /* header len + payload 
> len */
> @@ -1067,6 +1071,13 @@
>       PPPOE_ADD_16(p, PPPOE_TAG_HUNIQUE);
>       PPPOE_ADD_16(p, sizeof(sc));
>       memcpy(p, &sc, sizeof sc);
> +     p += sizeof(sc);
> +
> +     if (sc->sc_sppp.pp_if.if_mtu > PPPOE_MAXMTU) {
> +             PPPOE_ADD_16(p, PPPOE_TAG_MAX_PAYLOAD);
> +             PPPOE_ADD_16(p, 2);
> +             PPPOE_ADD_16(p, (uint16_t)sc->sc_sppp.pp_if.if_mtu);
> +     }
>  
>  #ifdef PPPOE_DEBUG
>       p += sizeof sc;
> @@ -1283,6 +1294,9 @@
>               len += 2 + 2 + sc->sc_ac_cookie_len;    /* AC cookie */
>       if (sc->sc_relay_sid_len > 0)
>               len += 2 + 2 + sc->sc_relay_sid_len;    /* Relay SID */
> +     if (sc->sc_sppp.pp_if.if_mtu > PPPOE_MAXMTU) {
> +             len += 2 + 2 + 2;
> +     }

It would be nice to duplicate the MTU check here. You could set a variable
"tag_max_payload_len" to either 6 or 0 above.

        Kind regards

-- 
Matthias Scheler                                  http://zhadum.org.uk/