Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework default route handling. #544

Merged
merged 1 commit into from
Feb 23, 2025
Merged

Rework default route handling. #544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 41 additions & 36 deletions pppd/ipcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ u_int32_t netmask = 0; /* IP netmask to set on interface */
bool disable_defaultip = 0; /* Don't use hostname for default IP adrs */
bool noremoteip = 0; /* Let him have no IP address */

unsigned dfl_route_metric = 0; /* metric of the default route to set over the PPP link */

ip_up_hook_fn *ip_up_hook = NULL;
ip_down_hook_fn *ip_down_hook = NULL;
ip_choose_hook_fn *ip_choose_hook = NULL;
Expand Down Expand Up @@ -136,6 +138,8 @@ static int setvjslots (char **);
static int setdnsaddr (char **);
static int setwinsaddr (char **);
static int setnetmask (char **);
static int replacedefaultroute_nonfunctional();

int setipaddr (char *, char **, int);
static void printipaddr (struct option *, void (*)(void *, char *,...),void *);

Expand Down Expand Up @@ -196,15 +200,20 @@ static struct option ipcp_option_list[] = {
"disable defaultroute option", OPT_ALIAS | OPT_A2CLR,
&ipcp_wantoptions[0].default_route },

{ "defaultroute-metric", o_int, &dfl_route_metric,
"Metric to use for the default route (Linux only; default 0)",
OPT_PRIV|OPT_LLIMIT|OPT_INITONLY, NULL, 0, -1 },

#ifdef __linux__
{ "replacedefaultroute", o_bool,
&ipcp_wantoptions[0].replace_default_route,
"Replace default route", OPT_PRIV | 1
},
{ "noreplacedefaultroute", o_bool,
&ipcp_wantoptions[0].replace_default_route,
"Do not replace default route", 0 },
{ "replacedefaultroute", o_special_noarg,
&replacedefaultroute_nonfunctional,
"Removed option to replace default route", OPT_PRIV
},
{ "noreplacedefaultroute", o_special_noarg,
&replacedefaultroute_nonfunctional,
"Removed option to not replace default route", 0 },
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I prefer not to break existing working setups by removing options, if it is reasonable for the option to become a no-op (or at worst emit a warning). Could these be made no-ops?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I can't reply by the 2^31 issue, the kernel type is unsigned, but sure, as long as we can restrict to positive integer that's fine by me. If you need metrics >=2^31 you can do the relevant patching.

Re no-op, will handle. Normally I'd argue for "let it break so that someone knows something need to change", but in this case since it might well relate to something that the admin needs to gain access to the system ... I'm happy to concede.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulusmack please check if you're happy with the change.

I took the opportunity to move options out of pppd/options.c and into the NCPs where they belong at the same time (at least those that I touched). I trust this will be in-order as it honestly makes more sense to have IPCP options reside in ipcp.c and IPv6CP options in ipv6cp.c.

I did (from the initial PR) include some whitespace only changes into the PR. If that's an issue I can rework to exclude that. Didn't make the same mistake on other files (vim automatically cleans up trailing whitespace and a few others on my system).


{ "proxyarp", o_bool, &ipcp_wantoptions[0].proxy_arp,
"Add proxy ARP entry", OPT_ENABLE|1, &ipcp_allowoptions[0].proxy_arp },
{ "noproxyarp", o_bool, &ipcp_allowoptions[0].proxy_arp,
Expand Down Expand Up @@ -284,7 +293,7 @@ struct protent ipcp_protent = {
ip_active_pkt
};

static void ipcp_clear_addrs (int, u_int32_t, u_int32_t, bool);
static void ipcp_clear_addrs (int, u_int32_t, u_int32_t);
static void ipcp_script (char *, int); /* Run an up/down script */
static void ipcp_script_done (void *);

Expand Down Expand Up @@ -437,7 +446,7 @@ setipaddr(char *arg, char **argv, int doit)
return 0;
if (!doit)
return 1;

/*
* If colon first character, then no local addr.
*/
Expand All @@ -459,7 +468,7 @@ setipaddr(char *arg, char **argv, int doit)
*colon = ':';
prio_local = option_priority;
}

