On Mon, Nov 08, 2021 at 11:52:52AM +0100, Bjorn Ketelaars wrote:
> Diff below does two things:
> 1. add PPP IPCP extensions for name server addresses (rfc1877) to
> sppp(4)
> 2. propose negotiated name servers from sppp(4) to resolvd(8) using
> RTM_PROPOSAL_STATIC route messages.
>
> With this I'm able to use DNS servers as provided by my ISP who uses
> PPPoE. resolv.conf is updated by resolvd(8) as function of status
> changes of pppoe(4).
>
> rfc1877 implementation is derived from code from NetBSD, and inspired by
> [0] and [1]. Borrowed code from umb(4) for the route messages.
>
> Opinions/comments/OK?
>
>
> [0] https://marc.info/?l=openbsd-tech&m=134943767022961&w=2
> [1] https://marc.info/?l=openbsd-tech&m=159405677416423&w=2
>
>
> diff --git sys/net/if_sppp.h sys/net/if_sppp.h
> index ff559fcc369..f2dab61e46b 100644
> --- sys/net/if_sppp.h
> +++ sys/net/if_sppp.h
> @@ -132,6 +132,8 @@ struct sipcp {
> * original one here, in network byte order */
> u_int32_t req_hisaddr; /* remote address requested (IPv4) */
> u_int32_t req_myaddr; /* local address requested (IPv4) */
> +#define IPCP_MAX_DNSSRV 2
> + u_int32_t dns[IPCP_MAX_DNSSRV]; /* IPv4 DNS servers (RFC 1877) */
I would very much prefer to use struct in_addr here (or maybe in_addr_t).
It makes it more clear what the data is.
> #ifdef INET6
> struct in6_aliasreq req_ifid; /* local ifid requested (IPv6) */
> #endif
> diff --git sys/net/if_spppsubr.c sys/net/if_spppsubr.c
> index ac1dc9a709d..225ad8f5a3e 100644
> --- sys/net/if_spppsubr.c
> +++ sys/net/if_spppsubr.c
> @@ -132,6 +132,10 @@
> #define IPCP_OPT_ADDRESSES 1 /* both IP addresses; deprecated */
> #define IPCP_OPT_COMPRESSION 2 /* IP compression protocol (VJ) */
> #define IPCP_OPT_ADDRESS 3 /* local IP address */
> +#define IPCP_OPT_PRIMDNS 129 /* primary remote dns address */
> +#define _IPCP_OPT_PRIMDNS_ 4 /* set/check option */
> +#define IPCP_OPT_SECDNS 131 /* secondary remote dns address
> */
> +#define _IPCP_OPT_SECDNS_ 5 /* set/check option */
As kn@ noted this underscore version is a bit rough.
I wonder if IPCP_OPT definitions (from the standard) should be decoupled
from the bitmask values to enable/disable a feature.
> #define IPV6CP_OPT_IFID 1 /* interface identifier */
> #define IPV6CP_OPT_COMPRESSION 2 /* IPv6 compression protocol */
> @@ -338,6 +342,8 @@ void sppp_update_gw(struct ifnet *ifp);
> void sppp_set_ip_addrs(void *);
> void sppp_clear_ip_addrs(void *);
> void sppp_set_phase(struct sppp *sp);
> +void sppp_update_dns(struct ifnet *ifp);
> +void sppp_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt);
>
> /* our control protocol descriptors */
> static const struct cp lcp = {
> @@ -701,6 +707,7 @@ sppp_attach(struct ifnet *ifp)
>
> sp->pp_if.if_type = IFT_PPP;
> sp->pp_if.if_output = sppp_output;
> + sp->pp_if.if_rtrequest = sppp_rtrequest;
> ifq_set_maxlen(&sp->pp_if.if_snd, 50);
> mq_init(&sp->pp_cpq, 50, IPL_NET);
> sp->pp_loopcnt = 0;
> @@ -2519,6 +2526,12 @@ sppp_ipcp_RCN_rej(struct sppp *sp, struct lcp_header
> *h, int len)
> sp->ipcp.opts &= ~(1 << IPCP_OPT_COMPRESS);
> break;
> #endif
> + case IPCP_OPT_PRIMDNS:
> + sp->ipcp.opts &= ~(1 << _IPCP_OPT_PRIMDNS_);
> + break;
> + case IPCP_OPT_SECDNS:
> + sp->ipcp.opts &= ~(1 << _IPCP_OPT_SECDNS_);
> + break;
> }
> }
> if (debug)
> @@ -2584,6 +2597,16 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struct lcp_header
> *h, int len)
> */
> break;
> #endif
> + case IPCP_OPT_PRIMDNS:
> + if (len >= 6 && p[1] == 6)
> + sp->ipcp.dns[0] = p[2] << 24 | p[3] << 16 |
> + p[4] << 8 | p[5];
Please use memcpy and here. Since this remains in network byte order it
should just work.
> + break;
> + case IPCP_OPT_SECDNS:
> + if (len >= 6 && p[1] == 6)
> + sp->ipcp.dns[1] = p[2] << 24 | p[3] << 16 |
> + p[4] << 8 | p[5];
See above.
> + break;
> }
> }
> if (debug)
> @@ -2612,6 +2635,7 @@ sppp_ipcp_tls(struct sppp *sp)
> IPCP_MYADDR_DYN|IPCP_HISADDR_DYN);
> sp->ipcp.req_myaddr = 0;
> sp->ipcp.req_hisaddr = 0;
> + memset(&sp->ipcp.dns, 0, sizeof sp->ipcp.dns);
>
> sppp_get_ip_addrs(sp, &myaddr, &hisaddr, 0);
> /*
> @@ -2644,6 +2668,10 @@ sppp_ipcp_tls(struct sppp *sp)
> sp->ipcp.flags |= IPCP_HISADDR_DYN;
> }
>
> + /* negotiate name server addresses */
> + sp->ipcp.opts |= (1 << _IPCP_OPT_PRIMDNS_);
> + sp->ipcp.opts |= (1 << _IPCP_OPT_SECDNS_);
> +
> /* indicate to LCP that it must stay alive */
> sp->lcp.protos |= (1 << IDX_IPCP);
> }
> @@ -2663,7 +2691,7 @@ sppp_ipcp_tlf(struct sppp *sp)
> void
> sppp_ipcp_scr(struct sppp *sp)
> {
> - char opt[6 /* compression */ + 6 /* address */];
> + char opt[6 /* compression */ + 6 /* address */ + 12 /* dns addresses
> */];
This line is too long now. I feel this should be documented in a different
way.
> u_int32_t ouraddr;
> int i = 0;
>
> @@ -2692,6 +2720,24 @@ sppp_ipcp_scr(struct sppp *sp)
> opt[i++] = ouraddr;
> }
>
> + if (sp->ipcp.opts & (1 << _IPCP_OPT_PRIMDNS_)) {
> + opt[i++] = IPCP_OPT_PRIMDNS;
> + opt[i++] = 6;
> + opt[i++] = sp->ipcp.dns[0] >> 24;
> + opt[i++] = sp->ipcp.dns[0] >> 16;
> + opt[i++] = sp->ipcp.dns[0] >> 8;
> + opt[i++] = sp->ipcp.dns[0];
Again this calls for memcpy()
> + }
> +
> + if (sp->ipcp.opts & (1 << _IPCP_OPT_SECDNS_)) {
> + opt[i++] = IPCP_OPT_SECDNS;
> + opt[i++] = 6;
> + opt[i++] = sp->ipcp.dns[1] >> 24;
> + opt[i++] = sp->ipcp.dns[1] >> 16;
> + opt[i++] = sp->ipcp.dns[1] >> 8;
> + opt[i++] = sp->ipcp.dns[1];
> + }
> +
> sp->confid[IDX_IPCP] = ++sp->pp_seq;
> sppp_cp_send(sp, PPP_IPCP, CONF_REQ, sp->confid[IDX_IPCP], i, opt);
> }
> @@ -4242,6 +4288,7 @@ sppp_set_ip_addrs(void *arg1)
> goto out;
> }
> sppp_update_gw(ifp);
> + sppp_update_dns(ifp);
> }
> out:
> NET_UNLOCK();
> @@ -4302,6 +4349,9 @@ sppp_clear_ip_addrs(void *arg1)
> goto out;
> }
> sppp_update_gw(ifp);
> +
> + memset(sp->ipcp.dns, 0, sizeof(sp->ipcp.dns));
> + sppp_update_dns(ifp);
> }
> out:
> NET_UNLOCK();
> @@ -4721,6 +4771,8 @@ sppp_ipcp_opt_name(u_char opt)
> case IPCP_OPT_ADDRESSES: return "addresses";
> case IPCP_OPT_COMPRESSION: return "compression";
> case IPCP_OPT_ADDRESS: return "address";
> + case IPCP_OPT_PRIMDNS: return "primdns";
> + case IPCP_OPT_SECDNS: return "secdns";
> }
> snprintf (buf, sizeof buf, "0x%x", opt);
> return buf;
> @@ -4851,3 +4903,45 @@ sppp_set_phase(struct sppp *sp)
> if_link_state_change(ifp);
> }
> }
> +
> +void
> +sppp_update_dns(struct ifnet *ifp)
> +{
> + struct rt_addrinfo info;
> + struct sockaddr_rtdns rtdns;
> + struct sppp *sp = ifp->if_softc;
> + u_int32_t dns;
> + int i, flag = 0;
> + size_t sz = 0;
> +
> + memset(&rtdns, 0, sizeof(rtdns));
> + memset(&info, 0, sizeof(info));
> +
> + for (i = 0; i < IPCP_MAX_DNSSRV; i++) {
> + dns = ntohl(sp->ipcp.dns[i]);
> + if (dns == INADDR_ANY)
> + break;
> + sz = sizeof(dns);
> + memcpy(rtdns.sr_dns + i * sz, &dns, sz);
> + flag = RTF_UP;
> + }
> +
> + rtdns.sr_family = AF_INET;
> + rtdns.sr_len = 2 + i * sz;
> + info.rti_info[RTAX_DNS] = srtdnstosa(&rtdns);
> +
> + rtm_proposal(ifp, &info, flag, RTP_PROPOSAL_STATIC);
> +}
> +
> +void
> +sppp_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt)
> +{
> + if (req == RTM_PROPOSAL) {
> + KERNEL_LOCK();
> + sppp_update_dns(ifp);
> + KERNEL_UNLOCK();
> + return;
> + }
> +
> + p2p_rtrequest(ifp, req, rt);
> +}
>
Apart from that good work :)
--
:wq Claudio
