Thread: [Accel-ppp-users] [PATCH 0/7] iputils: stricter route deletion and cleanup
Status: Beta
Brought to you by:
xebd
Menu
▾
▴
accel-ppp-users
|
From: Guillaume N. <g....@al...> - 2018-12-19 18:13:07
|
This series brings the ability to use the same parameters in
iproute_del() as in iproute_add(). The objective is to ensure that
we only delete routes that have the same properties as the ones we
originally inserted.
The rest of the series makes some cleanup in the rest of iputils.c,
simplifying a bit netlink header generation.
Patch 1 adds the 'src' and 'gw' parameters to iproute_del().
Patch 2 uses the new 'gw' parameter in radius.c.
Patch 7 uses 'src' in ipoe.c. I put this patch at the end of the series
and mark it RFC as I don't have any working IPoE environment to test
it. The route insertion and deletion process looks quite clear, and
setting 'src' there seems to fit properly.
Then patches 3 to 6 simplify a little bit the way netlink messages are
generated, without modifying their semantic.
Guillaume Nault (7):
iputils: add 'src' and 'gw' parameters to iproute_del()
radius: specify gateway in iproute_del()
iputils: set scope depending on gateway in iproute_{add,del}()
iputils: always set scope to RT_SCOPE_UNIVERSE in ip6route_{add,del}()
iputils: remove NLM_F_CREATE flag from ip6{route,addr}_del()
iputils: remove unnecessary NLM_F_ACK
ipoe: stricter route deletion
accel-pppd/ctrl/ipoe/ipoe.c | 10 +++++-----
accel-pppd/libnetlink/iputils.c | 29 ++++++++++++++++-------------
accel-pppd/libnetlink/iputils.h | 2 +-
accel-pppd/radius/radius.c | 2 +-
4 files changed, 23 insertions(+), 20 deletions(-)
--
2.20.1
|
|
From: Guillaume N. <g....@al...> - 2018-12-19 18:13:21
|
Be more specific about which route we want to remove. By not specifying
the gateway we could remove a different route than the one we
originally inserted.
Signed-off-by: Guillaume Nault <g....@al...>
---
accel-pppd/radius/radius.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/accel-pppd/radius/radius.c b/accel-pppd/radius/radius.c
index abcb2e5d..d910c9ce 100644
--- a/accel-pppd/radius/radius.c
+++ b/accel-pppd/radius/radius.c
@@ -628,7 +628,7 @@ static void ses_finishing(struct ap_session *ses)
for (fr = rpd->fr; fr; fr = fr->next) {
if (fr->gw)
- iproute_del(0, 0, fr->dst, 0, 3, fr->mask, fr->prio);
+ iproute_del(0, 0, fr->dst, fr->gw, 3, fr->mask, fr->prio);
}
if (rpd->acct_started || rpd->acct_req)
--
2.20.1
|
|
From: Guillaume N. <g....@al...> - 2018-12-19 18:13:21
|
Rework iproute_del() to have the same parameters as iproute_add().
This will allow callers to specify more precisely the route they want
to delete.
Callers will later be converted to make use of these parameters to
ensure that the removed route precisely matches the one that was
originaly inserted.
Signed-off-by: Guillaume Nault <g....@al...>
---
accel-pppd/ctrl/ipoe/ipoe.c | 4 ++--
accel-pppd/libnetlink/iputils.c | 9 ++++++---
accel-pppd/libnetlink/iputils.h | 2 +-
accel-pppd/radius/radius.c | 2 +-
4 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/accel-pppd/ctrl/ipoe/ipoe.c b/accel-pppd/ctrl/ipoe/ipoe.c
index ed79b656..0fe16ae1 100644
--- a/accel-pppd/ctrl/ipoe/ipoe.c
+++ b/accel-pppd/ctrl/ipoe/ipoe.c
@@ -1167,9 +1167,9 @@ static void ipoe_session_finished(struct ap_session *s)
} else if (ses->started) {
if (!serv->opt_ifcfg) {
if (serv->opt_ip_unnumbered)
- iproute_del(serv->ifindex, ses->yiaddr, conf_proto, 32, 0);
+ iproute_del(serv->ifindex, 0, ses->yiaddr, 0, conf_proto, 32, 0);
else
- iproute_del(serv->ifindex, ses->yiaddr, conf_proto, ses->mask, 0);
+ iproute_del(serv->ifindex, 0, ses->yiaddr, 0, conf_proto, ses->mask, 0);
}
}
diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c
index 343088f3..5f830f09 100644
--- a/accel-pppd/libnetlink/iputils.c
+++ b/accel-pppd/libnetlink/iputils.c
@@ -500,7 +500,7 @@ int __export iproute_add(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw
return r;
}
-int __export iproute_del(int ifindex, in_addr_t dst, int proto, int mask, uint32_t prio)
+int __export iproute_del(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw, int proto, int mask, uint32_t prio)
{
struct ipaddr_req {
struct nlmsghdr n;
@@ -525,12 +525,15 @@ int __export iproute_del(int ifindex, in_addr_t dst, int proto, int mask, uint32
req.i.rtm_type = RTN_UNICAST;
req.i.rtm_dst_len = mask;
- addattr32(&req.n, sizeof(req), RTA_DST, dst);
-
if (ifindex)
addattr32(&req.n, sizeof(req), RTA_OIF, ifindex);
+ if (src)
+ addattr32(&req.n, sizeof(req), RTA_PREFSRC, src);
+ if (gw)
+ addattr32(&req.n, sizeof(req), RTA_GATEWAY, gw);
if (prio)
addattr32(&req.n, sizeof(req), RTA_PRIORITY, prio);
+ addattr32(&req.n, sizeof(req), RTA_DST, dst);
if (rtnl_talk(rth, &req.n, 0, 0, NULL, NULL, NULL, 0) < 0)
r = -1;
diff --git a/accel-pppd/libnetlink/iputils.h b/accel-pppd/libnetlink/iputils.h
index 15104b16..78224745 100644
--- a/accel-pppd/libnetlink/iputils.h
+++ b/accel-pppd/libnetlink/iputils.h
@@ -21,7 +21,7 @@ int ipaddr_del(int ifindex, in_addr_t addr, int mask);
int ipaddr_del_peer(int ifindex, in_addr_t addr, in_addr_t peer);
int iproute_add(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw, int proto, int mask, uint32_t prio);
-int iproute_del(int ifindex, in_addr_t dst, int proto, int mask, uint32_t prio);
+int iproute_del(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw, int proto, int mask, uint32_t prio);
in_addr_t iproute_get(in_addr_t dst, in_addr_t *gw);
int ip6route_add(int ifindex, const struct in6_addr *dst, int pref_len, const struct in6_addr *gw, int proto, uint32_t prio);
diff --git a/accel-pppd/radius/radius.c b/accel-pppd/radius/radius.c
index 062e3b72..abcb2e5d 100644
--- a/accel-pppd/radius/radius.c
+++ b/accel-pppd/radius/radius.c
@@ -628,7 +628,7 @@ static void ses_finishing(struct ap_session *ses)
for (fr = rpd->fr; fr; fr = fr->next) {
if (fr->gw)
- iproute_del(0, fr->dst, 3, fr->mask, fr->prio);
+ iproute_del(0, 0, fr->dst, 0, 3, fr->mask, fr->prio);
}
if (rpd->acct_started || rpd->acct_req)
--
2.20.1
|
|
From: Guillaume N. <g....@al...> - 2018-12-19 18:13:23
|
>From a logical point of view, we have link scope if no gateway is present, and global scope otherwise. Therefore it makes more sense to set rtm_scope depending on 'gw' rather than on 'ifindex'. Currently, callers of iproute_add() and iproute_del() either set 'ifindex' or 'gw', but never both. So even if confusing, the current code results in right scope selection. However one can't figure this out without analysing every caller. We should set rtm_scope based on the presence of the gateway instead. Given the current code base, that doesn't change the end result, but that better maches the scope concept. Also, that's the way iproute2 does its selection. Furthermore, it'd be perfectly valid to have both 'iface' and 'gw' set. In that case, scope should be RT_SCOPE_UNIVERSE instead of RT_SCOPE_LINK. Basing scope selection on 'gw' makes this case work correctly. Signed-off-by: Guillaume Nault <g....@al...> --- accel-pppd/libnetlink/iputils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c index 5f830f09..ffa57267 100644 --- a/accel-pppd/libnetlink/iputils.c +++ b/accel-pppd/libnetlink/iputils.c @@ -477,7 +477,7 @@ int __export iproute_add(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw req.n.nlmsg_type = RTM_NEWROUTE; req.i.rtm_family = AF_INET; req.i.rtm_table = RT_TABLE_MAIN; - req.i.rtm_scope = ifindex ? RT_SCOPE_LINK : RT_SCOPE_UNIVERSE; + req.i.rtm_scope = gw ? RT_SCOPE_UNIVERSE : RT_SCOPE_LINK; req.i.rtm_protocol = proto; req.i.rtm_type = RTN_UNICAST; req.i.rtm_dst_len = mask; @@ -520,7 +520,7 @@ int __export iproute_del(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw req.n.nlmsg_type = RTM_DELROUTE; req.i.rtm_family = AF_INET; req.i.rtm_table = RT_TABLE_MAIN; - req.i.rtm_scope = ifindex ? RT_SCOPE_LINK : RT_SCOPE_UNIVERSE; + req.i.rtm_scope = gw ? RT_SCOPE_UNIVERSE : RT_SCOPE_LINK; req.i.rtm_protocol = proto; req.i.rtm_type = RTN_UNICAST; req.i.rtm_dst_len = mask; -- 2.20.1 |
|
From: Guillaume N. <g....@al...> - 2018-12-19 18:13:25
|
No need to be clever here. All IPv6 routes have global scope (kernel ignores rtm_scope for IPv6 and always reports RT_SCOPE_UNIVERSE when dumping such routes). Signed-off-by: Guillaume Nault <g....@al...> --- accel-pppd/libnetlink/iputils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c index ffa57267..f04f3489 100644 --- a/accel-pppd/libnetlink/iputils.c +++ b/accel-pppd/libnetlink/iputils.c @@ -563,7 +563,7 @@ int __export ip6route_add(int ifindex, const struct in6_addr *dst, int pref_len, req.n.nlmsg_type = RTM_NEWROUTE; req.i.rtm_family = AF_INET6; req.i.rtm_table = RT_TABLE_MAIN; - req.i.rtm_scope = (pref_len == 128) ? RT_SCOPE_HOST : RT_SCOPE_LINK; + req.i.rtm_scope = RT_SCOPE_UNIVERSE; req.i.rtm_protocol = proto; req.i.rtm_type = RTN_UNICAST; req.i.rtm_dst_len = pref_len; @@ -604,7 +604,7 @@ int __export ip6route_del(int ifindex, const struct in6_addr *dst, int pref_len, req.n.nlmsg_type = RTM_DELROUTE; req.i.rtm_family = AF_INET6; req.i.rtm_table = RT_TABLE_MAIN; - req.i.rtm_scope = (pref_len == 128) ? RT_SCOPE_HOST : RT_SCOPE_LINK; + req.i.rtm_scope = RT_SCOPE_UNIVERSE; req.i.rtm_protocol = proto; req.i.rtm_type = RTN_UNICAST; req.i.rtm_dst_len = pref_len; -- 2.20.1 |
|
From: Guillaume N. <g....@al...> - 2018-12-19 18:13:29
|
These are deletion requests. NLM_F_CREATE is confusing for readers and ignored by kernel. Signed-off-by: Guillaume Nault <g....@al...> --- accel-pppd/libnetlink/iputils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c index f04f3489..5dfd097b 100644 --- a/accel-pppd/libnetlink/iputils.c +++ b/accel-pppd/libnetlink/iputils.c @@ -600,7 +600,7 @@ int __export ip6route_del(int ifindex, const struct in6_addr *dst, int pref_len, memset(&req, 0, sizeof(req) - 4096); req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)); - req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE; + req.n.nlmsg_flags = NLM_F_REQUEST; req.n.nlmsg_type = RTM_DELROUTE; req.i.rtm_family = AF_INET6; req.i.rtm_table = RT_TABLE_MAIN; @@ -708,7 +708,7 @@ int __export ip6addr_del(int ifindex, struct in6_addr *addr, int prefix_len) memset(&req, 0, sizeof(req) - 4096); req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifaddrmsg)); - req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE; + req.n.nlmsg_flags = NLM_F_REQUEST; req.n.nlmsg_type = RTM_DELADDR; req.i.ifa_family = AF_INET6; req.i.ifa_index = ifindex; -- 2.20.1 |
|
From: Guillaume N. <g....@al...> - 2018-12-19 18:13:29
|
Using NLM_F_ACK in these functions is confusing because they don't parse any netlink response. In fact, NLM_F_ACK is only required internally by rtnl_talk(), which already adds it when its 'answer' parameter is NULL. Therefore it's useless to manually set it in functions that don't set 'answer'. Signed-off-by: Guillaume Nault <g....@al...> --- accel-pppd/libnetlink/iputils.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c index 5dfd097b..a1ededbf 100644 --- a/accel-pppd/libnetlink/iputils.c +++ b/accel-pppd/libnetlink/iputils.c @@ -169,7 +169,7 @@ int __export iplink_set_mtu(int ifindex, int mtu) memset(&req, 0, sizeof(req) - 1024); req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)); - req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK; + req.n.nlmsg_flags = NLM_F_REQUEST; req.n.nlmsg_type = RTM_SETLINK; req.i.ifi_family = AF_UNSPEC; req.i.ifi_index = ifindex; @@ -200,7 +200,7 @@ int __export iplink_vlan_add(const char *ifname, int ifindex, int vid) memset(&req, 0, sizeof(req) - 4096); req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)); - req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_EXCL; + req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL; req.n.nlmsg_type = RTM_NEWLINK; req.i.ifi_family = AF_UNSPEC; @@ -243,7 +243,7 @@ int __export iplink_vlan_del(int ifindex) memset(&req, 0, sizeof(req) - 4096); req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)); - req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK; + req.n.nlmsg_flags = NLM_F_REQUEST; req.n.nlmsg_type = RTM_DELLINK; req.i.ifi_family = AF_UNSPEC; req.i.ifi_index = ifindex; @@ -516,7 +516,7 @@ int __export iproute_del(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw memset(&req, 0, sizeof(req) - 4096); req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)); - req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK; + req.n.nlmsg_flags = NLM_F_REQUEST; req.n.nlmsg_type = RTM_DELROUTE; req.i.rtm_family = AF_INET; req.i.rtm_table = RT_TABLE_MAIN; -- 2.20.1 |
|
From: Guillaume N. <g....@al...> - 2018-12-19 18:13:32
|
Rework the conditionals to make __ipoe_session_activate() and
ipoe_session_finished() follow the same logic:
* Drop the second '!serv->opt_ifcfg' test in __ipoe_session_activate(),
which is is already checked by the parent conditional.
* Invert the order of the tests in ipoe_session_finished(), so that
it uses the same conditions as __ipoe_session_activate().
Finally, set the 'src' parameter in iproute_del(), so that we can be
sure that the deleted route matches the one added by
__ipoe_session_activate().
Signed-off-by: Guillaume Nault <g....@al...>
---
Sending as RFC, as I couldn't test this patch beyond compilation.
Setting the source here looks quite logical, but an external review
would be in order.
accel-pppd/ctrl/ipoe/ipoe.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/accel-pppd/ctrl/ipoe/ipoe.c b/accel-pppd/ctrl/ipoe/ipoe.c
index 0fe16ae1..eb06e7b7 100644
--- a/accel-pppd/ctrl/ipoe/ipoe.c
+++ b/accel-pppd/ctrl/ipoe/ipoe.c
@@ -981,9 +981,9 @@ static void __ipoe_session_activate(struct ipoe_session *ses)
ap_session_activate(&ses->ses);
if (ses->ifindex == -1 && !serv->opt_ifcfg) {
- if (serv->opt_ip_unnumbered == 0)
+ if (!serv->opt_ip_unnumbered)
iproute_add(serv->ifindex, ses->router, ses->yiaddr, 0, conf_proto, ses->mask, 0);
- else if (!serv->opt_ifcfg)
+ else
iproute_add(serv->ifindex, serv->opt_src ?: ses->router, ses->yiaddr, 0, conf_proto, 32, 0);
}
@@ -1166,10 +1166,10 @@ static void ipoe_session_finished(struct ap_session *s)
ipoe_nl_delete(ses->ifindex);
} else if (ses->started) {
if (!serv->opt_ifcfg) {
- if (serv->opt_ip_unnumbered)
- iproute_del(serv->ifindex, 0, ses->yiaddr, 0, conf_proto, 32, 0);
+ if (!serv->opt_ip_unnumbered)
+ iproute_del(serv->ifindex, ses->router, ses->yiaddr, 0, conf_proto, ses->mask, 0);
else
- iproute_del(serv->ifindex, 0, ses->yiaddr, 0, conf_proto, ses->mask, 0);
+ iproute_del(serv->ifindex, serv->opt_src ?: ses->router, ses->yiaddr, 0, conf_proto, 32, 0);
}
}
--
2.20.1
|
|
From: Dmitry K. <xe...@ma...> - 2018-12-20 03:24:16
|
>This series brings the ability to use the same parameters in
>iproute_del() as in iproute_add(). The objective is to ensure that
>we only delete routes that have the same properties as the ones we
>originally inserted.
>
>The rest of the series makes some cleanup in the rest of iputils.c,
>simplifying a bit netlink header generation.
>
>Patch 1 adds the 'src' and 'gw' parameters to iproute_del().
>
>Patch 2 uses the new 'gw' parameter in radius.c.
>
>Patch 7 uses 'src' in ipoe.c. I put this patch at the end of the series
>and mark it RFC as I don't have any working IPoE environment to test
>it. The route insertion and deletion process looks quite clear, and
>setting 'src' there seems to fit properly.
>
>Then patches 3 to 6 simplify a little bit the way netlink messages are
>generated, without modifying their semantic.
>
>Guillaume Nault (7):
> iputils: add 'src' and 'gw' parameters to iproute_del()
> radius: specify gateway in iproute_del()
> iputils: set scope depending on gateway in iproute_{add,del}()
> iputils: always set scope to RT_SCOPE_UNIVERSE in ip6route_{add,del}()
> iputils: remove NLM_F_CREATE flag from ip6{route,addr}_del()
> iputils: remove unnecessary NLM_F_ACK
> ipoe: stricter route deletion
>
> accel-pppd/ctrl/ipoe/ipoe.c | 10 +++++-----
> accel-pppd/libnetlink/iputils.c | 29 ++++++++++++++++-------------
> accel-pppd/libnetlink/iputils.h | 2 +-
> accel-pppd/radius/radius.c | 2 +-
> 4 files changed, 23 insertions(+), 20 deletions(-)
>
>--
>2.20.1
>
Applied, thanks
--
Dmitry Kozlov
|
Thanks for helping keep SourceForge clean.
X