[PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in dump request

[PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in dump request

[dsahern]

From: David Ahern <dsahern@gmail.com>

There are many use cases where a user wants to influence what is
returned in a dump for some rtnetlink command: one is wanting data
for a different namespace than the one the request is received and
another is limiting the amount of data returned in the dump to a
specific set of interest to userspace, reducing the cpu overhead of
both kernel and userspace. Unfortunately, the kernel has historically
not been strict with checking for the proper header or checking the
values passed in the header. This lenient implementation has allowed
iproute2 and other packages to pass any struct or data in the dump
request as long as the family is the first byte. For example, ifinfomsg
struct is used by iproute2 for all generic dump requests - links,
addresses, routes and rules when it is really only valid for link
requests.

There is 1 is example where the kernel deals with the wrong struct: link
dumps after VF support was added. Older iproute2 was sending rtgenmsg as
the header instead of ifinfomsg so a patch was added to try and detect
old userspace vs new:
e5eca6d41f53 ("rtnetlink: fix userspace API breakage for iproute2 < v3.9.0")

The latest example is Christian's patch set wanting to return addresses for
a target namespace. It guesses the header struct is an ifaddrmsg and if it
guesses wrong a netlink warning is generated in the kernel log on every
address dump which is unacceptable.

Another example where the kernel is a bit lenient is route dumps: iproute2
can send either a request with either ifinfomsg or a rtmsg as the header
struct, yet the kernel always treats the header as an rtmsg (see
inet_dump_fib and rtm_flags check).

How to resolve the problem of not breaking old userspace yet be able to
move forward with new features such as kernel side filtering which are
crucial for efficient operation at high scale?

This patch set addresses the problem by adding a new netlink flag,
NLM_F_DUMP_PROPER_HDR, that userspace can set to say "I have a clue, and
I am sending the right header struct" and that the struct fields and any
attributes after it should be used for filtering the data returned in the
dump.

Kernel side, the dump handlers are updated to check every field in the
header struct and all attributes passed. Only ones where filtering is
implemented are allowed to be set. Any other values cause the dump to fail
with EINVAL. If the new flag is honored by the kernel and the dump contents
adjusted by any data passed in the request, the dump handler sets the
NLM_F_DUMP_FILTERED flag in the netlink message header.

This is an RFC set with the address handlers updated. If the approach is
acceptable, then I will do the same to the other rtnetlink dump handlers.


David Ahern (5):
  net/netlink: Pass extack to dump callbacks
  net/ipv6: Refactor address dump to push inet6_fill_args to
    in6_dump_addrs
  netlink: introduce NLM_F_DUMP_PROPER_HDR flag
  net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR
  net/ipv6: Update inet6_dump_addr to support NLM_F_DUMP_PROPER_HDR

 include/linux/netlink.h      |   2 +
 include/uapi/linux/netlink.h |   1 +
 net/core/rtnetlink.c         |   1 +
 net/ipv4/devinet.c           |  52 +++++++++++++++++-----
 net/ipv6/addrconf.c          | 101 +++++++++++++++++++++++++++++--------------
 net/netlink/af_netlink.c     |   1 +
 6 files changed, 114 insertions(+), 44 deletions(-)

-- 
2.11.0

[PATCH RFC net-next 1/5] net/netlink: Pass extack to dump callbacks

[dsahern]

From: David Ahern <dsahern@gmail.com>

Pass extack to dump callbacks by adding extack to netlink_dump_control
and transferring to netlink_callback. Update rtnetlink as the first
user.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/linux/netlink.h  | 2 ++
 net/core/rtnetlink.c     | 1 +
 net/netlink/af_netlink.c | 1 +
 3 files changed, 4 insertions(+)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 71f121b66ca8..8fc90308a653 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -176,6 +176,7 @@ struct netlink_callback {
 	void			*data;
 	/* the module that dump function belong to */
 	struct module		*module;
+	struct netlink_ext_ack	*extack;
 	u16			family;
 	u16			min_dump_alloc;
 	unsigned int		prev_seq, seq;
@@ -197,6 +198,7 @@ struct netlink_dump_control {
 	int (*done)(struct netlink_callback *);
 	void *data;
 	struct module *module;
+	struct netlink_ext_ack *extack;
 	u16 min_dump_alloc;
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 35162e1b06ad..da91b38297d3 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4689,6 +4689,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 				.dump		= dumpit,
 				.min_dump_alloc	= min_dump_alloc,
 				.module		= owner,
+				.extack		= extack
 			};
 			err = netlink_dump_start(rtnl, skb, nlh, &c);
 			/* netlink_dump_start() will keep a reference on
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index e3a0538ec0be..7d9e735b32c4 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2307,6 +2307,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	cb->module = control->module;
 	cb->min_dump_alloc = control->min_dump_alloc;
 	cb->skb = skb;
+	cb->extack = control->extack;
 
 	if (control->start) {
 		ret = control->start(cb);
-- 
2.11.0

[PATCH RFC net-next 2/5] net/ipv6: Refactor address dump to push inet6_fill_args to in6_dump_addrs

[dsahern]

From: David Ahern <dsahern@gmail.com>

Pull the inet6_fill_args arg up to in6_dump_addrs and move netnsid
into it. Since IFA_TARGET_NETNSID is a kernel side filter add the
NLM_F_DUMP_FILTERED flag so userspace knows the request was honored.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/addrconf.c | 59 +++++++++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index a9a317322388..375ea9d9869b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4793,12 +4793,19 @@ static inline int inet6_ifaddr_msgsize(void)
 	       + nla_total_size(4)  /* IFA_RT_PRIORITY */;
 }
 
+enum addr_type_t {
+	UNICAST_ADDR,
+	MULTICAST_ADDR,
+	ANYCAST_ADDR,
+};
+
 struct inet6_fill_args {
 	u32 portid;
 	u32 seq;
 	int event;
 	unsigned int flags;
 	int netnsid;
+	enum addr_type_t type;
 };
 
 static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
@@ -4930,39 +4937,28 @@ static int inet6_fill_ifacaddr(struct sk_buff *skb, struct ifacaddr6 *ifaca,
 	return 0;
 }
 
-enum addr_type_t {
-	UNICAST_ADDR,
-	MULTICAST_ADDR,
-	ANYCAST_ADDR,
-};
-
 /* called with rcu_read_lock() */
 static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
-			  struct netlink_callback *cb, enum addr_type_t type,
-			  int s_ip_idx, int *p_ip_idx, int netnsid)
+			  struct netlink_callback *cb,
+			  int s_ip_idx, int *p_ip_idx,
+			  struct inet6_fill_args *fillargs)
 {
-	struct inet6_fill_args fillargs = {
-		.portid = NETLINK_CB(cb->skb).portid,
-		.seq = cb->nlh->nlmsg_seq,
-		.flags = NLM_F_MULTI,
-		.netnsid = netnsid,
-	};
 	struct ifmcaddr6 *ifmca;
 	struct ifacaddr6 *ifaca;
 	int err = 1;
 	int ip_idx = *p_ip_idx;
 
 	read_lock_bh(&idev->lock);
-	switch (type) {
+	switch (fillargs->type) {
 	case UNICAST_ADDR: {
 		struct inet6_ifaddr *ifa;
-		fillargs.event = RTM_NEWADDR;
+		fillargs->event = RTM_NEWADDR;
 
 		/* unicast address incl. temp addr */
 		list_for_each_entry(ifa, &idev->addr_list, if_list) {
 			if (++ip_idx < s_ip_idx)
 				continue;
-			err = inet6_fill_ifaddr(skb, ifa, &fillargs);
+			err = inet6_fill_ifaddr(skb, ifa, fillargs);
 			if (err < 0)
 				break;
 			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
@@ -4970,26 +4966,26 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
 		break;
 	}
 	case MULTICAST_ADDR:
-		fillargs.event = RTM_GETMULTICAST;
+		fillargs->event = RTM_GETMULTICAST;
 
 		/* multicast address */
 		for (ifmca = idev->mc_list; ifmca;
 		     ifmca = ifmca->next, ip_idx++) {
 			if (ip_idx < s_ip_idx)
 				continue;
-			err = inet6_fill_ifmcaddr(skb, ifmca, &fillargs);
+			err = inet6_fill_ifmcaddr(skb, ifmca, fillargs);
 			if (err < 0)
 				break;
 		}
 		break;
 	case ANYCAST_ADDR:
-		fillargs.event = RTM_GETANYCAST;
+		fillargs->event = RTM_GETANYCAST;
 		/* anycast address */
 		for (ifaca = idev->ac_list; ifaca;
 		     ifaca = ifaca->aca_next, ip_idx++) {
 			if (ip_idx < s_ip_idx)
 				continue;
-			err = inet6_fill_ifacaddr(skb, ifaca, &fillargs);
+			err = inet6_fill_ifacaddr(skb, ifaca, fillargs);
 			if (err < 0)
 				break;
 		}
@@ -5005,10 +5001,16 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
 static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
 			   enum addr_type_t type)
 {
+	struct inet6_fill_args fillargs = {
+		.portid = NETLINK_CB(cb->skb).portid,
+		.seq = cb->nlh->nlmsg_seq,
+		.flags = NLM_F_MULTI,
+		.netnsid = -1,
+		.type = type,
+	};
 	struct net *net = sock_net(skb->sk);
 	struct nlattr *tb[IFA_MAX+1];
 	struct net *tgt_net = net;
-	int netnsid = -1;
 	int h, s_h;
 	int idx, ip_idx;
 	int s_idx, s_ip_idx;
@@ -5023,11 +5025,14 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
 	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
 			ifa_ipv6_policy, NULL) >= 0) {
 		if (tb[IFA_TARGET_NETNSID]) {
-			netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
+			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
 
-			tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
+			tgt_net = rtnl_get_net_ns_capable(skb->sk,
+							  fillargs.netnsid);
 			if (IS_ERR(tgt_net))
 				return PTR_ERR(tgt_net);
+
+			fillargs.flags |= NLM_F_DUMP_FILTERED;
 		}
 	}
 
@@ -5046,8 +5051,8 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
 			if (!idev)
 				goto cont;
 
-			if (in6_dump_addrs(idev, skb, cb, type,
-					   s_ip_idx, &ip_idx, netnsid) < 0)
+			if (in6_dump_addrs(idev, skb, cb, s_ip_idx, &ip_idx,
+					   &fillargs) < 0)
 				goto done;
 cont:
 			idx++;
@@ -5058,7 +5063,7 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
 	cb->args[0] = h;
 	cb->args[1] = idx;
 	cb->args[2] = ip_idx;
-	if (netnsid >= 0)
+	if (fillargs.netnsid >= 0)
 		put_net(tgt_net);
 
 	return skb->len;
-- 
2.11.0

[PATCH RFC net-next 3/5] netlink: introduce NLM_F_DUMP_PROPER_HDR flag

[dsahern]

From: David Ahern <dsahern@gmail.com>

Add a new flag, NLM_F_DUMP_PROPER_HDR, for userspace to indicate to the
kernel that it believes it is sending the right header struct for the
dump message type (ifinfomsg, ifaddrmsg, rtmsg, fib_rule_hdr, ...).

Setting the flag in the netlink message header indicates to the kernel
it should do rigid checking on all data passed in the dump request and
filter the data returned based on data passed in.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/uapi/linux/netlink.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 776bc92e9118..e722bed88dee 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -57,6 +57,7 @@ struct nlmsghdr {
 #define NLM_F_ECHO		0x08	/* Echo this request 		*/
 #define NLM_F_DUMP_INTR		0x10	/* Dump was inconsistent due to sequence change */
 #define NLM_F_DUMP_FILTERED	0x20	/* Dump was filtered as requested */
+#define NLM_F_DUMP_PROPER_HDR	0x40	/* Dump request has the proper header for type */
 
 /* Modifiers to GET request */
 #define NLM_F_ROOT	0x100	/* specify tree	root	*/
-- 
2.11.0

[PATCH RFC net-next 4/5] net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR

[dsahern]

From: David Ahern <dsahern@gmail.com>

Update inet_dump_ifaddr to check for NLM_F_DUMP_PROPER_HDR in the netlink
message header. If the flag is set, the dump request is expected to have
an ifaddrmsg struct as the header potentially followed by one or more
attributes. Any data passed in the header or as an attribute is taken as
a request to influence the data returned. Only values suppored by the
dump handler are allowed to be non-0 or set in the request. At the moment
only the IFA_TARGET_NETNSID attribute is supported. Follow on patches
will support for other fields (e.g., honor ifa_index and only return data
for the given device index).

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv4/devinet.c | 52 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 44d931a3cd50..1e06a21cd8f4 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1661,15 +1661,15 @@ static int inet_fill_ifaddr(struct sk_buff *skb, struct in_ifaddr *ifa,
 
 static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
 {
+	struct netlink_ext_ack *extack = cb->extack;
+	const struct nlmsghdr *nlh = cb->nlh;
 	struct inet_fill_args fillargs = {
 		.portid = NETLINK_CB(cb->skb).portid,
-		.seq = cb->nlh->nlmsg_seq,
+		.seq = nlh->nlmsg_seq,
 		.event = RTM_NEWADDR,
-		.flags = NLM_F_MULTI,
 		.netnsid = -1,
 	};
 	struct net *net = sock_net(skb->sk);
-	struct nlattr *tb[IFA_MAX+1];
 	struct net *tgt_net = net;
 	int h, s_h;
 	int idx, s_idx;
@@ -1683,15 +1683,45 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
 	s_idx = idx = cb->args[1];
 	s_ip_idx = ip_idx = cb->args[2];
 
-	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
-			ifa_ipv4_policy, NULL) >= 0) {
-		if (tb[IFA_TARGET_NETNSID]) {
-			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
+	if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
+		struct nlattr *tb[IFA_MAX+1];
+		struct ifaddrmsg *ifm;
+		int err, i;
+
+		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
+			NL_SET_ERR_MSG(extack, "Invalid header");
+			return -EINVAL;
+		}
+
+		ifm = (struct ifaddrmsg *) nlmsg_data(cb->nlh);
+		if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
+			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
+			return -EINVAL;
+		}
+		if (ifm->ifa_index) {
+			NL_SET_ERR_MSG(extack, "Filter by device index not supported");
+			return -EINVAL;
+		}
+		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
+				ifa_ipv4_policy, NULL);
+		if (err < 0)
+			return err;
 
-			tgt_net = rtnl_get_net_ns_capable(skb->sk,
-							  fillargs.netnsid);
-			if (IS_ERR(tgt_net))
-				return PTR_ERR(tgt_net);
+		for (i = 0; i < IFA_MAX; ++i) {
+			if (i == IFA_TARGET_NETNSID) {
+				fillargs.netnsid = nla_get_s32(tb[i]);
+
+				tgt_net = rtnl_get_net_ns_capable(skb->sk,
+								  fillargs.netnsid);
+				if (IS_ERR(tgt_net))
+					return PTR_ERR(tgt_net);
+
+				fillargs.flags |= NLM_F_DUMP_FILTERED;
+			}
+			if (tb[i]) {
+				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
+				return -EINVAL;
+			}
 		}
 	}
 
-- 
2.11.0

[PATCH RFC net-next 5/5] net/ipv6: Update inet6_dump_addr to support NLM_F_DUMP_PROPER_HDR

[dsahern]

From: David Ahern <dsahern@gmail.com>

Update inet6_dump_addr to check for NLM_F_DUMP_PROPER_HDR in the netlink
message header. If the flag is set, the dump request is expected to have
an ifaddrmsg struct as the header potentially followed by one or more
attributes. Any data passed in the header or as an attribute is taken as
a request to influence the data returned. Only values suppored by the
dump handler are allowed to be non-0 or set in the request. At the moment
only the IFA_TARGET_NETNSID attribute is supported. Follow on patches
will support for other fields (e.g., honor ifa_index and only return data
for the given device index).

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/addrconf.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 10 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 375ea9d9869b..888e5a4b8dd2 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5001,6 +5001,8 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
 static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
 			   enum addr_type_t type)
 {
+	struct netlink_ext_ack *extack = cb->extack;
+	const struct nlmsghdr *nlh = cb->nlh;
 	struct inet6_fill_args fillargs = {
 		.portid = NETLINK_CB(cb->skb).portid,
 		.seq = cb->nlh->nlmsg_seq,
@@ -5009,7 +5011,6 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
 		.type = type,
 	};
 	struct net *net = sock_net(skb->sk);
-	struct nlattr *tb[IFA_MAX+1];
 	struct net *tgt_net = net;
 	int h, s_h;
 	int idx, ip_idx;
@@ -5022,17 +5023,46 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
 	s_idx = idx = cb->args[1];
 	s_ip_idx = ip_idx = cb->args[2];
 
-	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
-			ifa_ipv6_policy, NULL) >= 0) {
-		if (tb[IFA_TARGET_NETNSID]) {
-			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
+	if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
+		struct nlattr *tb[IFA_MAX+1];
+		struct ifaddrmsg *ifm;
+		int err, i;
+
+		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
+			NL_SET_ERR_MSG(extack, "Invalid header");
+			return -EINVAL;
+		}
 
-			tgt_net = rtnl_get_net_ns_capable(skb->sk,
-							  fillargs.netnsid);
-			if (IS_ERR(tgt_net))
-				return PTR_ERR(tgt_net);
+		ifm = (struct ifaddrmsg *)nlmsg_data(cb->nlh);
+		if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
+			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
+			return -EINVAL;
+		}
+		if (ifm->ifa_index) {
+			NL_SET_ERR_MSG(extack, "Filter by device index not supported");
+			return -EINVAL;
+		}
 
