* [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry
@ 2022-06-30 13:30 David Lamparter
2022-06-30 13:30 ` [PATCH 1/2] ipv6: make rtm_ipv6_policy available David Lamparter
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: David Lamparter @ 2022-06-30 13:30 UTC (permalink / raw)
To: netdev; +Cc: David Lamparter
The IPv6 multicast routing code implements RTM_GETROUTE, but only for a
dump request. Retrieving a single MFC entry is not currently possible
via netlink.
While most of the data here can also be retrieved with SIOCGETSGCNT_IN6,
the lastused / RTA_EXPIRES is not included in the ioctl result (and we
need it in FRR.)
=> Implement single-entry RTM_GETROUTE by copying and adapting the IPv4
code.
Tested against FRRouting's (work-in-progress) IPv6 PIM implementation.
Cheers,
-David
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] ipv6: make rtm_ipv6_policy available
2022-06-30 13:30 [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry David Lamparter
@ 2022-06-30 13:30 ` David Lamparter
2022-06-30 13:30 ` [PATCH 2/2] net: ip6mr: add RTM_GETROUTE netlink op David Lamparter
2022-07-01 3:27 ` [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry Jakub Kicinski
2 siblings, 0 replies; 17+ messages in thread
From: David Lamparter @ 2022-06-30 13:30 UTC (permalink / raw)
To: netdev; +Cc: David Lamparter
... so ip6mr.c can use it too (as it is in IPv4.)
Signed-off-by: David Lamparter <equinox@diac24.net>
---
1:1 analog to IPv4, where rtm_ipv4_policy is exposed for pretty exactly
the same thing. IPv6 just got away with not using this across file
boundaries so far.
---
include/net/ip6_fib.h | 1 +
net/ipv6/route.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 6268963d9599..a12fedea97de 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -483,6 +483,7 @@ void rt6_get_prefsrc(const struct rt6_info *rt, struct in6_addr *addr)
rcu_read_unlock();
}
+extern const struct nla_policy rtm_ipv6_policy[];
int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
struct fib6_config *cfg, gfp_t gfp_flags,
struct netlink_ext_ack *extack);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 1d6f75eef4bd..26c7b31abe96 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4964,7 +4964,7 @@ void rt6_mtu_change(struct net_device *dev, unsigned int mtu)
fib6_clean_all(dev_net(dev), rt6_mtu_change_route, &arg);
}
-static const struct nla_policy rtm_ipv6_policy[RTA_MAX+1] = {
+const struct nla_policy rtm_ipv6_policy[RTA_MAX+1] = {
[RTA_UNSPEC] = { .strict_start_type = RTA_DPORT + 1 },
[RTA_GATEWAY] = { .len = sizeof(struct in6_addr) },
[RTA_PREFSRC] = { .len = sizeof(struct in6_addr) },
--
2.36.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] net: ip6mr: add RTM_GETROUTE netlink op
2022-06-30 13:30 [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry David Lamparter
2022-06-30 13:30 ` [PATCH 1/2] ipv6: make rtm_ipv6_policy available David Lamparter
@ 2022-06-30 13:30 ` David Lamparter
2022-07-01 3:27 ` [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry Jakub Kicinski
2 siblings, 0 replies; 17+ messages in thread
From: David Lamparter @ 2022-06-30 13:30 UTC (permalink / raw)
To: netdev; +Cc: David Lamparter
The IPv6 multicast routing code previously implemented only the dump
variant of RTM_GETROUTE. Implement single MFC item retrieval by copying
and adapting the respective IPv4 code.
Tested against FRRouting's IPv6 PIM stack.
Signed-off-by: David Lamparter <equinox@diac24.net>
---
Pretty much copypasted from IPv4.
---
net/ipv6/ip6mr.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 118 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index ec6e1509fc7c..a10bed171417 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt,
static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc,
int cmd);
static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt);
+static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack);
static int ip6mr_rtm_dumproute(struct sk_buff *skb,
struct netlink_callback *cb);
static void mroute_clean_tables(struct mr_table *mrt, int flags);
@@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void)
}
#endif
err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE,
- NULL, ip6mr_rtm_dumproute, 0);
+ ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0);
if (err == 0)
return 0;
@@ -2510,6 +2512,121 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk
rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS);
}
+static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb,
+ const struct nlmsghdr *nlh,
+ struct nlattr **tb,
+ struct netlink_ext_ack *extack)
+{
+ struct rtmsg *rtm;
+ int i, err;
+
+ if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
+ NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request");
+ return -EINVAL;
+ }
+
+ if (!netlink_strict_get_check(skb))
+ return nlmsg_parse_deprecated(nlh, sizeof(*rtm), tb, RTA_MAX,
+ rtm_ipv6_policy, extack);
+
+ rtm = nlmsg_data(nlh);
+ if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
+ (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
+ rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
+ rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
+ NL_SET_ERR_MSG(extack, "ipv6: Invalid values in header for multicast route get request");
+ return -EINVAL;
+ }
+
+ err = nlmsg_parse_deprecated_strict(nlh, sizeof(*rtm), tb, RTA_MAX,
+ rtm_ipv6_policy, extack);
+ if (err)
+ return err;
+
+ if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
+ (tb[RTA_DST] && !rtm->rtm_dst_len)) {
+ NL_SET_ERR_MSG(extack, "ipv6: rtm_src_len and rtm_dst_len must be 128 for IPv6");
+ return -EINVAL;
+ }
+
+ for (i = 0; i <= RTA_MAX; i++) {
+ if (!tb[i])
+ continue;
+
+ switch (i) {
+ case RTA_SRC:
+ case RTA_DST:
+ case RTA_TABLE:
+ break;
+ default:
+ NL_SET_ERR_MSG(extack, "ipv6: Unsupported attribute in multicast route get request");
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
+{
+ struct net *net = sock_net(in_skb->sk);
+ struct nlattr *tb[RTA_MAX + 1];
+ struct sk_buff *skb = NULL;
+ struct mfc6_cache *cache;
+ struct mr_table *mrt;
+ struct in6_addr src = {}, grp = {};
+ u32 tableid;
+ int err;
+
+ err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
+ if (err < 0)
+ goto errout;
+
+ if (tb[RTA_SRC])
+ src = nla_get_in6_addr(tb[RTA_SRC]);
+ if (tb[RTA_DST])
+ grp = nla_get_in6_addr(tb[RTA_DST]);
+ tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0;
+
+ mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT);
+ if (!mrt) {
+ NL_SET_ERR_MSG_MOD(extack, "ipv6 MR table does not exist");
+ err = -ENOENT;
+ goto errout_free;
+ }
+
+ /* entries are added/deleted only under RTNL */
+ rcu_read_lock();
+ cache = ip6mr_cache_find(mrt, &src, &grp);
+ rcu_read_unlock();
+ if (!cache) {
+ NL_SET_ERR_MSG_MOD(extack, "ipv6 MR cache entry not found");
+ err = -ENOENT;
+ goto errout_free;
+ }
+
+ skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL);
+ if (!skb) {
+ err = -ENOBUFS;
+ goto errout_free;
+ }
+
+ err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid,
+ nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0);
+ if (err < 0)
+ goto errout_free;
+
+ err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
+
+errout:
+ return err;
+
+errout_free:
+ kfree_skb(skb);
+ goto errout;
+}
+
static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
{
const struct nlmsghdr *nlh = cb->nlh;
--
2.36.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry
2022-06-30 13:30 [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry David Lamparter
2022-06-30 13:30 ` [PATCH 1/2] ipv6: make rtm_ipv6_policy available David Lamparter
2022-06-30 13:30 ` [PATCH 2/2] net: ip6mr: add RTM_GETROUTE netlink op David Lamparter
@ 2022-07-01 3:27 ` Jakub Kicinski
2022-07-01 7:58 ` [PATCH resend " David Lamparter
2022-07-04 9:58 ` [PATCH net-next v2] " David Lamparter
2 siblings, 2 replies; 17+ messages in thread
From: Jakub Kicinski @ 2022-07-01 3:27 UTC (permalink / raw)
To: David Lamparter; +Cc: netdev
On Thu, 30 Jun 2022 15:30:49 +0200 David Lamparter wrote:
> The IPv6 multicast routing code implements RTM_GETROUTE, but only for a
> dump request. Retrieving a single MFC entry is not currently possible
> via netlink.
>
> While most of the data here can also be retrieved with SIOCGETSGCNT_IN6,
> the lastused / RTA_EXPIRES is not included in the ioctl result (and we
> need it in FRR.)
>
> => Implement single-entry RTM_GETROUTE by copying and adapting the IPv4
> code.
>
> Tested against FRRouting's (work-in-progress) IPv6 PIM implementation.
You must CC maintainers (./scripts/get_maintainer.pl). With the patch
volume on netdev and constant trouble with gmail the chances of people
missing a submission are just too high. Please repost.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH resend net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry
2022-07-01 3:27 ` [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry Jakub Kicinski
@ 2022-07-01 7:58 ` David Lamparter
2022-07-01 7:58 ` [PATCH net-next 1/2] ipv6: make rtm_ipv6_policy available David Lamparter
2022-07-01 7:58 ` [PATCH net-next 2/2] net: ip6mr: add RTM_GETROUTE netlink op David Lamparter
2022-07-04 9:58 ` [PATCH net-next v2] " David Lamparter
1 sibling, 2 replies; 17+ messages in thread
From: David Lamparter @ 2022-07-01 7:58 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, David Ahern, Eric Dumazet, David Lamparter
[resend with a slightly random pick of Cc:s - apologies if I ended up
choosing poorly]
The IPv6 multicast routing code implements RTM_GETROUTE, but only for a
dump request. Retrieving a single MFC entry is not currently possible
via netlink.
While most of the data here can also be retrieved with SIOCGETSGCNT_IN6,
the lastused / RTA_EXPIRES is not included in the ioctl result (and we
need it in FRR.)
=> Implement single-entry RTM_GETROUTE by copying and adapting the IPv4
code.
Tested against FRRouting's (work-in-progress) IPv6 PIM implementation.
Cheers,
-David
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next 1/2] ipv6: make rtm_ipv6_policy available
2022-07-01 7:58 ` [PATCH resend " David Lamparter
@ 2022-07-01 7:58 ` David Lamparter
2022-07-01 7:58 ` [PATCH net-next 2/2] net: ip6mr: add RTM_GETROUTE netlink op David Lamparter
1 sibling, 0 replies; 17+ messages in thread
From: David Lamparter @ 2022-07-01 7:58 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, David Ahern, Eric Dumazet, David Lamparter
... so ip6mr.c can use it too (as it is in IPv4.)
Signed-off-by: David Lamparter <equinox@diac24.net>
---
1:1 analog to IPv4, where rtm_ipv4_policy is exposed for pretty exactly
the same thing. IPv6 just got away with not using this across file
boundaries so far.
---
include/net/ip6_fib.h | 1 +
net/ipv6/route.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 6268963d9599..a12fedea97de 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -483,6 +483,7 @@ void rt6_get_prefsrc(const struct rt6_info *rt, struct in6_addr *addr)
rcu_read_unlock();
}
+extern const struct nla_policy rtm_ipv6_policy[];
int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
struct fib6_config *cfg, gfp_t gfp_flags,
struct netlink_ext_ack *extack);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 1d6f75eef4bd..26c7b31abe96 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4964,7 +4964,7 @@ void rt6_mtu_change(struct net_device *dev, unsigned int mtu)
fib6_clean_all(dev_net(dev), rt6_mtu_change_route, &arg);
}
-static const struct nla_policy rtm_ipv6_policy[RTA_MAX+1] = {
+const struct nla_policy rtm_ipv6_policy[RTA_MAX+1] = {
[RTA_UNSPEC] = { .strict_start_type = RTA_DPORT + 1 },
[RTA_GATEWAY] = { .len = sizeof(struct in6_addr) },
[RTA_PREFSRC] = { .len = sizeof(struct in6_addr) },
--
2.36.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net-next 2/2] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-01 7:58 ` [PATCH resend " David Lamparter
2022-07-01 7:58 ` [PATCH net-next 1/2] ipv6: make rtm_ipv6_policy available David Lamparter
@ 2022-07-01 7:58 ` David Lamparter
2022-07-03 19:07 ` David Ahern
1 sibling, 1 reply; 17+ messages in thread
From: David Lamparter @ 2022-07-01 7:58 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, David Ahern, Eric Dumazet, David Lamparter
The IPv6 multicast routing code previously implemented only the dump
variant of RTM_GETROUTE. Implement single MFC item retrieval by copying
and adapting the respective IPv4 code.
Tested against FRRouting's IPv6 PIM stack.
Signed-off-by: David Lamparter <equinox@diac24.net>
---
Pretty much copypasted from IPv4.
---
net/ipv6/ip6mr.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 118 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index ec6e1509fc7c..a10bed171417 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt,
static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc,
int cmd);
static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt);
+static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack);
static int ip6mr_rtm_dumproute(struct sk_buff *skb,
struct netlink_callback *cb);
static void mroute_clean_tables(struct mr_table *mrt, int flags);
@@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void)
}
#endif
err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE,
- NULL, ip6mr_rtm_dumproute, 0);
+ ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0);
if (err == 0)
return 0;
@@ -2510,6 +2512,121 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk
rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS);
}
+static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb,
+ const struct nlmsghdr *nlh,
+ struct nlattr **tb,
+ struct netlink_ext_ack *extack)
+{
+ struct rtmsg *rtm;
+ int i, err;
+
+ if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
+ NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request");
+ return -EINVAL;
+ }
+
+ if (!netlink_strict_get_check(skb))
+ return nlmsg_parse_deprecated(nlh, sizeof(*rtm), tb, RTA_MAX,
+ rtm_ipv6_policy, extack);
+
+ rtm = nlmsg_data(nlh);
+ if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
+ (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
+ rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
+ rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
+ NL_SET_ERR_MSG(extack, "ipv6: Invalid values in header for multicast route get request");
+ return -EINVAL;
+ }
+
+ err = nlmsg_parse_deprecated_strict(nlh, sizeof(*rtm), tb, RTA_MAX,
+ rtm_ipv6_policy, extack);
+ if (err)
+ return err;
+
+ if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
+ (tb[RTA_DST] && !rtm->rtm_dst_len)) {
+ NL_SET_ERR_MSG(extack, "ipv6: rtm_src_len and rtm_dst_len must be 128 for IPv6");
+ return -EINVAL;
+ }
+
+ for (i = 0; i <= RTA_MAX; i++) {
+ if (!tb[i])
+ continue;
+
+ switch (i) {
+ case RTA_SRC:
+ case RTA_DST:
+ case RTA_TABLE:
+ break;
+ default:
+ NL_SET_ERR_MSG(extack, "ipv6: Unsupported attribute in multicast route get request");
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
+{
+ struct net *net = sock_net(in_skb->sk);
+ struct nlattr *tb[RTA_MAX + 1];
+ struct sk_buff *skb = NULL;
+ struct mfc6_cache *cache;
+ struct mr_table *mrt;
+ struct in6_addr src = {}, grp = {};
+ u32 tableid;
+ int err;
+
+ err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
+ if (err < 0)
+ goto errout;
+
+ if (tb[RTA_SRC])
+ src = nla_get_in6_addr(tb[RTA_SRC]);
+ if (tb[RTA_DST])
+ grp = nla_get_in6_addr(tb[RTA_DST]);
+ tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0;
+
+ mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT);
+ if (!mrt) {
+ NL_SET_ERR_MSG_MOD(extack, "ipv6 MR table does not exist");
+ err = -ENOENT;
+ goto errout_free;
+ }
+
+ /* entries are added/deleted only under RTNL */
+ rcu_read_lock();
+ cache = ip6mr_cache_find(mrt, &src, &grp);
+ rcu_read_unlock();
+ if (!cache) {
+ NL_SET_ERR_MSG_MOD(extack, "ipv6 MR cache entry not found");
+ err = -ENOENT;
+ goto errout_free;
+ }
+
+ skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL);
+ if (!skb) {
+ err = -ENOBUFS;
+ goto errout_free;
+ }
+
+ err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid,
+ nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0);
+ if (err < 0)
+ goto errout_free;
+
+ err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
+
+errout:
+ return err;
+
+errout_free:
+ kfree_skb(skb);
+ goto errout;
+}
+
static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
{
const struct nlmsghdr *nlh = cb->nlh;
--
2.36.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 2/2] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-01 7:58 ` [PATCH net-next 2/2] net: ip6mr: add RTM_GETROUTE netlink op David Lamparter
@ 2022-07-03 19:07 ` David Ahern
2022-07-04 8:35 ` David Lamparter
0 siblings, 1 reply; 17+ messages in thread
From: David Ahern @ 2022-07-03 19:07 UTC (permalink / raw)
To: David Lamparter, netdev; +Cc: David S. Miller, Eric Dumazet
On 7/1/22 1:58 AM, David Lamparter wrote:
> @@ -2510,6 +2512,121 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk
> rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS);
> }
>
> +static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb,
> + const struct nlmsghdr *nlh,
> + struct nlattr **tb,
> + struct netlink_ext_ack *extack)
> +{
> + struct rtmsg *rtm;
> + int i, err;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
> + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request");
> + return -EINVAL;
> + }
> +
> + if (!netlink_strict_get_check(skb))
> + return nlmsg_parse_deprecated(nlh, sizeof(*rtm), tb, RTA_MAX,
> + rtm_ipv6_policy, extack);
Since this is new code, it always operates in strict mode.
> +
> + rtm = nlmsg_data(nlh);
> + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
> + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
> + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
> + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
> + NL_SET_ERR_MSG(extack, "ipv6: Invalid values in header for multicast route get request");
> + return -EINVAL;
> + }
> +
> + err = nlmsg_parse_deprecated_strict(nlh, sizeof(*rtm), tb, RTA_MAX,
> + rtm_ipv6_policy, extack);
nlmsg_parse here.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next 2/2] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-03 19:07 ` David Ahern
@ 2022-07-04 8:35 ` David Lamparter
0 siblings, 0 replies; 17+ messages in thread
From: David Lamparter @ 2022-07-04 8:35 UTC (permalink / raw)
To: David Ahern; +Cc: netdev
On Sun, Jul 03, 2022 at 01:07:33PM -0600, David Ahern wrote:
> On 7/1/22 1:58 AM, David Lamparter wrote:
[...]
> > + if (!netlink_strict_get_check(skb))
> > + return nlmsg_parse_deprecated(nlh, sizeof(*rtm), tb, RTA_MAX,
> > + rtm_ipv6_policy, extack);
>
> Since this is new code, it always operates in strict mode.
>
[...]
> > + err = nlmsg_parse_deprecated_strict(nlh, sizeof(*rtm), tb, RTA_MAX,
> > + rtm_ipv6_policy, extack);
>
> nlmsg_parse here.
Thanks for the feedback! I've fixed the code, am currently retesting,
and will post a v2 shortly.
-David / equi
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v2] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-01 3:27 ` [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry Jakub Kicinski
2022-07-01 7:58 ` [PATCH resend " David Lamparter
@ 2022-07-04 9:58 ` David Lamparter
2022-07-04 10:22 ` Nikolay Aleksandrov
2022-07-05 3:38 ` [PATCH net-next v2] " kernel test robot
1 sibling, 2 replies; 17+ messages in thread
From: David Lamparter @ 2022-07-04 9:58 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, David Ahern, Eric Dumazet, David Lamparter
The IPv6 multicast routing code previously implemented only the dump
variant of RTM_GETROUTE. Implement single MFC item retrieval by copying
and adapting the respective IPv4 code.
Tested against FRRouting's IPv6 PIM stack.
Signed-off-by: David Lamparter <equinox@diac24.net>
Cc: David Ahern <dsahern@kernel.org>
---
v2: changeover to strict netlink attribute parsing. Doing so actually
exposed a bunch of other issues, first and foremost that rtm_ipv6_policy
does not have RTA_SRC or RTA_DST. This made reusing that policy rather
pointless so I changed it to use a separate rtm_ipv6_mr_policy.
Thanks again dsahern@ for the feedback on the previous version!
---
net/ipv6/ip6mr.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 127 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index ec6e1509fc7c..95dc366a2d9b 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt,
static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc,
int cmd);
static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt);
+static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack);
static int ip6mr_rtm_dumproute(struct sk_buff *skb,
struct netlink_callback *cb);
static void mroute_clean_tables(struct mr_table *mrt, int flags);
@@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void)
}
#endif
err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE,
- NULL, ip6mr_rtm_dumproute, 0);
+ ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0);
if (err == 0)
return 0;
@@ -2510,6 +2512,130 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk
rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS);
}
+const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = {
+ [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC },
+ [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
+ [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
+ [RTA_TABLE] = { .type = NLA_U32 },
+};
+
+static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb,
+ const struct nlmsghdr *nlh,
+ struct nlattr **tb,
+ struct netlink_ext_ack *extack)
+{
+ struct rtmsg *rtm;
+ int i, err;
+
+ if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
+ NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request");
+ return -EINVAL;
+ }
+
+ rtm = nlmsg_data(nlh);
+ if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
+ (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
+ rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
+ rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
+ NL_SET_ERR_MSG(extack,
+ "ipv6: Invalid values in header for multicast route get request");
+ return -EINVAL;
+ }
+
+ err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv6_mr_policy,
+ extack);
+ if (err)
+ return err;
+
+ if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
+ (tb[RTA_DST] && !rtm->rtm_dst_len)) {
+ NL_SET_ERR_MSG(extack, "ipv6: rtm_src_len and rtm_dst_len must be 128 for IPv6");
+ return -EINVAL;
+ }
+
+ /* rtm_ipv6_mr_policy does not list other attributes right now, but
+ * future changes may reuse rtm_ipv6_mr_policy with adding further
+ * attrs. Enforce the subset.
+ */
+ for (i = 0; i <= RTA_MAX; i++) {
+ if (!tb[i])
+ continue;
+
+ switch (i) {
+ case RTA_SRC:
+ case RTA_DST:
+ case RTA_TABLE:
+ break;
+ default:
+ NL_SET_ERR_MSG_ATTR(extack, tb[i],
+ "ipv6: Unsupported attribute in multicast route get request");
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
+{
+ struct net *net = sock_net(in_skb->sk);
+ struct nlattr *tb[RTA_MAX + 1];
+ struct sk_buff *skb = NULL;
+ struct mfc6_cache *cache;
+ struct mr_table *mrt;
+ struct in6_addr src = {}, grp = {};
+ u32 tableid;
+ int err;
+
+ err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
+ if (err < 0)
+ goto errout;
+
+ if (tb[RTA_SRC])
+ src = nla_get_in6_addr(tb[RTA_SRC]);
+ if (tb[RTA_DST])
+ grp = nla_get_in6_addr(tb[RTA_DST]);
+ tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0;
+
+ mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT);
+ if (!mrt) {
+ NL_SET_ERR_MSG_MOD(extack, "ipv6 MR table does not exist");
+ err = -ENOENT;
+ goto errout_free;
+ }
+
+ /* entries are added/deleted only under RTNL */
+ rcu_read_lock();
+ cache = ip6mr_cache_find(mrt, &src, &grp);
+ rcu_read_unlock();
+ if (!cache) {
+ NL_SET_ERR_MSG_MOD(extack, "ipv6 MR cache entry not found");
+ err = -ENOENT;
+ goto errout_free;
+ }
+
+ skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL);
+ if (!skb) {
+ err = -ENOBUFS;
+ goto errout_free;
+ }
+
+ err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid,
+ nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0);
+ if (err < 0)
+ goto errout_free;
+
+ err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
+
+errout:
+ return err;
+
+errout_free:
+ kfree_skb(skb);
+ goto errout;
+}
+
static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
{
const struct nlmsghdr *nlh = cb->nlh;
--
2.36.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v2] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-04 9:58 ` [PATCH net-next v2] " David Lamparter
@ 2022-07-04 10:22 ` Nikolay Aleksandrov
2022-07-04 10:38 ` David Lamparter
2022-07-05 3:38 ` [PATCH net-next v2] " kernel test robot
1 sibling, 1 reply; 17+ messages in thread
From: Nikolay Aleksandrov @ 2022-07-04 10:22 UTC (permalink / raw)
To: David Lamparter, netdev; +Cc: David S. Miller, David Ahern, Eric Dumazet
On 04/07/2022 12:58, David Lamparter wrote:
> The IPv6 multicast routing code previously implemented only the dump
> variant of RTM_GETROUTE. Implement single MFC item retrieval by copying
> and adapting the respective IPv4 code.
>
> Tested against FRRouting's IPv6 PIM stack.
>
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Cc: David Ahern <dsahern@kernel.org>
> ---
>
> v2: changeover to strict netlink attribute parsing. Doing so actually
> exposed a bunch of other issues, first and foremost that rtm_ipv6_policy
> does not have RTA_SRC or RTA_DST. This made reusing that policy rather
> pointless so I changed it to use a separate rtm_ipv6_mr_policy.
>
> Thanks again dsahern@ for the feedback on the previous version!
>
> ---
> net/ipv6/ip6mr.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 127 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index ec6e1509fc7c..95dc366a2d9b 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt,
> static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc,
> int cmd);
> static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt);
> +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack);
> static int ip6mr_rtm_dumproute(struct sk_buff *skb,
> struct netlink_callback *cb);
> static void mroute_clean_tables(struct mr_table *mrt, int flags);
> @@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void)
> }
> #endif
> err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE,
> - NULL, ip6mr_rtm_dumproute, 0);
> + ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0);
> if (err == 0)
> return 0;
>
> @@ -2510,6 +2512,130 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk
> rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS);
> }
>
> +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = {
> + [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC },
I don't think you need to add RTA_UNSPEC, nlmsg_parse() would reject it due to NL_VALIDATE_STRICT
> + [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
> + [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
> + [RTA_TABLE] = { .type = NLA_U32 },
> +};
> +
> +static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb,
> + const struct nlmsghdr *nlh,
> + struct nlattr **tb,
> + struct netlink_ext_ack *extack)
> +{
> + struct rtmsg *rtm;
> + int i, err;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
> + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request");
> + return -EINVAL;
> + }
I think you can drop this check if you...
> +
> + rtm = nlmsg_data(nlh);
> + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
> + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
> + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
> + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
> + NL_SET_ERR_MSG(extack,
> + "ipv6: Invalid values in header for multicast route get request");
> + return -EINVAL;
> + }
...move these after nlmsg_parse() because it already does the hdrlen check for you
> +
> + err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv6_mr_policy,
> + extack);
> + if (err)
> + return err;
> +
> + if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
> + (tb[RTA_DST] && !rtm->rtm_dst_len)) {
> + NL_SET_ERR_MSG(extack, "ipv6: rtm_src_len and rtm_dst_len must be 128 for IPv6");
> + return -EINVAL;
> + }
> +
> + /* rtm_ipv6_mr_policy does not list other attributes right now, but
> + * future changes may reuse rtm_ipv6_mr_policy with adding further
> + * attrs. Enforce the subset.
> + */
> + for (i = 0; i <= RTA_MAX; i++) {
> + if (!tb[i])
> + continue;
> +
> + switch (i) {
> + case RTA_SRC:
> + case RTA_DST:
> + case RTA_TABLE:
> + break;
> + default:
> + NL_SET_ERR_MSG_ATTR(extack, tb[i],
> + "ipv6: Unsupported attribute in multicast route get request");
> + return -EINVAL;
> + }
> + }
I think you can skip this loop as well, nlmsg_parse() shouldn't allow attributes that
don't have policy defined when policy is provided (i.e. they should show up as NLA_UNSPEC
and you should get "Error: Unknown attribute type.").
> +
> + return 0;
> +}
> +
> +static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack)
> +{
> + struct net *net = sock_net(in_skb->sk);
> + struct nlattr *tb[RTA_MAX + 1];
> + struct sk_buff *skb = NULL;
> + struct mfc6_cache *cache;
> + struct mr_table *mrt;
> + struct in6_addr src = {}, grp = {};
reverse xmas tree order
> + u32 tableid;
> + int err;
> +
> + err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
> + if (err < 0)
> + goto errout;
> +
> + if (tb[RTA_SRC])
> + src = nla_get_in6_addr(tb[RTA_SRC]);
> + if (tb[RTA_DST])
> + grp = nla_get_in6_addr(tb[RTA_DST]);
> + tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0;
> +
> + mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT);
> + if (!mrt) {
> + NL_SET_ERR_MSG_MOD(extack, "ipv6 MR table does not exist");
> + err = -ENOENT;
> + goto errout_free;
> + }
> +
> + /* entries are added/deleted only under RTNL */
> + rcu_read_lock();
> + cache = ip6mr_cache_find(mrt, &src, &grp);
> + rcu_read_unlock();
> + if (!cache) {
> + NL_SET_ERR_MSG_MOD(extack, "ipv6 MR cache entry not found");
> + err = -ENOENT;
> + goto errout_free;
> + }
> +
> + skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL);
> + if (!skb) {
> + err = -ENOBUFS;
> + goto errout_free;
> + }
> +
> + err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid,
> + nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0);
> + if (err < 0)
> + goto errout_free;
> +
> + err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> +
> +errout:
> + return err;
> +
> +errout_free:
> + kfree_skb(skb);
> + goto errout;
> +}
> +
> static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
> {
> const struct nlmsghdr *nlh = cb->nlh;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v2] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-04 10:22 ` Nikolay Aleksandrov
@ 2022-07-04 10:38 ` David Lamparter
2022-07-04 10:44 ` Nikolay Aleksandrov
0 siblings, 1 reply; 17+ messages in thread
From: David Lamparter @ 2022-07-04 10:38 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, David S. Miller, David Ahern, Eric Dumazet
On Mon, Jul 04, 2022 at 01:22:36PM +0300, Nikolay Aleksandrov wrote:
> On 04/07/2022 12:58, David Lamparter wrote:
> > +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = {
> > + [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC },
>
> I don't think you need to add RTA_UNSPEC, nlmsg_parse() would reject
> it due to NL_VALIDATE_STRICT
Will remove it.
> > + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
> > + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request");
> > + return -EINVAL;
> > + }
>
> I think you can drop this check if you...
>
> > +
> > + rtm = nlmsg_data(nlh);
> > + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
> > + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
> > + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
> > + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
> > + NL_SET_ERR_MSG(extack,
> > + "ipv6: Invalid values in header for multicast route get request");
> > + return -EINVAL;
> > + }
>
> ...move these after nlmsg_parse() because it already does the hdrlen
> check for you
Indeed it does. Moving it down.
[...]
> > + /* rtm_ipv6_mr_policy does not list other attributes right now, but
> > + * future changes may reuse rtm_ipv6_mr_policy with adding further
> > + * attrs. Enforce the subset.
> > + */
> > + for (i = 0; i <= RTA_MAX; i++) {
> > + if (!tb[i])
> > + continue;
> > +
> > + switch (i) {
> > + case RTA_SRC:
> > + case RTA_DST:
> > + case RTA_TABLE:
> > + break;
> > + default:
> > + NL_SET_ERR_MSG_ATTR(extack, tb[i],
> > + "ipv6: Unsupported attribute in multicast route get request");
> > + return -EINVAL;
> > + }
> > + }
>
> I think you can skip this loop as well, nlmsg_parse() shouldn't allow attributes that
> don't have policy defined when policy is provided (i.e. they should show up as NLA_UNSPEC
> and you should get "Error: Unknown attribute type.").
I left it in with the comment above:
> > + /* rtm_ipv6_mr_policy does not list other attributes right now, but
> > + * future changes may reuse rtm_ipv6_mr_policy with adding further
> > + * attrs. Enforce the subset.
> > + */
... to try and avoid silently starting to accept more attributes if/when
future patches add other netlink operations reusing the same policy but
with adding new attributes.
But I don't feel particularly about this - shall I remove it? (just
confirming with the rationale above)
> > + struct net *net = sock_net(in_skb->sk);
> > + struct nlattr *tb[RTA_MAX + 1];
> > + struct sk_buff *skb = NULL;
> > + struct mfc6_cache *cache;
> > + struct mr_table *mrt;
> > + struct in6_addr src = {}, grp = {};
>
> reverse xmas tree order
Ah. Wasn't aware of that coding style aspect. Fixing.
Thanks for the review!
-David/equi
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v2] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-04 10:38 ` David Lamparter
@ 2022-07-04 10:44 ` Nikolay Aleksandrov
2022-07-04 10:52 ` [PATCH net-next v3] " David Lamparter
0 siblings, 1 reply; 17+ messages in thread
From: Nikolay Aleksandrov @ 2022-07-04 10:44 UTC (permalink / raw)
To: David Lamparter; +Cc: netdev, David S. Miller, David Ahern, Eric Dumazet
On 04/07/2022 13:38, David Lamparter wrote:
> On Mon, Jul 04, 2022 at 01:22:36PM +0300, Nikolay Aleksandrov wrote:
>> On 04/07/2022 12:58, David Lamparter wrote:
>>> +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = {
>>> + [RTA_UNSPEC] = { .strict_start_type = RTA_UNSPEC },
>>
>> I don't think you need to add RTA_UNSPEC, nlmsg_parse() would reject
>> it due to NL_VALIDATE_STRICT
>
> Will remove it.
>
>>> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
>>> + NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request");
>>> + return -EINVAL;
>>> + }
>>
>> I think you can drop this check if you...
>>
>>> +
>>> + rtm = nlmsg_data(nlh);
>>> + if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
>>> + (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
>>> + rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
>>> + rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
>>> + NL_SET_ERR_MSG(extack,
>>> + "ipv6: Invalid values in header for multicast route get request");
>>> + return -EINVAL;
>>> + }
>>
>> ...move these after nlmsg_parse() because it already does the hdrlen
>> check for you
>
> Indeed it does. Moving it down.
>
> [...]
>>> + /* rtm_ipv6_mr_policy does not list other attributes right now, but
>>> + * future changes may reuse rtm_ipv6_mr_policy with adding further
>>> + * attrs. Enforce the subset.
>>> + */
>>> + for (i = 0; i <= RTA_MAX; i++) {
>>> + if (!tb[i])
>>> + continue;
>>> +
>>> + switch (i) {
>>> + case RTA_SRC:
>>> + case RTA_DST:
>>> + case RTA_TABLE:
>>> + break;
>>> + default:
>>> + NL_SET_ERR_MSG_ATTR(extack, tb[i],
>>> + "ipv6: Unsupported attribute in multicast route get request");
>>> + return -EINVAL;
>>> + }
>>> + }
>>
>> I think you can skip this loop as well, nlmsg_parse() shouldn't allow attributes that
>> don't have policy defined when policy is provided (i.e. they should show up as NLA_UNSPEC
>> and you should get "Error: Unknown attribute type.").
>
> I left it in with the comment above:
>
>>> + /* rtm_ipv6_mr_policy does not list other attributes right now, but
>>> + * future changes may reuse rtm_ipv6_mr_policy with adding further
>>> + * attrs. Enforce the subset.
>>> + */
>
> ... to try and avoid silently starting to accept more attributes if/when
> future patches add other netlink operations reusing the same policy but
> with adding new attributes.
>
They really should be using policies specific to their actions with only the allowed
attributes. Re-using this policy is ok only if those match, otherwise it's a bug IMO.
> But I don't feel particularly about this - shall I remove it? (just
> confirming with the rationale above)
>
I don't have a preference either, IMO if anyone re-uses this policy without making
sure the same attributes and types are needed is adding buggy code. Actually the thing
that I like about keeping the loop is the more specific error message, let's see what
others think.
>>> + struct net *net = sock_net(in_skb->sk);
>>> + struct nlattr *tb[RTA_MAX + 1];
>>> + struct sk_buff *skb = NULL;
>>> + struct mfc6_cache *cache;
>>> + struct mr_table *mrt;
>>> + struct in6_addr src = {}, grp = {};
>>
>> reverse xmas tree order
>
> Ah. Wasn't aware of that coding style aspect. Fixing.
>
> Thanks for the review!
>
>
> -David/equi
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v3] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-04 10:44 ` Nikolay Aleksandrov
@ 2022-07-04 10:52 ` David Lamparter
2022-07-05 2:50 ` Jakub Kicinski
0 siblings, 1 reply; 17+ messages in thread
From: David Lamparter @ 2022-07-04 10:52 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, David Ahern, Eric Dumazet, David Lamparter,
Nikolay Aleksandrov
The IPv6 multicast routing code previously implemented only the dump
variant of RTM_GETROUTE. Implement single MFC item retrieval by copying
and adapting the respective IPv4 code.
Tested against FRRouting's IPv6 PIM stack.
Signed-off-by: David Lamparter <equinox@diac24.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Nikolay Aleksandrov <razor@blackwall.org>
---
v3: reorder/remove some redundant bits, fix style. Thanks Nikolay for
pointing them out. The "extra" validation loop is still there for the
time being; happy to drop it if that's the consensus.
v2: changeover to strict netlink attribute parsing. Doing so actually
exposed a bunch of other issues, first and foremost that rtm_ipv6_policy
does not have RTA_SRC or RTA_DST. This made reusing that policy rather
pointless so I changed it to use a separate rtm_ipv6_mr_policy.
Thanks again dsahern@ for the feedback on the previous version!
---
net/ipv6/ip6mr.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 121 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index ec6e1509fc7c..9909ff77f5a6 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -95,6 +95,8 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt,
static void mr6_netlink_event(struct mr_table *mrt, struct mfc6_cache *mfc,
int cmd);
static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pkt);
+static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack);
static int ip6mr_rtm_dumproute(struct sk_buff *skb,
struct netlink_callback *cb);
static void mroute_clean_tables(struct mr_table *mrt, int flags);
@@ -1390,7 +1392,7 @@ int __init ip6_mr_init(void)
}
#endif
err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE,
- NULL, ip6mr_rtm_dumproute, 0);
+ ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0);
if (err == 0)
return 0;
@@ -2510,6 +2512,124 @@ static void mrt6msg_netlink_event(const struct mr_table *mrt, struct sk_buff *pk
rtnl_set_sk_err(net, RTNLGRP_IPV6_MROUTE_R, -ENOBUFS);
}
+const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = {
+ [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
+ [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
+ [RTA_TABLE] = { .type = NLA_U32 },
+};
+
+static int ip6mr_rtm_valid_getroute_req(struct sk_buff *skb,
+ const struct nlmsghdr *nlh,
+ struct nlattr **tb,
+ struct netlink_ext_ack *extack)
+{
+ struct rtmsg *rtm;
+ int i, err;
+
+ err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv6_mr_policy,
+ extack);
+ if (err)
+ return err;
+
+ rtm = nlmsg_data(nlh);
+ if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
+ (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
+ rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
+ rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
+ NL_SET_ERR_MSG(extack,
+ "ipv6: Invalid values in header for multicast route get request");
+ return -EINVAL;
+ }
+
+ if ((tb[RTA_SRC] && !rtm->rtm_src_len) ||
+ (tb[RTA_DST] && !rtm->rtm_dst_len)) {
+ NL_SET_ERR_MSG(extack, "ipv6: rtm_src_len and rtm_dst_len must be 128 for IPv6");
+ return -EINVAL;
+ }
+
+ /* rtm_ipv6_mr_policy does not list other attributes right now, but
+ * future changes may reuse rtm_ipv6_mr_policy with adding further
+ * attrs. Enforce the subset.
+ */
+ for (i = 0; i <= RTA_MAX; i++) {
+ if (!tb[i])
+ continue;
+
+ switch (i) {
+ case RTA_SRC:
+ case RTA_DST:
+ case RTA_TABLE:
+ break;
+ default:
+ NL_SET_ERR_MSG_ATTR(extack, tb[i],
+ "ipv6: Unsupported attribute in multicast route get request");
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
+{
+ struct net *net = sock_net(in_skb->sk);
+ struct in6_addr src = {}, grp = {};
+ struct nlattr *tb[RTA_MAX + 1];
+ struct sk_buff *skb = NULL;
+ struct mfc6_cache *cache;
+ struct mr_table *mrt;
+ u32 tableid;
+ int err;
+
+ err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
+ if (err < 0)
+ goto errout;
+
+ if (tb[RTA_SRC])
+ src = nla_get_in6_addr(tb[RTA_SRC]);
+ if (tb[RTA_DST])
+ grp = nla_get_in6_addr(tb[RTA_DST]);
+ tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0;
+
+ mrt = ip6mr_get_table(net, tableid ? tableid : RT_TABLE_DEFAULT);
+ if (!mrt) {
+ NL_SET_ERR_MSG_MOD(extack, "ipv6 MR table does not exist");
+ err = -ENOENT;
+ goto errout_free;
+ }
+
+ /* entries are added/deleted only under RTNL */
+ rcu_read_lock();
+ cache = ip6mr_cache_find(mrt, &src, &grp);
+ rcu_read_unlock();
+ if (!cache) {
+ NL_SET_ERR_MSG_MOD(extack, "ipv6 MR cache entry not found");
+ err = -ENOENT;
+ goto errout_free;
+ }
+
+ skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL);
+ if (!skb) {
+ err = -ENOBUFS;
+ goto errout_free;
+ }
+
+ err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid,
+ nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0);
+ if (err < 0)
+ goto errout_free;
+
+ err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
+
+errout:
+ return err;
+
+errout_free:
+ kfree_skb(skb);
+ goto errout;
+}
+
static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
{
const struct nlmsghdr *nlh = cb->nlh;
--
2.36.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v3] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-04 10:52 ` [PATCH net-next v3] " David Lamparter
@ 2022-07-05 2:50 ` Jakub Kicinski
2022-07-06 9:52 ` David Lamparter
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2022-07-05 2:50 UTC (permalink / raw)
To: David Lamparter
Cc: netdev, David S. Miller, David Ahern, Eric Dumazet, Nikolay Aleksandrov
On Mon, 4 Jul 2022 12:52:23 +0200 David Lamparter wrote:
> +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = {
> + [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
> + [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
> + [RTA_TABLE] = { .type = NLA_U32 },
> +};
net/ipv6/ip6mr.c:2515:25: warning: symbol 'rtm_ipv6_mr_policy' was not declared. Should it be static?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v2] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-04 9:58 ` [PATCH net-next v2] " David Lamparter
2022-07-04 10:22 ` Nikolay Aleksandrov
@ 2022-07-05 3:38 ` kernel test robot
1 sibling, 0 replies; 17+ messages in thread
From: kernel test robot @ 2022-07-05 3:38 UTC (permalink / raw)
To: David Lamparter, netdev
Cc: kbuild-all, David Ahern, Eric Dumazet, David Lamparter
Hi David,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lamparter/net-ip6mr-add-RTM_GETROUTE-netlink-op/20220704-180235
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d0bf1fe6454e976e39bc1524b9159fa2c0fcf321
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220705/202207051158.Vo7Qj6VM-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/ab5f843c60bd5c7ef119d8be390e67f9c43d8d3b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Lamparter/net-ip6mr-add-RTM_GETROUTE-netlink-op/20220704-180235
git checkout ab5f843c60bd5c7ef119d8be390e67f9c43d8d3b
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash net/ipv6/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> net/ipv6/ip6mr.c:2515:25: sparse: sparse: symbol 'rtm_ipv6_mr_policy' was not declared. Should it be static?
net/ipv6/ip6mr.c:407:13: sparse: sparse: context imbalance in 'ip6mr_vif_seq_start' - different lock contexts for basic block
net/ipv6/ip6mr.c: note: in included file (through include/linux/rculist.h, include/linux/pid.h, include/linux/sched.h, ...):
include/linux/rcupdate.h:726:9: sparse: sparse: context imbalance in 'ip6_mr_forward' - unexpected unlock
net/ipv6/ip6mr.c: note: in included file (through include/linux/mroute6.h):
include/linux/mroute_base.h:432:31: sparse: sparse: context imbalance in 'mr_mfc_seq_stop' - unexpected unlock
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v3] net: ip6mr: add RTM_GETROUTE netlink op
2022-07-05 2:50 ` Jakub Kicinski
@ 2022-07-06 9:52 ` David Lamparter
0 siblings, 0 replies; 17+ messages in thread
From: David Lamparter @ 2022-07-06 9:52 UTC (permalink / raw)
To: Jakub Kicinski, Nikolay Aleksandrov; +Cc: netdev
On Mon, Jul 04, 2022 at 07:50:11PM -0700, Jakub Kicinski wrote:
> On Mon, 4 Jul 2022 12:52:23 +0200 David Lamparter wrote:
> > +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = {
> > + [RTA_SRC] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
> > + [RTA_DST] = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
> > + [RTA_TABLE] = { .type = NLA_U32 },
> > +};
>
> net/ipv6/ip6mr.c:2515:25: warning: symbol 'rtm_ipv6_mr_policy' was not declared. Should it be static?
As the great poet of our time, Homer Simpson, would say: "d'oh"
After thinking about it for a bit more, I agree with Nikolay that the
policy shouldn't be reused, so apart from adding the "static" here I'll
also rename it to clarify it's the RTM_GETROUTE policy.
-equi/David
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-07-06 9:52 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-30 13:30 [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry David Lamparter
2022-06-30 13:30 ` [PATCH 1/2] ipv6: make rtm_ipv6_policy available David Lamparter
2022-06-30 13:30 ` [PATCH 2/2] net: ip6mr: add RTM_GETROUTE netlink op David Lamparter
2022-07-01 3:27 ` [PATCH net-next 0/2] ip6mr: implement RTM_GETROUTE for single entry Jakub Kicinski
2022-07-01 7:58 ` [PATCH resend " David Lamparter
2022-07-01 7:58 ` [PATCH net-next 1/2] ipv6: make rtm_ipv6_policy available David Lamparter
2022-07-01 7:58 ` [PATCH net-next 2/2] net: ip6mr: add RTM_GETROUTE netlink op David Lamparter
2022-07-03 19:07 ` David Ahern
2022-07-04 8:35 ` David Lamparter
2022-07-04 9:58 ` [PATCH net-next v2] " David Lamparter
2022-07-04 10:22 ` Nikolay Aleksandrov
2022-07-04 10:38 ` David Lamparter
2022-07-04 10:44 ` Nikolay Aleksandrov
2022-07-04 10:52 ` [PATCH net-next v3] " David Lamparter
2022-07-05 2:50 ` Jakub Kicinski
2022-07-06 9:52 ` David Lamparter
2022-07-05 3:38 ` [PATCH net-next v2] " kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.