Skip to content

Set of minor fixes across the board #442

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

Merged
thom311 merged 8 commits into from
Aug 7, 2025
+89 −45
Merged

Set of minor fixes across the board #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f3a2a47
[ipvti] Fix a bug in ipvti_put_attrs
cpaasch-oai Aug 5, 2025
5463545
[nexthop] Fix clone'ing of route nexthop
cpaasch-oai Aug 5, 2025
873d543
[geneve] Use NLA_PUT_U16 for the port
cpaasch-oai Aug 6, 2025
c05a7ff
[ipgre] Fix ipgre_put_attrs and dump_details
cpaasch-oai Aug 6, 2025
c20968a
[vxlan] Use proper u16 for vxi_port and fixes to vxlan_compare
cpaasch-oai Aug 6, 2025
6d7b10d
[sit/ipip/ipvti] Use correct byte-order conversion routing
cpaasch-oai Aug 6, 2025
e71bdfb
[ip6gre/ip6vti]: Fix printing of the link-name
cpaasch-oai Aug 6, 2025
75352b7
[link] Avoid leaking rtnl_link object
cpaasch-oai Aug 6, 2025
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
2 changes: 1 addition & 1 deletion lib/route/link/geneve.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ static int geneve_put_attrs(struct nl_msg *msg, struct rtnl_link *link)
NLA_PUT_U32(msg, IFLA_GENEVE_LABEL, geneve->label);

if (geneve->mask & GENEVE_ATTR_PORT)
NLA_PUT_U32(msg, IFLA_GENEVE_PORT, geneve->port);
NLA_PUT_U16(msg, IFLA_GENEVE_PORT, geneve->port);

if (geneve->mask & GENEVE_ATTR_UDP_CSUM)
NLA_PUT_U8(msg, IFLA_GENEVE_UDP_CSUM, geneve->udp_csum);
Expand Down
12 changes: 10 additions & 2 deletions lib/route/link/ip6gre.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