-			fillargs.flags |= NLM_F_DUMP_FILTERED;
+		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
+				  ifa_ipv6_policy, NULL);
+		if (err < 0)
+			return err;
+
+		for (i = 0; i < IFA_MAX; ++i) {
+			if (i == IFA_TARGET_NETNSID) {
+				fillargs.netnsid = nla_get_s32(tb[i]);
+
+				tgt_net = rtnl_get_net_ns_capable(skb->sk,
+								  fillargs.netnsid);
+				if (IS_ERR(tgt_net))
+					return PTR_ERR(tgt_net);
+
+				fillargs.flags |= NLM_F_DUMP_FILTERED;
+			}
+			if (tb[i]) {
+				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
+				return -EINVAL;
+			}
 		}
 	}
 
-- 
2.11.0

Re: [PATCH RFC net-next 4/5] net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR

[Christian Brauner]

On Fri, Sep 28, 2018 at 08:45:01AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Update inet_dump_ifaddr to check for NLM_F_DUMP_PROPER_HDR in the netlink
> message header. If the flag is set, the dump request is expected to have
> an ifaddrmsg struct as the header potentially followed by one or more
> attributes. Any data passed in the header or as an attribute is taken as
> a request to influence the data returned. Only values suppored by the
> dump handler are allowed to be non-0 or set in the request. At the moment
> only the IFA_TARGET_NETNSID attribute is supported. Follow on patches
> will support for other fields (e.g., honor ifa_index and only return data
> for the given device index).
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  net/ipv4/devinet.c | 52 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 44d931a3cd50..1e06a21cd8f4 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1661,15 +1661,15 @@ static int inet_fill_ifaddr(struct sk_buff *skb, struct in_ifaddr *ifa,
>  
>  static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>  {
> +	struct netlink_ext_ack *extack = cb->extack;
> +	const struct nlmsghdr *nlh = cb->nlh;
>  	struct inet_fill_args fillargs = {
>  		.portid = NETLINK_CB(cb->skb).portid,
> -		.seq = cb->nlh->nlmsg_seq,
> +		.seq = nlh->nlmsg_seq,
>  		.event = RTM_NEWADDR,
> -		.flags = NLM_F_MULTI,
>  		.netnsid = -1,
>  	};
>  	struct net *net = sock_net(skb->sk);
> -	struct nlattr *tb[IFA_MAX+1];
>  	struct net *tgt_net = net;
>  	int h, s_h;
>  	int idx, s_idx;
> @@ -1683,15 +1683,45 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>  	s_idx = idx = cb->args[1];
>  	s_ip_idx = ip_idx = cb->args[2];
>  
> -	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> -			ifa_ipv4_policy, NULL) >= 0) {
> -		if (tb[IFA_TARGET_NETNSID]) {
> -			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> +	if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
> +		struct nlattr *tb[IFA_MAX+1];
> +		struct ifaddrmsg *ifm;
> +		int err, i;
> +
> +		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> +			NL_SET_ERR_MSG(extack, "Invalid header");
> +			return -EINVAL;
> +		}
> +
> +		ifm = (struct ifaddrmsg *) nlmsg_data(cb->nlh);
> +		if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
> +			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> +			return -EINVAL;
> +		}
> +		if (ifm->ifa_index) {
> +			NL_SET_ERR_MSG(extack, "Filter by device index not supported");
> +			return -EINVAL;
> +		}
> +		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> +				ifa_ipv4_policy, NULL);
> +		if (err < 0)
> +			return err;
>  
> -			tgt_net = rtnl_get_net_ns_capable(skb->sk,
> -							  fillargs.netnsid);
> -			if (IS_ERR(tgt_net))
> -				return PTR_ERR(tgt_net);
> +		for (i = 0; i < IFA_MAX; ++i) {
> +			if (i == IFA_TARGET_NETNSID) {
> +				fillargs.netnsid = nla_get_s32(tb[i]);
> +
> +				tgt_net = rtnl_get_net_ns_capable(skb->sk,
> +								  fillargs.netnsid);
> +				if (IS_ERR(tgt_net))
> +					return PTR_ERR(tgt_net);
> +
> +				fillargs.flags |= NLM_F_DUMP_FILTERED;
> +			}
> +			if (tb[i]) {
> +				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> +				return -EINVAL;
> +			}

That loop doesn't do what it promises, no? Shouldn't it be:

		for (i = 0; i < IFA_MAX; ++i) {
			if (i == IFA_TARGET_NETNSID && tb[IFA_TARGET_NETNSID]) {
				fillargs.netnsid = nla_get_s32(tb[i]);

				tgt_net = rtnl_get_net_ns_capable(skb->sk,
								  fillargs.netnsid);
				if (IS_ERR(tgt_net))
					return PTR_ERR(tgt_net);

				fillargs.flags |= NLM_F_DUMP_FILTERED;
				continue;
			}

			if (tb[i]) {
				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
				return -EINVAL;
			}
		}

>  	}
>  
> -- 
> 2.11.0
> 

