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

Jump to bottom

libnl3 updates for MPLS #7195

Merged
merged 1 commit into from
Jun 16, 2021
Merged

libnl3 updates for MPLS #7195

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
3 changes: 2 additions & 1 deletion files/build_templates/init_cfg.json.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
"polling_interval": "300",
{%- for crm_res in ["ipv4_route", "ipv6_route", "ipv4_nexthop", "ipv6_nexthop", "ipv4_neighbor",
"ipv6_neighbor", "nexthop_group_member", "nexthop_group", "acl_table",
"acl_group", "acl_entry", "acl_counter", "fdb_entry", "snat_entry", "dnat_entry", "ipmc_entry"] %}
"acl_group", "acl_entry", "acl_counter", "fdb_entry", "snat_entry", "dnat_entry",
"ipmc_entry", "mpls_inseg", "mpls_nexthop"] %}
"{{crm_res}}_threshold_type": "percentage",
"{{crm_res}}_low_threshold": "70",
"{{crm_res}}_high_threshold": "85"{% if not loop.last %},{% endif -%}
Expand Down
1 change: 1 addition & 0 deletions src/libnl3/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
!debian/
debian/libnl-*/
!Makefile
!patch
4 changes: 4 additions & 0 deletions src/libnl3/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% :
pushd libnl3-$(LIBNL3_VERSION_BASE)
git checkout tags/libnl$(subst .,_,$(LIBNL3_VERSION_BASE))

git checkout -b sonic
stg init
stg import -s ../patch/series

ln -s ../debian debian
dpkg-buildpackage -rfakeroot -b -us -uc -j$(SONIC_CONFIG_MAKE_JOBS) --admindir $(SONIC_DPKG_ADMINDIR)
popd
Expand Down
77 changes: 77 additions & 0 deletions src/libnl3/patch/0001-mpls-encap-accessors.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
From 701122c539d6da3fbd3367be6e62141328ebeb07 Mon Sep 17 00:00:00 2001
From: Ann Pokora <apokora@juniper.net>
Date: Tue, 25 May 2021 18:07:37 -0700
Subject: [PATCH 1/2] mpls encap accessors

Signed-off-by: Ann Pokora <apokora@juniper.net>
---
include/netlink/route/nexthop.h | 2 ++
lib/route/nh_encap_mpls.c | 28 ++++++++++++++++++++++++++++
libnl-route-3.sym | 2 ++
3 files changed, 32 insertions(+)

diff --git a/include/netlink/route/nexthop.h b/include/netlink/route/nexthop.h
index 5b422dd..a502005 100644
--- a/include/netlink/route/nexthop.h
+++ b/include/netlink/route/nexthop.h
@@ -70,6 +70,8 @@ extern int rtnl_route_nh_str2flags(const char *);
extern int rtnl_route_nh_encap_mpls(struct rtnl_nexthop *nh,
struct nl_addr *addr,
uint8_t ttl);
+extern struct nl_addr * rtnl_route_nh_get_encap_mpls_dst(struct rtnl_nexthop *);
+extern uint8_t rtnl_route_nh_get_encap_mpls_ttl(struct rtnl_nexthop *);
#ifdef __cplusplus
}
#endif
diff --git a/lib/route/nh_encap_mpls.c b/lib/route/nh_encap_mpls.c
index 081661e..18336ac 100644
--- a/lib/route/nh_encap_mpls.c
+++ b/lib/route/nh_encap_mpls.c
@@ -133,3 +133,31 @@ int rtnl_route_nh_encap_mpls(struct rtnl_nexthop *nh,

return 0;
}
+
+struct nl_addr *rtnl_route_nh_get_encap_mpls_dst(struct rtnl_nexthop *nh)
+{
+ struct mpls_iptunnel_encap *mpls_encap;
+
+ if (!nh->rtnh_encap || nh->rtnh_encap->ops->encap_type != LWTUNNEL_ENCAP_MPLS)
+ return NULL;
+
+ mpls_encap = (struct mpls_iptunnel_encap *)nh->rtnh_encap->priv;
+ if (!mpls_encap)
+ return NULL;
+
+ return mpls_encap->dst;
+}
+
+uint8_t rtnl_route_nh_get_encap_mpls_ttl(struct rtnl_nexthop *nh)
+{
+ struct mpls_iptunnel_encap *mpls_encap;
+
+ if (!nh->rtnh_encap || nh->rtnh_encap->ops->encap_type != LWTUNNEL_ENCAP_MPLS)
+ return 0;
+
+ mpls_encap = (struct mpls_iptunnel_encap *)nh->rtnh_encap->priv;
+ if (!mpls_encap)
+ return 0;
+
+ return mpls_encap->ttl;
+}
diff --git a/libnl-route-3.sym b/libnl-route-3.sym
index 4a65503..ce6d714 100644
--- a/libnl-route-3.sym
+++ b/libnl-route-3.sym
@@ -1127,6 +1127,8 @@ global:
rtnl_qdisc_mqprio_set_priomap;
rtnl_qdisc_mqprio_set_queue;
rtnl_qdisc_mqprio_set_shaper;
+ rtnl_route_nh_get_encap_mpls_dst;
+ rtnl_route_nh_get_encap_mpls_ttl;
rtnl_rule_get_dport;
rtnl_rule_get_ipproto;
rtnl_rule_get_protocol;
--
2.7.4