#define IP6GRE_ATTR_LINK (1 << 0)
#define IP6GRE_ATTR_IFLAGS (1 << 1)
Expand Down Expand Up @@ -242,12 +243,19 @@ static void ip6gre_dump_line(struct rtnl_link *link, struct nl_dump_params *p)
static void ip6gre_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
{
struct ip6gre_info *ip6gre = link->l_info;
char *name;
char addr[INET6_ADDRSTRLEN];

if (ip6gre->ip6gre_mask & IP6GRE_ATTR_LINK) {
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");
name = rtnl_link_get_name(link);

name = NULL;
parent = link_lookup(link->ce_cache, ip6gre->link);
Copy link
Owner

@thom311 thom311 Aug 6, 2025

Choose a reason for hiding this comment

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

I also think that link_lookup() returns a reference. So you would need _nl_auto_rtnl_link.

On the other hand, link_lookup() is internal API only (it can change). This API shouldn't return a reference/ownership, that would only make sense if cache and nl_object_get() were thread-safe and could be filled at the same time by other threads. But it is not, so nobody is going to unref the returned object unexpectedly. The callers just need to take care that they do nothing with the looked up link, after it might be destroyed (or take a reference here).

I would even go a step further and add a link_lookup_name() helper (that returns a const char *) and use that.

Copy link
Contributor Author

@cpaasch-oai cpaasch-oai Aug 6, 2025

Choose a reason for hiding this comment

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

Yes, we are leaking objects. 75352b7 intended to fix that.

If you rather prefer to change link_lookup to not take the ref and/or add link_lookup_name, I can do that !

if (parent)
name = rtnl_link_get_name(parent);

if (name)
nl_dump_line(p, "%s\n", name);
else
Expand Down
7 changes: 5 additions & 2 deletions lib/route/link/ip6tnl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

#define IP6_TNL_ATTR_LINK (1 << 0)
#define IP6_TNL_ATTR_LOCAL (1 << 1)
Expand Down Expand Up @@ -237,10 +238,12 @@ static void ip6_tnl_dump_details(struct rtnl_link *link,
struct nl_dump_params *p)
{
struct ip6_tnl_info *ip6_tnl = link->l_info;
char *name, addr[INET6_ADDRSTRLEN];
struct rtnl_link *parent;
char addr[INET6_ADDRSTRLEN];

if (ip6_tnl->ip6_tnl_mask & IP6_TNL_ATTR_LINK) {
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");

name = NULL;
Expand Down
12 changes: 10 additions & 2 deletions lib/route/link/ip6vti.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

#define IP6VTI_ATTR_LINK (1 << 0)
#define IP6VTI_ATTR_IKEY (1 << 1)
Expand Down Expand Up @@ -176,12 +177,19 @@ static void ip6vti_dump_line(struct rtnl_link *link, struct nl_dump_params *p)
static void ip6vti_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
{
struct ip6vti_info *ip6vti = link->l_info;
char *name;
char addr[INET6_ADDRSTRLEN];

if (ip6vti->ip6vti_mask & IP6VTI_ATTR_LINK) {
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");
name = rtnl_link_get_name(link);

name = NULL;
parent = link_lookup(link->ce_cache, ip6vti->link);
if (parent)
name = rtnl_link_get_name(parent);

if (name)
nl_dump_line(p, "%s\n", name);
else
Expand Down
21 changes: 12 additions & 9 deletions lib/route/link/ipgre.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

#define IPGRE_ATTR_LINK (1 << 0)
#define IPGRE_ATTR_IFLAGS (1 << 1)
Expand Down Expand Up @@ -181,10 +182,10 @@ static int ipgre_put_attrs(struct nl_msg *msg, struct rtnl_link *link)
if (ipgre->ipgre_mask & IPGRE_ATTR_LINK)
NLA_PUT_U32(msg, IFLA_GRE_LINK, ipgre->link);

if (ipgre->ipgre_mask & IFLA_GRE_IFLAGS)
if (ipgre->ipgre_mask & IPGRE_ATTR_IFLAGS)
NLA_PUT_U16(msg, IFLA_GRE_IFLAGS, ipgre->iflags);

if (ipgre->ipgre_mask & IFLA_GRE_OFLAGS)
if (ipgre->ipgre_mask & IPGRE_ATTR_OFLAGS)
NLA_PUT_U16(msg, IFLA_GRE_OFLAGS, ipgre->oflags);

if (ipgre->ipgre_mask & IPGRE_ATTR_IKEY)
Expand Down Expand Up @@ -234,10 +235,12 @@ static void ipgre_dump_line(struct rtnl_link *link, struct nl_dump_params *p)
static void ipgre_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
{
struct ipgre_info *ipgre = link->l_info;
char *name, addr[INET_ADDRSTRLEN];
struct rtnl_link *parent;
char addr[INET_ADDRSTRLEN];

if (ipgre->ipgre_mask & IPGRE_ATTR_LINK) {
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");

name = NULL;
Expand All @@ -263,7 +266,7 @@ static void ipgre_dump_details(struct rtnl_link *link, struct nl_dump_params *p)

if (ipgre->ipgre_mask & IPGRE_ATTR_IKEY) {
nl_dump(p, " ikey ");
nl_dump_line(p, "%x\n",ipgre->ikey);
nl_dump_line(p, "%x\n", ipgre->ikey);
}

if (ipgre->ipgre_mask & IPGRE_ATTR_OKEY) {
Expand All @@ -273,18 +276,18 @@ static void ipgre_dump_details(struct rtnl_link *link, struct nl_dump_params *p)

if (ipgre->ipgre_mask & IPGRE_ATTR_LOCAL) {
nl_dump(p, " local ");
if(inet_ntop(AF_INET, &ipgre->local, addr, sizeof(addr)))
if (inet_ntop(AF_INET, &ipgre->local, addr, sizeof(addr)))
nl_dump_line(p, "%s\n", addr);
else
nl_dump_line(p, "%#x\n", ntohs(ipgre->local));
nl_dump_line(p, "%#x\n", ntohl(ipgre->local));
}

if (ipgre->ipgre_mask & IPGRE_ATTR_REMOTE) {
nl_dump(p, " remote ");
if(inet_ntop(AF_INET, &ipgre->remote, addr, sizeof(addr)))
if (inet_ntop(AF_INET, &ipgre->remote, addr, sizeof(addr)))
nl_dump_line(p, "%s\n", addr);
else
nl_dump_line(p, "%#x\n", ntohs(ipgre->remote));
nl_dump_line(p, "%#x\n", ntohl(ipgre->remote));
}

if (ipgre->ipgre_mask & IPGRE_ATTR_TTL) {
Expand Down
17 changes: 11 additions & 6 deletions lib/route/link/ipip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

#define IPIP_ATTR_LINK (1 << 0)
#define IPIP_ATTR_LOCAL (1 << 1)
Expand Down Expand Up @@ -189,10 +190,12 @@ static void ipip_dump_line(struct rtnl_link *link, struct nl_dump_params *p)
static void ipip_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
{
struct ipip_info *ipip = link->l_info;
char *name, addr[INET_ADDRSTRLEN];
struct rtnl_link *parent;
char addr[INET_ADDRSTRLEN];

if (ipip->ipip_mask & IPIP_ATTR_LINK) {
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");

name = NULL;
Expand All @@ -208,18 +211,20 @@ static void ipip_dump_details(struct rtnl_link *link, struct nl_dump_params *p)

if (ipip->ipip_mask & IPIP_ATTR_LOCAL) {
nl_dump(p, " local ");
if(inet_ntop(AF_INET, &ipip->local, addr, sizeof(addr)))

if (inet_ntop(AF_INET, &ipip->local, addr, sizeof(addr)))
Copy link
Owner

@thom311 thom311 Aug 6, 2025

Choose a reason for hiding this comment

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

I think checking the return value for inet_ntop() is wrong. This function can only fail, if you pass invalid address/family or buffer size, which the caller should just not do. There is _nl_inet_ntop() for that reason.

I mean, if libc is not able to convert an u32 / struct in6_addr to a string buffer (of sufficient size), then we should reimplement it. But of course, libc is not going to fail here. Ever.

And the else path is just unreached code and noise and place for bugs.

But the patch is good. Maybe I will drop some inet_ntop() later -- unless you want to do it first :) Thank you.

Copy link
Contributor Author

@cpaasch-oai cpaasch-oai Aug 6, 2025

Choose a reason for hiding this comment

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

I see! I just blindly fixed whitespace here without actually reading what the purpose of inet_ntop here is 😅

nl_dump_line(p, "%s\n", addr);
else
nl_dump_line(p, "%#x\n", ntohs(ipip->local));
nl_dump_line(p, "%#x\n", ntohl(ipip->local));
}

if (ipip->ipip_mask & IPIP_ATTR_REMOTE) {
nl_dump(p, " remote ");
if(inet_ntop(AF_INET, &ipip->remote, addr, sizeof(addr)))

if (inet_ntop(AF_INET, &ipip->remote, addr, sizeof(addr)))
nl_dump_line(p, "%s\n", addr);
else
nl_dump_line(p, "%#x\n", ntohs(ipip->remote));
nl_dump_line(p, "%#x\n", ntohl(ipip->remote));
}

if (ipip->ipip_mask & IPIP_ATTR_TTL) {
Expand Down
21 changes: 13 additions & 8 deletions lib/route/link/ipvti.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

#define IPVTI_ATTR_LINK (1 << 0)
#define IPVTI_ATTR_IKEY (1 << 1)
Expand Down Expand Up @@ -144,7 +145,7 @@ static int ipvti_put_attrs(struct nl_msg *msg, struct rtnl_link *link)
if (ipvti->ipvti_mask & IPVTI_ATTR_IKEY)
NLA_PUT_U32(msg, IFLA_VTI_IKEY, ipvti->ikey);

if (ipvti->ipvti_mask & IFLA_VTI_IKEY)
if (ipvti->ipvti_mask & IPVTI_ATTR_OKEY)
NLA_PUT_U32(msg, IFLA_VTI_OKEY, ipvti->okey);

if (ipvti->ipvti_mask & IPVTI_ATTR_LOCAL)
Expand Down Expand Up @@ -179,10 +180,12 @@ static void ipvti_dump_line(struct rtnl_link *link, struct nl_dump_params *p)
static void ipvti_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
{
struct ipvti_info *ipvti = link->l_info;
char *name, addr[INET_ADDRSTRLEN];
struct rtnl_link *parent;
char addr[INET_ADDRSTRLEN];

if (ipvti->ipvti_mask & IPVTI_ATTR_LINK) {
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");

name = NULL;
Expand All @@ -208,18 +211,20 @@ static void ipvti_dump_details(struct rtnl_link *link, struct nl_dump_params *p)

if (ipvti->ipvti_mask & IPVTI_ATTR_LOCAL) {
nl_dump(p, " local ");
if(inet_ntop(AF_INET, &ipvti->local, addr, sizeof(addr)))

if (inet_ntop(AF_INET, &ipvti->local, addr, sizeof(addr)))
nl_dump_line(p, "%s\n", addr);
else
nl_dump_line(p, "%#x\n", ntohs(ipvti->local));
nl_dump_line(p, "%#x\n", ntohl(ipvti->local));
}

if (ipvti->ipvti_mask & IPVTI_ATTR_REMOTE) {
nl_dump(p, " remote ");
if(inet_ntop(AF_INET, &ipvti->remote, addr, sizeof(addr)))

if (inet_ntop(AF_INET, &ipvti->remote, addr, sizeof(addr)))
nl_dump_line(p, "%s\n", addr);
else
nl_dump_line(p, "%#x\n", ntohs(ipvti->remote));
nl_dump_line(p, "%#x\n", ntohl(ipvti->remote));
}

if (ipvti->ipvti_mask & IPVTI_ATTR_FWMARK) {
Expand Down Expand Up @@ -264,7 +269,7 @@ static struct rtnl_link_info_ops ipvti_info_ops = {

#define IS_IPVTI_LINK_ASSERT(link) \
if ((link)->l_info_ops != &ipvti_info_ops) { \
APPBUG("Link is not a ipvti link. set type \vti\" first."); \
APPBUG("Link is not a ipvti link. set type \"vti\" first.");\
return -NLE_OPNOTSUPP; \
}

Expand Down
17 changes: 11 additions & 6 deletions lib/route/link/sit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

#define SIT_ATTR_LINK (1 << 0)
#define SIT_ATTR_LOCAL (1 << 1)
Expand Down Expand Up @@ -257,10 +258,12 @@ static void sit_dump_line(struct rtnl_link *link, struct nl_dump_params *p)
static void sit_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
{
struct sit_info *sit = link->l_info;
char *name, addr[INET_ADDRSTRLEN], addr6[INET6_ADDRSTRLEN];
struct rtnl_link *parent;
char addr[INET_ADDRSTRLEN], addr6[INET6_ADDRSTRLEN];

if (sit->sit_mask & SIT_ATTR_LINK) {
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");

name = NULL;
Expand All @@ -276,18 +279,20 @@ static void sit_dump_details(struct rtnl_link *link, struct nl_dump_params *p)

if (sit->sit_mask & SIT_ATTR_LOCAL) {
nl_dump(p, " local ");
if(inet_ntop(AF_INET, &sit->local, addr, sizeof(addr)))

if (inet_ntop(AF_INET, &sit->local, addr, sizeof(addr)))
nl_dump_line(p, "%s\n", addr);
else
nl_dump_line(p, "%#x\n", ntohs(sit->local));
nl_dump_line(p, "%#x\n", ntohl(sit->local));
}

if (sit->sit_mask & SIT_ATTR_REMOTE) {
nl_dump(p, " remote ");
if(inet_ntop(AF_INET, &sit->remote, addr, sizeof(addr)))

if (inet_ntop(AF_INET, &sit->remote, addr, sizeof(addr)))
nl_dump_line(p, "%s\n", addr);
else
nl_dump_line(p, "%#x\n", ntohs(sit->remote));
nl_dump_line(p, "%#x\n", ntohl(sit->remote));
}

if (sit->sit_mask & SIT_ATTR_TTL) {
Expand Down
21 changes: 13 additions & 8 deletions lib/route/link/vxlan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

/** @cond SKIP */
#define VXLAN_ATTR_ID (1<<0)
Expand Down Expand Up @@ -312,8 +313,7 @@ static void vxlan_dump_line(struct rtnl_link *link, struct nl_dump_params *p)
static void vxlan_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
{
struct vxlan_info *vxi = link->l_info;
char *name, addr[INET6_ADDRSTRLEN];
struct rtnl_link *parent;
char addr[INET6_ADDRSTRLEN];

nl_dump_line(p, " vxlan-id %u\n", vxi->vxi_id);

Expand All @@ -328,6 +328,9 @@ static void vxlan_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
}

if (vxi->ce_mask & VXLAN_ATTR_LINK) {
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");

name = NULL;
Expand Down Expand Up @@ -572,7 +575,7 @@ static int vxlan_put_attrs(struct nl_msg *msg, struct rtnl_link *link)
NLA_PUT_U8(msg, IFLA_VXLAN_L3MISS, vxi->vxi_l3miss);

if (vxi->ce_mask & VXLAN_ATTR_PORT)
NLA_PUT_U32(msg, IFLA_VXLAN_PORT, vxi->vxi_port);
NLA_PUT_U16(msg, IFLA_VXLAN_PORT, vxi->vxi_port);

if (vxi->ce_mask & VXLAN_ATTR_UDP_CSUM)
NLA_PUT_U8(msg, IFLA_VXLAN_UDP_CSUM, vxi->vxi_udp_csum);
Expand Down Expand Up @@ -643,13 +646,15 @@ static int vxlan_compare(struct rtnl_link *link_a, struct rtnl_link *link_b,
sizeof(a->vxi_group6)) != 0);
diff |= _DIFF(VXLAN_ATTR_LOCAL6, memcmp(&a->vxi_local6, &b->vxi_local6,
sizeof(a->vxi_local6)) != 0);
diff |= _DIFF(VXLAN_ATTR_UDP_CSUM, a->vxi_proxy != b->vxi_proxy);
diff |= _DIFF(VXLAN_ATTR_UDP_CSUM, a->vxi_udp_csum != b->vxi_udp_csum);
diff |= _DIFF(VXLAN_ATTR_UDP_ZERO_CSUM6_TX,
a->vxi_proxy != b->vxi_proxy);
a->vxi_udp_zero_csum6_tx != b->vxi_udp_zero_csum6_tx);
diff |= _DIFF(VXLAN_ATTR_UDP_ZERO_CSUM6_RX,
a->vxi_proxy != b->vxi_proxy);
diff |= _DIFF(VXLAN_ATTR_REMCSUM_TX, a->vxi_proxy != b->vxi_proxy);
diff |= _DIFF(VXLAN_ATTR_REMCSUM_RX, a->vxi_proxy != b->vxi_proxy);
a->vxi_udp_zero_csum6_rx != b->vxi_udp_zero_csum6_rx);
diff |= _DIFF(VXLAN_ATTR_REMCSUM_TX,
a->vxi_remcsum_tx != b->vxi_remcsum_tx);
diff |= _DIFF(VXLAN_ATTR_REMCSUM_RX,
a->vxi_remcsum_rx != b->vxi_remcsum_rx);
diff |= _DIFF(VXLAN_ATTR_COLLECT_METADATA,
a->vxi_collect_metadata != b->vxi_collect_metadata);
diff |= _DIFF(VXLAN_ATTR_LABEL, a->vxi_label != b->vxi_label);
Expand Down
3 changes: 2 additions & 1 deletion lib/route/link/xfrmi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "nl-route.h"
#include "link-api.h"
#include "nl-aux-route/nl-route.h"

#define XFRMI_ATTR_LINK (1 << 0)
#define XFRMI_ATTR_IF_ID (1 << 1)
Expand Down Expand Up @@ -134,7 +135,7 @@ static void xfrmi_dump_details(struct rtnl_link *link, struct nl_dump_params *p)
struct xfrmi_info *xfrmi = link->l_info;

if (xfrmi->xfrmi_mask & XFRMI_ATTR_LINK) {
struct rtnl_link *parent;
_nl_auto_rtnl_link struct rtnl_link *parent = NULL;
char *name;

nl_dump(p, " link ");
Expand Down
Loading