Re: [PATCH RFC net-next 5/5] net/ipv6: Update inet6_dump_addr to support NLM_F_DUMP_PROPER_HDR

[Christian Brauner]

On Fri, Sep 28, 2018 at 08:45:02AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Update inet6_dump_addr to check for NLM_F_DUMP_PROPER_HDR in the netlink
> message header. If the flag is set, the dump request is expected to have
> an ifaddrmsg struct as the header potentially followed by one or more
> attributes. Any data passed in the header or as an attribute is taken as
> a request to influence the data returned. Only values suppored by the
> dump handler are allowed to be non-0 or set in the request. At the moment
> only the IFA_TARGET_NETNSID attribute is supported. Follow on patches
> will support for other fields (e.g., honor ifa_index and only return data
> for the given device index).
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  net/ipv6/addrconf.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 10 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 375ea9d9869b..888e5a4b8dd2 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -5001,6 +5001,8 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  			   enum addr_type_t type)
>  {
> +	struct netlink_ext_ack *extack = cb->extack;
> +	const struct nlmsghdr *nlh = cb->nlh;
>  	struct inet6_fill_args fillargs = {
>  		.portid = NETLINK_CB(cb->skb).portid,
>  		.seq = cb->nlh->nlmsg_seq,
> @@ -5009,7 +5011,6 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  		.type = type,
>  	};
>  	struct net *net = sock_net(skb->sk);
> -	struct nlattr *tb[IFA_MAX+1];
>  	struct net *tgt_net = net;
>  	int h, s_h;
>  	int idx, ip_idx;
> @@ -5022,17 +5023,46 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  	s_idx = idx = cb->args[1];
>  	s_ip_idx = ip_idx = cb->args[2];
>  
> -	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> -			ifa_ipv6_policy, NULL) >= 0) {
> -		if (tb[IFA_TARGET_NETNSID]) {
> -			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> +	if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
> +		struct nlattr *tb[IFA_MAX+1];
> +		struct ifaddrmsg *ifm;
> +		int err, i;
> +
> +		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> +			NL_SET_ERR_MSG(extack, "Invalid header");
> +			return -EINVAL;
> +		}
>  
> -			tgt_net = rtnl_get_net_ns_capable(skb->sk,
> -							  fillargs.netnsid);
> -			if (IS_ERR(tgt_net))
> -				return PTR_ERR(tgt_net);
> +		ifm = (struct ifaddrmsg *)nlmsg_data(cb->nlh);
> +		if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
> +			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> +			return -EINVAL;
> +		}
> +		if (ifm->ifa_index) {
> +			NL_SET_ERR_MSG(extack, "Filter by device index not supported");
> +			return -EINVAL;
> +		}
>  
> -			fillargs.flags |= NLM_F_DUMP_FILTERED;
> +		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> +				  ifa_ipv6_policy, NULL);
> +		if (err < 0)
> +			return err;
> +
> +		for (i = 0; i < IFA_MAX; ++i) {
> +			if (i == IFA_TARGET_NETNSID) {
> +				fillargs.netnsid = nla_get_s32(tb[i]);
> +
> +				tgt_net = rtnl_get_net_ns_capable(skb->sk,
> +								  fillargs.netnsid);
> +				if (IS_ERR(tgt_net))
> +					return PTR_ERR(tgt_net);
> +
> +				fillargs.flags |= NLM_F_DUMP_FILTERED;
> +			}
> +			if (tb[i]) {
> +				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> +				return -EINVAL;
> +			}

Same comment/question as for ipv4. Shouldn't it be:


   	for (i = 0; i < IFA_MAX; ++i) {
   		if (i == IFA_TARGET_NETNSID && tb[i]) {
   			fillargs.netnsid = nla_get_s32(tb[i]);

   			tgt_net = rtnl_get_net_ns_capable(skb->sk,
   							  fillargs.netnsid);
   			if (IS_ERR(tgt_net))
   				return PTR_ERR(tgt_net);

   			fillargs.flags |= NLM_F_DUMP_FILTERED;
			continue;
   		}
   		if (tb[i]) {
   			NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
   			return -EINVAL;
   		}
>  		}
>  	}
>  
> -- 
> 2.11.0
> 