63 changes: 63 additions & 0 deletions src/libnl3/patch/0002-mpls-remove-nl_addr_valid.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
From c89d1a129f71d3d2f76e6cbadb11ef41d8941a73 Mon Sep 17 00:00:00 2001
From: Ann Pokora <apokora@juniper.net>
Date: Tue, 25 May 2021 18:10:04 -0700
Subject: [PATCH 2/2] mpls remove nl_addr_valid
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we remove the validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan nl_addr_valid() arguments are supposed to be a pointer to an ASCII string and an address family value (AF_MPLS, AF_INET, AF_INET6)... it then uses these to validate that the string format contains an address for the specified family.
The removed calls to nl_addr_valid() were instead passing in a pointer to a binary address and the length of the binary address... this is completely wrong. For most cases, the length of the binary address would not match any of the values of supported address families, so nl_addr_valid() would return true. But if the binary address was an MPLS label stack with 3 labels, then the length would match the value of AF_DECnet (12) and it attempts to parse the binary address as if it were an ASCII string... this fails and nl_addr_valid() returns false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you put the above explaination into the this patch file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan explanation is added to patch file.


The removed calls to nl_addr_valid() are passing in a pointer to a binary address
and the length of the binary address, which does not match expected arguments for
nl_addr_valid().
nl_addr_valid() expects a pointer to an ASCII string and the address family of
the string format.
The incorrect arguments cause unexpected failures and the expected arguments are
not available in the context.

Signed-off-by: Ann Pokora <apokora@juniper.net>
---
lib/route/nexthop.c | 8 --------
lib/route/nh_encap_mpls.c | 4 ----
2 files changed, 12 deletions(-)

diff --git a/lib/route/nexthop.c b/lib/route/nexthop.c
index 7a9904c..ac0095e 100644
--- a/lib/route/nexthop.c
+++ b/lib/route/nexthop.c
@@ -351,10 +351,6 @@ int rtnl_route_nh_set_newdst(struct rtnl_nexthop *nh, struct nl_addr *addr)
{
struct nl_addr *old = nh->rtnh_newdst;

- if (!nl_addr_valid(nl_addr_get_binary_addr(addr),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing the validation, can we fix the arguments.

replace nl_addr_get_len(addr) with nl_addr_get_family(addr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smahesh That still isn't valid. Look at the code for nl_addr_valid() in libnl3. It is looking for an ASCII string and validating the string format. addr does not container a string, The return from nl_addr_get_binary_addr() is not a string.
We would have to first convert addr to string. It's pointless. nl_addr_valid() is not used anywhere else in libnl3, so I do not know why someone decided to use it in these MPLS functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nl_addr_valid() is using the route_nh_set which is not mpls specific. this is the IP functions. how can this function pass now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan RTA_NEWDST is specific to MPLS. NEWDST contains the MPLS labelstack associated with the NH.

- nl_addr_get_len(addr)))
- return -NLE_INVAL;
-
if (addr) {
nh->rtnh_newdst = nl_addr_get(addr);
nh->ce_mask |= NH_ATTR_NEWDST;
@@ -378,10 +374,6 @@ int rtnl_route_nh_set_via(struct rtnl_nexthop *nh, struct nl_addr *addr)
{
struct nl_addr *old = nh->rtnh_via;

- if (!nl_addr_valid(nl_addr_get_binary_addr(addr),
- nl_addr_get_len(addr)))
- return -NLE_INVAL;
-
if (addr) {
nh->rtnh_via = nl_addr_get(addr);
nh->ce_mask |= NH_ATTR_VIA;
diff --git a/lib/route/nh_encap_mpls.c b/lib/route/nh_encap_mpls.c
index 18336ac..6c5a3c7 100644
--- a/lib/route/nh_encap_mpls.c
+++ b/lib/route/nh_encap_mpls.c
@@ -109,10 +109,6 @@ int rtnl_route_nh_encap_mpls(struct rtnl_nexthop *nh,
if (!addr)
return -NLE_INVAL;

- if (!nl_addr_valid(nl_addr_get_binary_addr(addr),
- nl_addr_get_len(addr)))
- return -NLE_INVAL;
-
rtnh_encap = calloc(1, sizeof(*rtnh_encap));
if (!rtnh_encap)
return -NLE_NOMEM;
--
2.7.4

2 changes: 2 additions & 0 deletions src/libnl3/patch/series
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
0001-mpls-encap-accessors.patch
0002-mpls-remove-nl_addr_valid.patch