/*
* If colon last character, then no remote addr.
*/
Expand Down Expand Up @@ -527,6 +536,14 @@ setnetmask(char **argv)
return (1);
}

static int
replacedefaultroute_nonfunctional()
{
ppp_option_error("replacedefaultroute and noreplacedefaultroute route options no longer have any effect.");
ppp_option_error("Please refer to the man page for defaultroute and defaultroute-metric.");
return 1;
}

int
parse_dotted_ip(char *p, u_int32_t *vp)
{
Expand Down Expand Up @@ -851,7 +868,7 @@ ipcp_addci(fsm *f, u_char *ucp, int *lenp)
ADDCIWINS(CI_MS_WINS1, go->req_wins1, go->winsaddr[0]);

ADDCIWINS(CI_MS_WINS2, go->req_wins2, go->winsaddr[1]);

*lenp -= len;
}

Expand Down Expand Up @@ -1456,7 +1473,7 @@ ipcp_reqci(fsm *f, u_char *inp, int *len, int reject_if_disagree)
* Reset all his options.
*/
BZERO(ho, sizeof(*ho));

/*
* Process all his options.
*/
Expand Down Expand Up @@ -1566,7 +1583,7 @@ ipcp_reqci(fsm *f, u_char *inp, int *len, int reject_if_disagree)
wo->req_addr = 0; /* don't NAK with 0.0.0.0 later */
break;
}

ho->neg_addr = 1;
ho->hisaddr = ciaddr1;
break;
Expand Down Expand Up @@ -1610,7 +1627,7 @@ ipcp_reqci(fsm *f, u_char *inp, int *len, int reject_if_disagree)
orc = CONFNAK;
}
break;

