| Message ID | 20210114173811.7854-1-fw@strlen.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | [PATCHi,iproute2] mptcp: add support for event monitoring | expand |
Hi Florian, On 14/01/2021 18:38, Florian Westphal wrote: > Output will look similar to this: > > [ CREATED] token=83f3a692 remid=0 locid=0 saddr4=10.0.1.2 daddr4=10.0.1.1 sport=58710 dport=10011 > [ ESTABLISHED] token=83f3a692 remid=0 locid=0 saddr4=10.0.1.2 daddr4=10.0.1.1 sport=58710 dport=10011 > [SF_ESTABLISHED] token=83f3a692 remid=0 locid=1 saddr4=10.0.2.2 daddr4=10.0.1.1 sport=40195 dport=10011 backup=0 > [ CLOSED] token=83f3a692 Thank you for this new feature, that's a good idea! It looks good! Just the usage/man page to update I think! I just asked a few questions below if you don't mind looking :) (...) > diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h > index 321812307718..199b0c46ba70 100644 > --- a/include/uapi/linux/mptcp.h > +++ b/include/uapi/linux/mptcp.h > @@ -36,6 +36,7 @@ enum { > /* netlink interface */ > #define MPTCP_PM_NAME "mptcp_pm" > #define MPTCP_PM_CMD_GRP_NAME "mptcp_pm_cmds" > +#define MPTCP_PM_EV_GRP_NAME "mptcp_events" (just to be sure: we don't want to prefix with "_pm" to keep compatibility with mptcp.org kernel, right?) > #define MPTCP_PM_VER 0x1 I guess we don't need to change that, on older version of the kernel we can just check if "mptcp_events" has been added. (...) > diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c > index e1ffafb3c658..4716aa9e62e8 100644 > --- a/ip/ipmptcp.c > +++ b/ip/ipmptcp.c > @@ -385,6 +385,104 @@ static int mptcp_limit_get_set(int argc, char **argv, int cmd) > return 0; > } (...) > +static void print_addr(const char *key, int af, struct rtattr *value) > +{ > + void *data = RTA_DATA(value); > + char str[INET6_ADDRSTRLEN]; > + > + if (inet_ntop(af, data, str, sizeof(str))) > + printf(" %s=%s", key, str); Should we display an error in case of issue to parse the address? Or no need because it should not happen? > +} > + > +static int mptcp_monitor_msg(struct rtnl_ctrl_data *ctrl, > + struct nlmsghdr *n, void *arg) > +{ > + const struct genlmsghdr *ghdr = NLMSG_DATA(n); > + struct rtattr *tb[MPTCP_ATTR_MAX + 1]; > + int len = n->nlmsg_len; > + > + len -= NLMSG_LENGTH(GENL_HDRLEN); > + if (len < 0) > + return -1; > + > + if (n->nlmsg_type != genl_family) > + return 0; > + > + if (ghdr->cmd >= ARRAY_SIZE(event_to_str)) { > + printf("[UNKNOWN %u]\n", ghdr->cmd); > + fflush(stdout); > + return 0; (detail: maybe good to have a goto here and below to do the fflush + return (+puts?) mainly for later not to forget that? Up to you, a detail) (...) > + printf(" token=%08x", rta_getattr_u32(tb[MPTCP_ATTR_TOKEN])); If we are not the user ID 0, we cannot do the monitoring, right? > if (argc == 0) > @@ -429,6 +527,14 @@ int do_mptcp(int argc, char **argv) > MPTCP_PM_CMD_GET_LIMITS); > } > > + if (matches(*argv, "monitor") == 0) { May you update the "usage()" function here above and the man page to reflect this new parameter please? (...) > diff --git a/lib/libgenl.c b/lib/libgenl.c > index f2ce698fc711..4c51d47af46b 100644 > --- a/lib/libgenl.c > +++ b/lib/libgenl.c > @@ -67,6 +67,72 @@ int genl_resolve_family(struct rtnl_handle *grth, const char *family) > return fnum; > } > > +static int genl_parse_grps(struct rtattr *attr, const char *name, unsigned int *id) > +{ > + const struct rtattr *pos; > + > + rtattr_for_each_nested(pos, attr) { > + struct rtattr *tb[CTRL_ATTR_MCAST_GRP_MAX + 1]; > + > + parse_rtattr_nested(tb, CTRL_ATTR_MCAST_GRP_MAX, pos); > + > + if (tb[CTRL_ATTR_MCAST_GRP_NAME] && tb[CTRL_ATTR_MCAST_GRP_ID]) { > + if (strcmp(name, rta_getattr_str(tb[CTRL_ATTR_MCAST_GRP_NAME])) == 0) { Should we use strncmp()? (But not sure with which limit so probably no) Cheers, Matt
Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > Hi Florian, > > On 14/01/2021 18:38, Florian Westphal wrote: > > Output will look similar to this: > > > > [ CREATED] token=83f3a692 remid=0 locid=0 saddr4=10.0.1.2 daddr4=10.0.1.1 sport=58710 dport=10011 > > [ ESTABLISHED] token=83f3a692 remid=0 locid=0 saddr4=10.0.1.2 daddr4=10.0.1.1 sport=58710 dport=10011 > > [SF_ESTABLISHED] token=83f3a692 remid=0 locid=1 saddr4=10.0.2.2 daddr4=10.0.1.1 sport=40195 dport=10011 backup=0 > > [ CLOSED] token=83f3a692 > > Thank you for this new feature, that's a good idea! > > It looks good! Just the usage/man page to update I think! I just asked a few > questions below if you don't mind looking :) Right, will update this for the netdev submission. > > diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h > > index 321812307718..199b0c46ba70 100644 > > --- a/include/uapi/linux/mptcp.h > > +++ b/include/uapi/linux/mptcp.h > > @@ -36,6 +36,7 @@ enum { > > /* netlink interface */ > > #define MPTCP_PM_NAME "mptcp_pm" > > #define MPTCP_PM_CMD_GRP_NAME "mptcp_pm_cmds" > > +#define MPTCP_PM_EV_GRP_NAME "mptcp_events" > > (just to be sure: we don't want to prefix with "_pm" to keep compatibility > with mptcp.org kernel, right?) Only change needed on userspace side is use of "mptcp_pm" instead of plain "mptcp" for the genl name. Mandation use of "mptcp_pm_events" would mean a second change and I do not see a good reason for it. > > #define MPTCP_PM_VER 0x1 > > I guess we don't need to change that, on older version of the kernel we can > just check if "mptcp_events" has been added. On old kernel the subscription request will fail. I would only increment this in case of incompatible changes, which should be avoided anyway. > > +static void print_addr(const char *key, int af, struct rtattr *value) > > +{ > > + void *data = RTA_DATA(value); > > + char str[INET6_ADDRSTRLEN]; > > + > > + if (inet_ntop(af, data, str, sizeof(str))) > > + printf(" %s=%s", key, str); > > Should we display an error in case of issue to parse the address? Or no need > because it should not happen? I don't think it makes sense to handle errors here. The only error message we could display here would be something like "kernel netlink abi is broken". I can do that (plus abort()) in the next iteration. > > + if (ghdr->cmd >= ARRAY_SIZE(event_to_str)) { > > + printf("[UNKNOWN %u]\n", ghdr->cmd); > > + fflush(stdout); > > + return 0; > > (detail: maybe good to have a goto here and below to do the fflush + return > (+puts?) mainly for later not to forget that? Up to you, a detail) Ok, I can change it. > > + printf(" token=%08x", rta_getattr_u32(tb[MPTCP_ATTR_TOKEN])); > > If we are not the user ID 0, we cannot do the monitoring, right? No, netlink mcast subscription will fail with -EPERM in that case. > > if (argc == 0) > > @@ -429,6 +527,14 @@ int do_mptcp(int argc, char **argv) > > MPTCP_PM_CMD_GET_LIMITS); > > } > > + if (matches(*argv, "monitor") == 0) { > > May you update the "usage()" function here above and the man page to reflect > this new parameter please? Yes, sorry I forgot. > > +static int genl_parse_grps(struct rtattr *attr, const char *name, unsigned int *id) > > +{ > > + const struct rtattr *pos; > > + > > + rtattr_for_each_nested(pos, attr) { > > + struct rtattr *tb[CTRL_ATTR_MCAST_GRP_MAX + 1]; > > + > > + parse_rtattr_nested(tb, CTRL_ATTR_MCAST_GRP_MAX, pos); > > + > > + if (tb[CTRL_ATTR_MCAST_GRP_NAME] && tb[CTRL_ATTR_MCAST_GRP_ID]) { > > + if (strcmp(name, rta_getattr_str(tb[CTRL_ATTR_MCAST_GRP_NAME])) == 0) { > > Should we use strncmp()? (But not sure with which limit so probably no) No, the string is part of the abi, so it will always match "mptcp_event" in case the feature is present.
Hi Florian, On 16/01/2021 18:57, Florian Westphal wrote: > Matthieu Baerts <matthieu.baerts@tessares.net> wrote: >> On 14/01/2021 18:38, Florian Westphal wrote: >>> Output will look similar to this: >>> >>> [ CREATED] token=83f3a692 remid=0 locid=0 saddr4=10.0.1.2 daddr4=10.0.1.1 sport=58710 dport=10011 >>> [ ESTABLISHED] token=83f3a692 remid=0 locid=0 saddr4=10.0.1.2 daddr4=10.0.1.1 sport=58710 dport=10011 >>> [SF_ESTABLISHED] token=83f3a692 remid=0 locid=1 saddr4=10.0.2.2 daddr4=10.0.1.1 sport=40195 dport=10011 backup=0 >>> [ CLOSED] token=83f3a692 >> >> Thank you for this new feature, that's a good idea! >> >> It looks good! Just the usage/man page to update I think! I just asked a few >> questions below if you don't mind looking :) > > Right, will update this for the netdev submission. Thanks! And thank you for the other answers! Cheers, Matt
diff --git a/include/libgenl.h b/include/libgenl.h index 656493a2c3c4..97281cc1103f 100644 --- a/include/libgenl.h +++ b/include/libgenl.h @@ -21,6 +21,7 @@ struct { \ }, \ } +int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 genl_family, const char *group); int genl_resolve_family(struct rtnl_handle *grth, const char *family); int genl_init_handle(struct rtnl_handle *grth, const char *family, int *genl_family); diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h index 321812307718..199b0c46ba70 100644 --- a/include/uapi/linux/mptcp.h +++ b/include/uapi/linux/mptcp.h @@ -36,6 +36,7 @@ enum { /* netlink interface */ #define MPTCP_PM_NAME "mptcp_pm" #define MPTCP_PM_CMD_GRP_NAME "mptcp_pm_cmds" +#define MPTCP_PM_EV_GRP_NAME "mptcp_events" #define MPTCP_PM_VER 0x1 /* @@ -86,6 +87,81 @@ enum { __MPTCP_PM_CMD_AFTER_LAST }; +/* + * Events generated by MPTCP: + * - MPTCP_EVENT_CREATED: token, family, saddr4 | saddr6, daddr4 | daddr6, + * sport, dport + * A new connection has been created. It is the good time to allocate + * memory and send ADD_ADDR if needed. Depending on the traffic-patterns + * it can take a long time until the MPTCP_EVENT_ESTABLISHED is sent. + * + * - MPTCP_EVENT_ESTABLISHED: token, family, saddr4 | saddr6, daddr4 | daddr6, + * sport, dport + * A connection is established (can start new subflows). + * + * - MPTCP_EVENT_CLOSED: token + * A connection has stopped. + * + * - MPTCP_EVENT_ANNOUNCED: token, rem_id, family, daddr4 | daddr6 [, dport] + * A new address has been announced by the peer. + * + * - MPTCP_EVENT_REMOVED: token, rem_id + * An address has been lost by the peer. + * + * - MPTCP_EVENT_SUB_ESTABLISHED: token, family, saddr4 | saddr6, + * daddr4 | daddr6, sport, dport, backup, + * if_idx [, error] + * A new subflow has been established. 'error' should not be set. + * + * - MPTCP_EVENT_SUB_CLOSED: token, family, saddr4 | saddr6, daddr4 | daddr6, + * sport, dport, backup, if_idx [, error] + * A subflow has been closed. An error (copy of sk_err) could be set if an + * error has been detected for this subflow. + * + * - MPTCP_EVENT_SUB_PRIORITY: token, family, saddr4 | saddr6, daddr4 | daddr6, + * sport, dport, backup, if_idx [, error] + * The priority of a subflow has changed. 'error' should not be set. +*/ +enum mptcp_event { + MPTCP_EVENT_UNSPEC = 0, + + MPTCP_EVENT_CREATED = 1, + MPTCP_EVENT_ESTABLISHED, + MPTCP_EVENT_CLOSED, + + MPTCP_EVENT_ANNOUNCED = 6, + MPTCP_EVENT_REMOVED, + + MPTCP_EVENT_SUB_ESTABLISHED = 10, + MPTCP_EVENT_SUB_CLOSED, + + MPTCP_EVENT_SUB_PRIORITY = 13, + + __MPTCP_EVENT_AFTER_LAST = 16, +}; + +enum mptcp_event_attr { + MPTCP_ATTR_UNSPEC = 0, + + MPTCP_ATTR_TOKEN, /* u32 */ + MPTCP_ATTR_FAMILY, /* u16 */ + MPTCP_ATTR_LOC_ID, /* u8 */ + MPTCP_ATTR_REM_ID, /* u8 */ + MPTCP_ATTR_SADDR4, /* be32 */ + MPTCP_ATTR_SADDR6, /* struct in6_addr */ + MPTCP_ATTR_DADDR4, /* be32 */ + MPTCP_ATTR_DADDR6, /* struct in6_addr */ + MPTCP_ATTR_SPORT, /* be16 */ + MPTCP_ATTR_DPORT, /* be16 */ + MPTCP_ATTR_BACKUP, /* u8 */ + MPTCP_ATTR_ERROR, /* u8 */ + MPTCP_ATTR_FLAGS, /* u16 */ + MPTCP_ATTR_TIMEOUT, /* u32 */ + MPTCP_ATTR_IF_IDX, /* s32 */ + __MPTCP_ATTR_AFTER_LAST +}; +#define MPTCP_ATTR_MAX (__MPTCP_ATTR_AFTER_LAST - 1) + #define MPTCP_INFO_FLAG_FALLBACK _BITUL(0) #define MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED _BITUL(1) diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c index e1ffafb3c658..4716aa9e62e8 100644 --- a/ip/ipmptcp.c +++ b/ip/ipmptcp.c @@ -385,6 +385,104 @@ static int mptcp_limit_get_set(int argc, char **argv, int cmd) return 0; } +static const char * const event_to_str[] = { + [MPTCP_EVENT_CREATED] = "CREATED", + [MPTCP_EVENT_ESTABLISHED] = "ESTABLISHED", + [MPTCP_EVENT_CLOSED] = "CLOSED", + [MPTCP_EVENT_ANNOUNCED] = "ANNOUNCED", + [MPTCP_EVENT_REMOVED] = "REMOVED", + [MPTCP_EVENT_SUB_ESTABLISHED] = "SF_ESTABLISHED", + [MPTCP_EVENT_SUB_CLOSED] = "SF_CLOSED", + [MPTCP_EVENT_SUB_PRIORITY] = "SF_PRIO", +}; + +static void print_addr(const char *key, int af, struct rtattr *value) +{ + void *data = RTA_DATA(value); + char str[INET6_ADDRSTRLEN]; + + if (inet_ntop(af, data, str, sizeof(str))) + printf(" %s=%s", key, str); +} + +static int mptcp_monitor_msg(struct rtnl_ctrl_data *ctrl, + struct nlmsghdr *n, void *arg) +{ + const struct genlmsghdr *ghdr = NLMSG_DATA(n); + struct rtattr *tb[MPTCP_ATTR_MAX + 1]; + int len = n->nlmsg_len; + + len -= NLMSG_LENGTH(GENL_HDRLEN); + if (len < 0) + return -1; + + if (n->nlmsg_type != genl_family) + return 0; + + if (ghdr->cmd >= ARRAY_SIZE(event_to_str)) { + printf("[UNKNOWN %u]\n", ghdr->cmd); + fflush(stdout); + return 0; + } + + if (event_to_str[ghdr->cmd] == NULL) { + printf("[UNKNOWN %u]\n", ghdr->cmd); + fflush(stdout); + return 0; + } + + printf("[%14s]", event_to_str[ghdr->cmd]); + + parse_rtattr(tb, MPTCP_ATTR_MAX, (void *) ghdr + GENL_HDRLEN, len); + + printf(" token=%08x", rta_getattr_u32(tb[MPTCP_ATTR_TOKEN])); + + if (tb[MPTCP_ATTR_REM_ID]) + printf(" remid=%u", rta_getattr_u16(tb[MPTCP_ATTR_REM_ID])); + if (tb[MPTCP_ATTR_LOC_ID]) + printf(" locid=%u", rta_getattr_u16(tb[MPTCP_ATTR_LOC_ID])); + + if (tb[MPTCP_ATTR_SADDR4]) + print_addr("saddr4", AF_INET, tb[MPTCP_ATTR_SADDR4]); + if (tb[MPTCP_ATTR_DADDR4]) + print_addr("daddr4", AF_INET, tb[MPTCP_ATTR_DADDR4]); + if (tb[MPTCP_ATTR_SADDR6]) + print_addr("saddr6", AF_INET6, tb[MPTCP_ATTR_SADDR6]); + if (tb[MPTCP_ATTR_DADDR6]) + print_addr("daddr6", AF_INET6, tb[MPTCP_ATTR_DADDR6]); + if (tb[MPTCP_ATTR_SPORT]) + printf(" sport=%u", rta_getattr_be16(tb[MPTCP_ATTR_SPORT])); + if (tb[MPTCP_ATTR_DPORT]) + printf(" dport=%u", rta_getattr_be16(tb[MPTCP_ATTR_DPORT])); + if (tb[MPTCP_ATTR_BACKUP]) + printf(" backup=%d", rta_getattr_u8(tb[MPTCP_ATTR_BACKUP])); + if (tb[MPTCP_ATTR_ERROR]) + printf(" error=%d", rta_getattr_u8(tb[MPTCP_ATTR_ERROR])); + if (tb[MPTCP_ATTR_FLAGS]) + printf(" flags=%x", rta_getattr_u16(tb[MPTCP_ATTR_FLAGS])); + if (tb[MPTCP_ATTR_TIMEOUT]) + printf(" timeout=%u", rta_getattr_u32(tb[MPTCP_ATTR_TIMEOUT])); + if (tb[MPTCP_ATTR_IF_IDX]) + printf(" ifindex=%d", rta_getattr_s32(tb[MPTCP_ATTR_IF_IDX])); + + puts(""); + fflush(stdout); + return 0; +} + +static int mptcp_monitor(void) +{ + if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) { + perror("can't subscribe to mptcp events"); + return 1; + } + + if (rtnl_listen(&genl_rth, mptcp_monitor_msg, stdout) < 0) + return 2; + + return 0; +} + int do_mptcp(int argc, char **argv) { if (argc == 0) @@ -429,6 +527,14 @@ int do_mptcp(int argc, char **argv) MPTCP_PM_CMD_GET_LIMITS); } + if (matches(*argv, "monitor") == 0) { + NEXT_ARG_FWD(); + if (argc == 0) + return mptcp_monitor(); + + goto unknown; + } + unknown: fprintf(stderr, "Command \"%s\" is unknown, try \"ip mptcp help\".\n", *argv); diff --git a/lib/libgenl.c b/lib/libgenl.c index f2ce698fc711..4c51d47af46b 100644 --- a/lib/libgenl.c +++ b/lib/libgenl.c @@ -67,6 +67,72 @@ int genl_resolve_family(struct rtnl_handle *grth, const char *family) return fnum; } +static int genl_parse_grps(struct rtattr *attr, const char *name, unsigned int *id) +{ + const struct rtattr *pos; + + rtattr_for_each_nested(pos, attr) { + struct rtattr *tb[CTRL_ATTR_MCAST_GRP_MAX + 1]; + + parse_rtattr_nested(tb, CTRL_ATTR_MCAST_GRP_MAX, pos); + + if (tb[CTRL_ATTR_MCAST_GRP_NAME] && tb[CTRL_ATTR_MCAST_GRP_ID]) { + if (strcmp(name, rta_getattr_str(tb[CTRL_ATTR_MCAST_GRP_NAME])) == 0) { + *id = rta_getattr_u32(tb[CTRL_ATTR_MCAST_GRP_ID]); + return 0; + } + } + } + + return -1; +} + +int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group) +{ + GENL_REQUEST(req, 1024, GENL_ID_CTRL, 0, 0, CTRL_CMD_GETFAMILY, + NLM_F_REQUEST); + struct rtattr *tb[CTRL_ATTR_MAX + 1]; + struct nlmsghdr *answer = NULL; + struct genlmsghdr *ghdr; + struct rtattr *attrs; + int len, ret = -1; + unsigned int id; + + addattr16(&req.n, sizeof(req), CTRL_ATTR_FAMILY_ID, fnum); + + if (rtnl_talk(grth, &req.n, &answer) < 0) { + fprintf(stderr, "Error talking to the kernel\n"); + return -2; + } + + ghdr = NLMSG_DATA(answer); + len = answer->nlmsg_len; + + if (answer->nlmsg_type != GENL_ID_CTRL) + goto err_free; + + len -= NLMSG_LENGTH(GENL_HDRLEN); + if (len < 0) + goto err_free; + + attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN); + parse_rtattr(tb, CTRL_ATTR_MAX, attrs, len); + + if (tb[CTRL_ATTR_MCAST_GROUPS] == NULL) { + fprintf(stderr, "Missing mcast groups TLV\n"); + goto err_free; + } + + if (genl_parse_grps(tb[CTRL_ATTR_MCAST_GROUPS], group, &id) < 0) + goto err_free; + + ret = rtnl_add_nl_group(grth, id); + +err_free: + free(answer); + return ret; +} + int genl_init_handle(struct rtnl_handle *grth, const char *family, int *genl_family) {
Output will look similar to this: [ CREATED] token=83f3a692 remid=0 locid=0 saddr4=10.0.1.2 daddr4=10.0.1.1 sport=58710 dport=10011 [ ESTABLISHED] token=83f3a692 remid=0 locid=0 saddr4=10.0.1.2 daddr4=10.0.1.1 sport=58710 dport=10011 [SF_ESTABLISHED] token=83f3a692 remid=0 locid=1 saddr4=10.0.2.2 daddr4=10.0.1.1 sport=40195 dport=10011 backup=0 [ CLOSED] token=83f3a692 Signed-off-by: Florian Westphal <fw@strlen.de> --- include/libgenl.h | 1 + include/uapi/linux/mptcp.h | 76 ++++++++++++++++++++++++++ ip/ipmptcp.c | 106 +++++++++++++++++++++++++++++++++++++ lib/libgenl.c | 66 +++++++++++++++++++++++ 4 files changed, 249 insertions(+)