Re: [PATCH RFC net-next 4/5] net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR

[David Ahern]

On 9/28/18 12:41 PM, Christian Brauner wrote:
>> @@ -1683,15 +1683,45 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>>  	s_idx = idx = cb->args[1];
>>  	s_ip_idx = ip_idx = cb->args[2];
>>  
>> -	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
>> -			ifa_ipv4_policy, NULL) >= 0) {
>> -		if (tb[IFA_TARGET_NETNSID]) {
>> -			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
>> +	if (nlh->nlmsg_flags & NLM_F_DUMP_PROPER_HDR) {
>> +		struct nlattr *tb[IFA_MAX+1];
>> +		struct ifaddrmsg *ifm;
>> +		int err, i;
>> +
>> +		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
>> +			NL_SET_ERR_MSG(extack, "Invalid header");
>> +			return -EINVAL;
>> +		}
>> +
>> +		ifm = (struct ifaddrmsg *) nlmsg_data(cb->nlh);
>> +		if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
>> +			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
>> +			return -EINVAL;
>> +		}
>> +		if (ifm->ifa_index) {
>> +			NL_SET_ERR_MSG(extack, "Filter by device index not supported");
>> +			return -EINVAL;
>> +		}
>> +		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
>> +				ifa_ipv4_policy, NULL);
>> +		if (err < 0)
>> +			return err;
>>  
>> -			tgt_net = rtnl_get_net_ns_capable(skb->sk,
>> -							  fillargs.netnsid);
>> -			if (IS_ERR(tgt_net))
>> -				return PTR_ERR(tgt_net);
>> +		for (i = 0; i < IFA_MAX; ++i) {
>> +			if (i == IFA_TARGET_NETNSID) {
>> +				fillargs.netnsid = nla_get_s32(tb[i]);
>> +
>> +				tgt_net = rtnl_get_net_ns_capable(skb->sk,
>> +								  fillargs.netnsid);
>> +				if (IS_ERR(tgt_net))
>> +					return PTR_ERR(tgt_net);
>> +
>> +				fillargs.flags |= NLM_F_DUMP_FILTERED;
>> +			}
>> +			if (tb[i]) {
>> +				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
>> +				return -EINVAL;
>> +			}
> 
> That loop doesn't do what it promises, no? Shouldn't it be:

your right, that should be:
			} else if (tb[i]) {

Re: [PATCH RFC net-next 1/5] net/netlink: Pass extack to dump callbacks

[Christian Brauner]

On Fri, Sep 28, 2018 at 08:44:58AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Pass extack to dump callbacks by adding extack to netlink_dump_control
> and transferring to netlink_callback. Update rtnetlink as the first
> user.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

I like the idea of passing down extack.

Acked-by: Christian Brauner <christian@brauner.io>

> ---
>  include/linux/netlink.h  | 2 ++
>  net/core/rtnetlink.c     | 1 +
>  net/netlink/af_netlink.c | 1 +
>  3 files changed, 4 insertions(+)
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 71f121b66ca8..8fc90308a653 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -176,6 +176,7 @@ struct netlink_callback {
>  	void			*data;
>  	/* the module that dump function belong to */
>  	struct module		*module;
> +	struct netlink_ext_ack	*extack;
>  	u16			family;
>  	u16			min_dump_alloc;
>  	unsigned int		prev_seq, seq;
> @@ -197,6 +198,7 @@ struct netlink_dump_control {
>  	int (*done)(struct netlink_callback *);
>  	void *data;
>  	struct module *module;
> +	struct netlink_ext_ack *extack;
>  	u16 min_dump_alloc;
>  };
>  
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 35162e1b06ad..da91b38297d3 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4689,6 +4689,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>  				.dump		= dumpit,
>  				.min_dump_alloc	= min_dump_alloc,
>  				.module		= owner,
> +				.extack		= extack
>  			};
>  			err = netlink_dump_start(rtnl, skb, nlh, &c);
>  			/* netlink_dump_start() will keep a reference on
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index e3a0538ec0be..7d9e735b32c4 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2307,6 +2307,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
>  	cb->module = control->module;
>  	cb->min_dump_alloc = control->min_dump_alloc;
>  	cb->skb = skb;
> +	cb->extack = control->extack;
>  
>  	if (control->start) {
>  		ret = control->start(cb);
> -- 
> 2.11.0
> 

Re: [PATCH RFC net-next 2/5] net/ipv6: Refactor address dump to push inet6_fill_args to in6_dump_addrs

[Christian Brauner]

On Fri, Sep 28, 2018 at 08:44:59AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Pull the inet6_fill_args arg up to in6_dump_addrs and move netnsid
> into it. Since IFA_TARGET_NETNSID is a kernel side filter add the
> NLM_F_DUMP_FILTERED flag so userspace knows the request was honored.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

Acked-by: Christian Brauner <christian@brauner.io>

> ---
>  net/ipv6/addrconf.c | 59 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index a9a317322388..375ea9d9869b 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4793,12 +4793,19 @@ static inline int inet6_ifaddr_msgsize(void)
>  	       + nla_total_size(4)  /* IFA_RT_PRIORITY */;
>  }
>  
> +enum addr_type_t {
> +	UNICAST_ADDR,
> +	MULTICAST_ADDR,
> +	ANYCAST_ADDR,
> +};
> +
>  struct inet6_fill_args {
>  	u32 portid;
>  	u32 seq;
>  	int event;
>  	unsigned int flags;
>  	int netnsid;
> +	enum addr_type_t type;
>  };
>  
>  static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
> @@ -4930,39 +4937,28 @@ static int inet6_fill_ifacaddr(struct sk_buff *skb, struct ifacaddr6 *ifaca,
>  	return 0;
>  }
>  
> -enum addr_type_t {
> -	UNICAST_ADDR,
> -	MULTICAST_ADDR,
> -	ANYCAST_ADDR,
> -};
> -
>  /* called with rcu_read_lock() */
>  static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
> -			  struct netlink_callback *cb, enum addr_type_t type,
> -			  int s_ip_idx, int *p_ip_idx, int netnsid)
> +			  struct netlink_callback *cb,
> +			  int s_ip_idx, int *p_ip_idx,
> +			  struct inet6_fill_args *fillargs)
>  {
> -	struct inet6_fill_args fillargs = {
> -		.portid = NETLINK_CB(cb->skb).portid,
> -		.seq = cb->nlh->nlmsg_seq,
> -		.flags = NLM_F_MULTI,
> -		.netnsid = netnsid,
> -	};
>  	struct ifmcaddr6 *ifmca;
>  	struct ifacaddr6 *ifaca;
>  	int err = 1;
>  	int ip_idx = *p_ip_idx;
>  
>  	read_lock_bh(&idev->lock);
> -	switch (type) {
> +	switch (fillargs->type) {
>  	case UNICAST_ADDR: {
>  		struct inet6_ifaddr *ifa;
> -		fillargs.event = RTM_NEWADDR;
> +		fillargs->event = RTM_NEWADDR;
>  
>  		/* unicast address incl. temp addr */
>  		list_for_each_entry(ifa, &idev->addr_list, if_list) {
>  			if (++ip_idx < s_ip_idx)
>  				continue;
> -			err = inet6_fill_ifaddr(skb, ifa, &fillargs);
> +			err = inet6_fill_ifaddr(skb, ifa, fillargs);
>  			if (err < 0)
>  				break;
>  			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
> @@ -4970,26 +4966,26 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  		break;
>  	}
>  	case MULTICAST_ADDR:
> -		fillargs.event = RTM_GETMULTICAST;
> +		fillargs->event = RTM_GETMULTICAST;
>  
>  		/* multicast address */
>  		for (ifmca = idev->mc_list; ifmca;
>  		     ifmca = ifmca->next, ip_idx++) {
>  			if (ip_idx < s_ip_idx)
>  				continue;
> -			err = inet6_fill_ifmcaddr(skb, ifmca, &fillargs);
> +			err = inet6_fill_ifmcaddr(skb, ifmca, fillargs);
>  			if (err < 0)
>  				break;
>  		}
>  		break;
>  	case ANYCAST_ADDR:
> -		fillargs.event = RTM_GETANYCAST;
> +		fillargs->event = RTM_GETANYCAST;
>  		/* anycast address */
>  		for (ifaca = idev->ac_list; ifaca;
>  		     ifaca = ifaca->aca_next, ip_idx++) {
>  			if (ip_idx < s_ip_idx)
>  				continue;
> -			err = inet6_fill_ifacaddr(skb, ifaca, &fillargs);
> +			err = inet6_fill_ifacaddr(skb, ifaca, fillargs);
>  			if (err < 0)
>  				break;
>  		}
> @@ -5005,10 +5001,16 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,
>  static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  			   enum addr_type_t type)
>  {
> +	struct inet6_fill_args fillargs = {
> +		.portid = NETLINK_CB(cb->skb).portid,
> +		.seq = cb->nlh->nlmsg_seq,
> +		.flags = NLM_F_MULTI,
> +		.netnsid = -1,
> +		.type = type,
> +	};
>  	struct net *net = sock_net(skb->sk);
>  	struct nlattr *tb[IFA_MAX+1];
>  	struct net *tgt_net = net;
> -	int netnsid = -1;
>  	int h, s_h;
>  	int idx, ip_idx;
>  	int s_idx, s_ip_idx;
> @@ -5023,11 +5025,14 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  	if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
>  			ifa_ipv6_policy, NULL) >= 0) {
>  		if (tb[IFA_TARGET_NETNSID]) {
> -			netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> +			fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
>  
> -			tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
> +			tgt_net = rtnl_get_net_ns_capable(skb->sk,
> +							  fillargs.netnsid);
>  			if (IS_ERR(tgt_net))
>  				return PTR_ERR(tgt_net);
> +
> +			fillargs.flags |= NLM_F_DUMP_FILTERED;
>  		}
>  	}
>  
> @@ -5046,8 +5051,8 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  			if (!idev)
>  				goto cont;
>  
> -			if (in6_dump_addrs(idev, skb, cb, type,
> -					   s_ip_idx, &ip_idx, netnsid) < 0)
> +			if (in6_dump_addrs(idev, skb, cb, s_ip_idx, &ip_idx,
> +					   &fillargs) < 0)
>  				goto done;
>  cont:
>  			idx++;
> @@ -5058,7 +5063,7 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,
>  	cb->args[0] = h;
>  	cb->args[1] = idx;
>  	cb->args[2] = ip_idx;
> -	if (netnsid >= 0)
> +	if (fillargs.netnsid >= 0)
>  		put_net(tgt_net);
>  
>  	return skb->len;
> -- 
> 2.11.0
> 

Re: [PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in dump request

[Christian Brauner]

On Fri, Sep 28, 2018 at 08:44:57AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> There are many use cases where a user wants to influence what is
> returned in a dump for some rtnetlink command: one is wanting data
> for a different namespace than the one the request is received and
> another is limiting the amount of data returned in the dump to a
> specific set of interest to userspace, reducing the cpu overhead of
> both kernel and userspace. Unfortunately, the kernel has historically
> not been strict with checking for the proper header or checking the
> values passed in the header. This lenient implementation has allowed
> iproute2 and other packages to pass any struct or data in the dump
> request as long as the family is the first byte. For example, ifinfomsg
> struct is used by iproute2 for all generic dump requests - links,
> addresses, routes and rules when it is really only valid for link
> requests.
> 
> There is 1 is example where the kernel deals with the wrong struct: link
> dumps after VF support was added. Older iproute2 was sending rtgenmsg as
> the header instead of ifinfomsg so a patch was added to try and detect
> old userspace vs new:
> e5eca6d41f53 ("rtnetlink: fix userspace API breakage for iproute2 < v3.9.0")
> 
> The latest example is Christian's patch set wanting to return addresses for
> a target namespace. It guesses the header struct is an ifaddrmsg and if it
> guesses wrong a netlink warning is generated in the kernel log on every
> address dump which is unacceptable.
> 
> Another example where the kernel is a bit lenient is route dumps: iproute2
> can send either a request with either ifinfomsg or a rtmsg as the header
> struct, yet the kernel always treats the header as an rtmsg (see
> inet_dump_fib and rtm_flags check).
> 
> How to resolve the problem of not breaking old userspace yet be able to
> move forward with new features such as kernel side filtering which are
> crucial for efficient operation at high scale?
> 
> This patch set addresses the problem by adding a new netlink flag,
> NLM_F_DUMP_PROPER_HDR, that userspace can set to say "I have a clue, and
> I am sending the right header struct" and that the struct fields and any
> attributes after it should be used for filtering the data returned in the
> dump.
> 
> Kernel side, the dump handlers are updated to check every field in the
> header struct and all attributes passed. Only ones where filtering is
> implemented are allowed to be set. Any other values cause the dump to fail
> with EINVAL. If the new flag is honored by the kernel and the dump contents
> adjusted by any data passed in the request, the dump handler sets the
> NLM_F_DUMP_FILTERED flag in the netlink message header.
> 
> This is an RFC set with the address handlers updated. If the approach is
> acceptable, then I will do the same to the other rtnetlink dump handlers.

I like the idea and I think this might be a good solution to this
problem. If we can agree on this in favor of mine I'm all for it!
Thanks, David!

Christian

> 
> 
> David Ahern (5):
>   net/netlink: Pass extack to dump callbacks
>   net/ipv6: Refactor address dump to push inet6_fill_args to
>     in6_dump_addrs
>   netlink: introduce NLM_F_DUMP_PROPER_HDR flag
>   net/ipv4: Update inet_dump_ifaddr to support NLM_F_DUMP_PROPER_HDR
>   net/ipv6: Update inet6_dump_addr to support NLM_F_DUMP_PROPER_HDR
> 
>  include/linux/netlink.h      |   2 +
>  include/uapi/linux/netlink.h |   1 +
>  net/core/rtnetlink.c         |   1 +
>  net/ipv4/devinet.c           |  52 +++++++++++++++++-----
>  net/ipv6/addrconf.c          | 101 +++++++++++++++++++++++++++++--------------
>  net/netlink/af_netlink.c     |   1 +
>  6 files changed, 114 insertions(+), 44 deletions(-)
> 
> -- 
> 2.11.0
>