case CI_COMPRESSTYPE:
if (!ao->neg_vj ||
(cilen != CILEN_VJ && cilen != CILEN_COMPRESS)) {
Expand All @@ -1629,7 +1646,7 @@ ipcp_reqci(fsm *f, u_char *inp, int *len, int reject_if_disagree)
ho->vj_protocol = cishort;
if (cilen == CILEN_VJ) {
GETCHAR(maxslotindex, p);
if (maxslotindex > ao->maxslotindex) {
if (maxslotindex > ao->maxslotindex) {
orc = CONFNAK;
if (!reject_if_disagree){
DECPTR(1, p);
Expand Down Expand Up @@ -1776,8 +1793,7 @@ ip_demand_conf(int u)
if (!sifnpmode(u, PPP_IP, NPMODE_QUEUE))
return 0;
if (wo->default_route)
if (sifdefaultroute(u, wo->ouraddr, wo->hisaddr,
wo->replace_default_route))
if (sifdefaultroute(u, wo->ouraddr, wo->hisaddr))
default_route_set[u] = 1;
if (wo->proxy_arp)
if (sifproxyarp(u, wo->hisaddr))
Expand Down Expand Up @@ -1878,8 +1894,7 @@ ipcp_up(fsm *f)
*/
if (demand) {
if (go->ouraddr != wo->ouraddr || ho->hisaddr != wo->hisaddr) {
ipcp_clear_addrs(f->unit, wo->ouraddr, wo->hisaddr,
wo->replace_default_route);
ipcp_clear_addrs(f->unit, wo->ouraddr, wo->hisaddr);
if (go->ouraddr != wo->ouraddr) {
warn("Local IP address changed to %I", go->ouraddr);
ppp_script_setenv("OLDIPLOCAL", ip_ntoa(wo->ouraddr), 0);
Expand All @@ -1904,9 +1919,8 @@ ipcp_up(fsm *f)
}

/* assign a default route through the interface if required */
if (ipcp_wantoptions[f->unit].default_route)
if (sifdefaultroute(f->unit, go->ouraddr, ho->hisaddr,
wo->replace_default_route))
if (ipcp_wantoptions[f->unit].default_route)
if (sifdefaultroute(f->unit, go->ouraddr, ho->hisaddr))
default_route_set[f->unit] = 1;

/* Make a proxy ARP entry if requested. */
Expand Down Expand Up @@ -1964,9 +1978,8 @@ ipcp_up(fsm *f)
sifnpmode(f->unit, PPP_IP, NPMODE_PASS);

/* assign a default route through the interface if required */
if (ipcp_wantoptions[f->unit].default_route)
if (sifdefaultroute(f->unit, go->ouraddr, ho->hisaddr,
wo->replace_default_route))
if (ipcp_wantoptions[f->unit].default_route)
if (sifdefaultroute(f->unit, go->ouraddr, ho->hisaddr))
default_route_set[f->unit] = 1;

/* Make a proxy ARP entry if requested. */
Expand Down Expand Up @@ -2043,7 +2056,7 @@ ipcp_down(fsm *f)
sifnpmode(f->unit, PPP_IP, NPMODE_DROP);
sifdown(f->unit);
ipcp_clear_addrs(f->unit, ipcp_gotoptions[f->unit].ouraddr,
ipcp_hisoptions[f->unit].hisaddr, 0);
ipcp_hisoptions[f->unit].hisaddr);
}

/* Execute the ip-down script */
Expand All @@ -2059,21 +2072,13 @@ ipcp_down(fsm *f)
* proxy arp entries, etc.
*/
static void
ipcp_clear_addrs(int unit, u_int32_t ouraddr, u_int32_t hisaddr, bool replacedefaultroute)
ipcp_clear_addrs(int unit, u_int32_t ouraddr, u_int32_t hisaddr)
{
if (proxy_arp_set[unit]) {
cifproxyarp(unit, hisaddr);
proxy_arp_set[unit] = 0;
}
/* If replacedefaultroute, sifdefaultroute will be called soon
* with replacedefaultroute set and that will overwrite the current
* default route. This is the case only when doing demand, otherwise
* during demand, this cifdefaultroute would restore the old default
* route which is not what we want in this case. In the non-demand
* case, we'll delete the default route and restore the old if there
* is one saved by an sifdefaultroute with replacedefaultroute.
*/
if (!replacedefaultroute && default_route_set[unit]) {
if (default_route_set[unit]) {
cifdefaultroute(unit, ouraddr, hisaddr);
default_route_set[unit] = 0;
}
Expand Down
1 change: 0 additions & 1 deletion pppd/ipcp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ typedef struct ipcp_options {
bool old_addrs; /* Use old (IP-Addresses) option? */
bool req_addr; /* Ask peer to send IP address? */
bool default_route; /* Assign default route through interface? */
bool replace_default_route; /* Replace default route through interface? */
bool proxy_arp; /* Make proxy ARP entry for peer? */
bool neg_vj; /* Van Jacobson Compression? */
bool old_vj; /* use old (short) form of VJ option? */
Expand Down
6 changes: 6 additions & 0 deletions pppd/ipv6cp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,8 @@ ipv6cp_options ipv6cp_allowoptions[NUM_PPP]; /* Options we allow peer to request
ipv6cp_options ipv6cp_hisoptions[NUM_PPP]; /* Options that we ack'd */
int no_ifaceid_neg = 0;

unsigned dfl_route6_metric = 0; /* metric of the default route to set over the PPP link */

/* local vars */
static int default_route_set[NUM_PPP]; /* Have set up a default route */
static int ipv6cp_is_up;
Expand Down Expand Up @@ -258,6 +260,10 @@ static struct option ipv6cp_option_list[] = {
"disable defaultroute6 option", OPT_ALIAS | OPT_A2CLR,
&ipv6cp_wantoptions[0].default_route },

{ "defaultroute6-metric", o_int, &dfl_route6_metric,
"Metric to use for the default route (Linux only; default 0)",
OPT_PRIV|OPT_LLIMIT|OPT_INITONLY, NULL, 0, -1 },

{ "ipv6cp-use-ipaddr", o_bool, &ipv6cp_allowoptions[0].use_ip,
"Use (default) IPv4 addresses for both local and remote interface identifiers", 1 },
{ "ipv6cp-use-persistent", o_bool, &ipv6cp_wantoptions[0].use_persistent,
Expand Down
5 changes: 0 additions & 5 deletions pppd/options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ bool dryrun; /* print out option values and exit */
char *domain; /* domain name set by domain option */
int child_wait = 5; /* # seconds to wait for children at exit */
struct userenv *userenv_list; /* user environment variables */
int dfl_route_metric = -1; /* metric of the default route to set over the PPP link */

#ifdef PPP_WITH_IPV6CP
char path_ipv6up[MAXPATHLEN]; /* pathname of ipv6-up script */
Expand Down Expand Up @@ -331,10 +330,6 @@ struct option general_options[] = {
"Unset user environment variable",
OPT_A2PRINTER | OPT_NOPRINT, (void *)user_unsetprint },

{ "defaultroute-metric", o_int, &dfl_route_metric,
"Metric to use for the default route (Linux only; -1 for default behavior)",
OPT_PRIV|OPT_LLIMIT|OPT_INITONLY, NULL, 0, -1 },

{ "net-init-script", o_string, path_net_init,
"Set pathname of net-init script",
OPT_PRIV|OPT_STATIC, NULL, MAXPATHLEN },
Expand Down
2 changes: 1 addition & 1 deletion pppd/pppd-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ int sif6addr(int, eui64_t, eui64_t);
int cif6addr(int, eui64_t, eui64_t);
/* Remove an IPv6 address from i/f */
#endif
int sifdefaultroute(int, u_int32_t, u_int32_t, bool replace_default_rt);
int sifdefaultroute(int, u_int32_t, u_int32_t);
/* Create default route through i/f */
int cifdefaultroute(int, u_int32_t, u_int32_t);
/* Delete default route through i/f */
Expand Down
19 changes: 6 additions & 13 deletions pppd/pppd.8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,8 @@ This entry is removed when the PPP connection is broken. This option
is privileged if the \fInodefaultroute\fR option has been specified.
.TP
.B defaultroute-metric
Define the metric of the \fIdefaultroute\fR and only add it if there
is no other default route with the same metric. With the default
value of -1, the route is only added if there is no default route at
all.
.TP
.B replacedefaultroute
This option is a flag to the defaultroute option. If defaultroute is
set and this flag is also set, pppd replaces an existing default route
with the new default route. This option is privileged.
Define the metric of the \fIdefaultroute\fR. By default the default route will
be added with a metric of 0. This option is privileged.
.TP
.B disconnect \fIscript
Execute the command specified by \fIscript\fR, by passing it to a
Expand Down Expand Up @@ -367,6 +360,10 @@ configured by kernel automatically too based on ICMPv6 Router Advertisement
packets. This option may conflict with kernel IPv6 route setup and should
be used only for broken IPv6 networks.
.TP
.B defaultroute6-metric
Define the metric of the \fIdefaultroute6\fR. By default the default route will
be added with a metric of 0. This option is privileged.
.TP
.B deflate \fInr,nt
Request that the peer compress packets that it sends, using the
Deflate scheme, with a maximum window size of \fI2**nr\fR bytes, and
Expand Down Expand Up @@ -802,10 +799,6 @@ Disable the \fIdefaultroute\fR option. The system administrator who
wishes to prevent users from adding a default route with pppd
can do so by placing this option in the /etc/ppp/options file.
.TP
.B noreplacedefaultroute
Disable the \fIreplacedefaultroute\fR option. This allows to disable a
\fIreplacedefaultroute\fR option set previously in the configuration.
.TP
.B nodefaultroute6
Disable the \fIdefaultroute6\fR option. The system administrator who
wishes to prevent users from adding a default route with pppd
Expand Down
Loading