[PATCH net-next 0/4] rtnl fdb get

[PATCH net-next 0/4] rtnl fdb get

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This series adds support for rtnl fdb get similar to
route get.

Roopa Prabhu (4):
  net: rtnetlink: support for fdb get
  bridge: support for ndo_fdb_get
  vxlan: support for ndo_fdb_get
  selftests: net: rtnetlink.sh: add fdb get test

 drivers/net/vxlan.c                      |  35 ++++++++++
 include/linux/netdevice.h                |   6 +-
 net/bridge/br_device.c                   |   1 +
 net/bridge/br_fdb.c                      |  24 +++++++
 net/bridge/br_private.h                  |   2 +
 net/core/rtnetlink.c                     | 107 ++++++++++++++++++++++++++++++-
 tools/testing/selftests/net/rtnetlink.sh |  53 +++++++++++++++
 7 files changed, 226 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH net-next 1/4] net: rtnetlink: support for fdb get

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This patch adds support for fdb get similar to
route get. arguments can be any of the following (similar to fdb add/del/dump):
[bridge, mac, vlan] or
[bridge_port, mac, vlan, flags=[NTF_MASTER]] or
[dev, mac, [vni|vlan], flags=[NTF_SELF]]

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 include/linux/netdevice.h |   7 ++-
 net/core/rtnetlink.c      | 107 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 811632d..1377d08 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1387,7 +1387,12 @@ struct net_device_ops {
 						struct net_device *dev,
 						struct net_device *filter_dev,
 						int *idx);
-
+	int			(*ndo_fdb_get)(struct sk_buff *skb,
+					       struct nlattr *tb[],
+					       struct net_device *dev,
+					       const unsigned char *addr,
+					       u16 vid, u32 portid, u32 seq,
+					       struct netlink_ext_ack *extack);
 	int			(*ndo_bridge_setlink)(struct net_device *dev,
 						      struct nlmsghdr *nlh,
 						      u16 flags,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index f8bdb8ad..fcea76b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4021,6 +4021,111 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
+static int rtnl_fdb_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
+			struct netlink_ext_ack *extack)
+{
+	const struct net_device_ops *ops = NULL;
+	struct net *net = sock_net(in_skb->sk);
+	struct net_device *dev = NULL, *br_dev = NULL;
+	struct nlattr *tb[NDA_MAX + 1];
+	struct sk_buff *skb;
+	struct ndmsg *ndm;
+	int br_idx = 0;
+	u8 *addr;
+	u16 vid;
+	int err;
+
+	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL, extack);
+	if (err < 0)
+		return err;
+
+	ndm = nlmsg_data(nlh);
+	if (ndm->ndm_ifindex) {
+		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
+		if (!dev) {
+			NL_SET_ERR_MSG(extack, "unknown dev ifindex");
+			return -ENODEV;
+		}
+	}
+
+	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
+		NL_SET_ERR_MSG(extack, "invalid address");
+		return -EINVAL;
+	}
+
+	addr = nla_data(tb[NDA_LLADDR]);
+
+	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
+	if (err)
+		return err;
+
+	if (tb[NDA_MASTER]) {
+		if (dev) {
+			NL_SET_ERR_MSG(extack, "master and dev are mutually exclusive");
+			return -EINVAL;
+		}
+
+		br_idx = nla_get_u32(tb[NDA_MASTER]);
+		br_dev = __dev_get_by_index(net, br_idx);
+		if (!br_dev) {
+			NL_SET_ERR_MSG(extack, "invalid master ifindex");
+			return -EINVAL;
+		}
+		ops = br_dev->netdev_ops;
+	}
+
+	if (dev) {
+		if (!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) {
+			if (!(dev->priv_flags & IFF_BRIDGE_PORT)) {
+				NL_SET_ERR_MSG(extack, "dev is not a bridge port");
+				return -EINVAL;
+			}
+			br_dev = netdev_master_upper_dev_get(dev);
+			if (!br_dev) {
+				NL_SET_ERR_MSG(extack, "master of dev not found");
+				return -EINVAL;
+			}
+			ops = br_dev->netdev_ops;
+		} else {
+			if (!(ndm->ndm_flags & NTF_SELF)) {
+				NL_SET_ERR_MSG(extack, "missing NTF_SELF");
+				return -EINVAL;
+			}
+			ops = dev->netdev_ops;
+		}
+	}
+
+	if (!br_dev && !dev) {
+		NL_SET_ERR_MSG(extack, "Unable to find device");
+		return -ENODEV;
+	}
+
+	if (!ops || !ops->ndo_fdb_get) {
+		NL_SET_ERR_MSG(extack, "fdb get operation not supported by device");
+		return -EOPNOTSUPP;
+	}
+
+	skb = nlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!skb)
+		return -ENOBUFS;
+
+	if (br_dev)
+		err = ops->ndo_fdb_get(skb, tb, br_dev, addr, vid,
+				       NETLINK_CB(in_skb).portid,
+				       nlh->nlmsg_seq, extack);
+	else
+		err = ops->ndo_fdb_get(skb, tb, dev, addr, vid,
+				       NETLINK_CB(in_skb).portid,
+				       nlh->nlmsg_seq, extack);
+	if (err)
+		goto out;
+
+	return rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
+out:
+	kfree_skb(skb);
+	return err;
+}
+
 static int brport_nla_put_flag(struct sk_buff *skb, u32 flags, u32 mask,
 			       unsigned int attrnum, unsigned int flag)
 {
@@ -5081,7 +5186,7 @@ void __init rtnetlink_init(void)
 
 	rtnl_register(PF_BRIDGE, RTM_NEWNEIGH, rtnl_fdb_add, NULL, 0);
 	rtnl_register(PF_BRIDGE, RTM_DELNEIGH, rtnl_fdb_del, NULL, 0);
-	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, NULL, rtnl_fdb_dump, 0);
+	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
 
 	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
 	rtnl_register(PF_BRIDGE, RTM_DELLINK, rtnl_bridge_dellink, NULL, 0);
-- 
2.1.4

[PATCH net-next 2/4] bridge: support for ndo_fdb_get

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This patch implements ndo_fdb_get for the bridge
fdb.

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 net/bridge/br_device.c  |  1 +
 net/bridge/br_fdb.c     | 26 ++++++++++++++++++++++++++
 net/bridge/br_private.h |  3 +++
 3 files changed, 30 insertions(+)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 9f41a5d..013323b 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -403,6 +403,7 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_fdb_add		 = br_fdb_add,
 	.ndo_fdb_del		 = br_fdb_delete,
 	.ndo_fdb_dump		 = br_fdb_dump,
+	.ndo_fdb_get		 = br_fdb_get,
 	.ndo_bridge_getlink	 = br_getlink,
 	.ndo_bridge_setlink	 = br_setlink,
 	.ndo_bridge_dellink	 = br_dellink,
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 38b1d0d..fafff6c 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -773,6 +773,32 @@ int br_fdb_dump(struct sk_buff *skb,
 	return err;
 }
 
+int br_fdb_get(struct sk_buff *skb,
+	       struct nlattr *tb[],
+	       struct net_device *dev,
+	       const unsigned char *addr,
+	       u16 vid, u32 portid, u32 seq,
+	       struct netlink_ext_ack *extack)
+{
+	struct net_bridge *br = netdev_priv(dev);
+	struct net_bridge_fdb_entry *f;
+	int err = 0;
+
+	rcu_read_lock();
+	f = br_fdb_find_rcu(br, addr, vid);
+	if (!f) {
+		NL_SET_ERR_MSG(extack, "fdb entry not found");
+		err = -ENOENT;
+		goto errout;
+	}
+
+	err = fdb_fill_info(skb, br, f, portid, seq,
+			    RTM_NEWNEIGH, 0);
+errout:
+	rcu_read_unlock();
+	return err;
+}
+
 /* Update (create or replace) forwarding database entry */
 static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 			 const u8 *addr, u16 state, u16 flags, u16 vid,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index ff3dfb2..d240b3e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -575,6 +575,9 @@ int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
 	       const unsigned char *addr, u16 vid, u16 nlh_flags);
 int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 		struct net_device *dev, struct net_device *fdev, int *idx);
+int br_fdb_get(struct sk_buff *skb, struct nlattr *tb[], struct net_device *dev,
+	       const unsigned char *addr, u16 vid, u32 portid, u32 seq,
+	       struct netlink_ext_ack *extack);
 int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
 void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
 int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
-- 
2.1.4

[PATCH net-next 3/4] vxlan: support for ndo_fdb_get

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This patch implements ndo_fdb_get for a vxlan device.

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 drivers/net/vxlan.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 49d4b58..43bc077 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1152,6 +1152,44 @@ static int vxlan_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 	return err;
 }
 
+static int vxlan_fdb_get(struct sk_buff *skb,
+			 struct nlattr *tb[],
+			 struct net_device *dev,
+			 const unsigned char *addr,
+			 u16 vid, u32 portid, u32 seq,
+			 struct netlink_ext_ack *extack)
+{
+	struct vxlan_dev *vxlan = netdev_priv(dev);
+	struct vxlan_fdb *f;
+	__be32 vni;
+	int err;
+
+	if (tb[NDA_VNI]) {
+		if (nla_len(tb[NDA_VNI]) != sizeof(u32)) {
+			NL_SET_ERR_MSG(extack, "Invalid VNI");
+			return -EINVAL;
+		}
+		vni = cpu_to_be32(nla_get_u32(tb[NDA_VNI]));
+	} else {
+		vni = vxlan->default_dst.remote_vni;
+	}
+
+	rcu_read_lock();
+
+	f = __vxlan_find_mac(vxlan, addr, vni);
+	if (!f) {
+		NL_SET_ERR_MSG(extack, "fdb entry not found");
+		err = -ENOENT;
+		goto errout;
+	}
+
+	err = vxlan_fdb_info(skb, vxlan, f, portid, seq,
+			     RTM_NEWNEIGH, 0, first_remote_rcu(f));
+errout:
+	rcu_read_unlock();
+	return err;
+}
+
 /* Watch incoming packets to learn mapping between Ethernet address
  * and Tunnel endpoint.
  * Return true if packet is bogus and should be dropped.
@@ -2805,6 +2843,7 @@ static const struct net_device_ops vxlan_netdev_ether_ops = {
 	.ndo_fdb_add		= vxlan_fdb_add,
 	.ndo_fdb_del		= vxlan_fdb_delete,
 	.ndo_fdb_dump		= vxlan_fdb_dump,
+	.ndo_fdb_get		= vxlan_fdb_get,
 	.ndo_fill_metadata_dst	= vxlan_fill_metadata_dst,
 };
 
-- 
2.1.4

[PATCH net-next 4/4] selftests: net: rtnetlink.sh: add fdb get test

[Roopa Prabhu]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

tests the below three cases of bridge fdb get:
[bridge, mac, vlan]
[bridge_port, mac, vlan, flags=[NTF_MASTER]]
[vxlandev, mac, flags=NTF_SELF]

depends on iproute2 support for bridge fdb get.

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 tools/testing/selftests/net/rtnetlink.sh | 53 ++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index e101af5..bb3436f 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -955,6 +955,58 @@ kci_test_ip6erspan()
 	ip netns del "$testns"
 }
 
+kci_test_fdb_get()
+{
+	IP="ip -netns testns"
+	BRIDGE="bridge -netns testns"
+	brdev="test-br0"
+	vxlandev="vxlan10"
+	test_mac=de:ad:be:ef:13:37
+	localip="10.0.2.2"
+	dstip="10.0.2.3"
+	ret=0
+
+	bridge fdb help 2>&1 |grep -q 'bridge fdb get'
+	if [ $? -ne 0 ];then
+		echo "SKIP: fdb get tests: iproute2 too old"
+		return $ksft_skip
+	fi
+
+	ip netns add testns
+	if [ $? -ne 0 ]; then
+		echo "SKIP fdb get tests: cannot add net namespace $testns"
+		return $ksft_skip
+	fi
+
+	$IP link add "$vxlandev" type vxlan id 10 local $localip \
+                dstport 4789 2>/dev/null
+	check_err $?
+	$IP link add name "$brdev" type bridge &>/dev/null
+	check_err $?
+	$IP link set dev "$vxlandev" master "$brdev" &>/dev/null
+	check_err $?
+	$BRIDGE fdb add $test_mac dev "$vxlandev" master &>/dev/null
+	check_err $?
+	$BRIDGE fdb add $test_mac dev "$vxlandev" dst $dstip self &>/dev/null
+	check_err $?
+
+	$BRIDGE fdb get $test_mac brport "$vxlandev" 2>/dev/null | grep -q "dev $vxlandev master $brdev"
+	check_err $?
+	$BRIDGE fdb get $test_mac br "$brdev" 2>/dev/null | grep -q "dev $vxlandev master $brdev"
+	check_err $?
+	$BRIDGE fdb get $test_mac dev "$vxlandev" self 2>/dev/null | grep -q "dev $vxlandev dst $dstip"
+	check_err $?
+
+	ip netns del testns &>/dev/null
+
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: bridge fdb get"
+		return 1
+	fi
+
+	echo "PASS: bridge fdb get"
+}
+
 kci_test_rtnl()
 {
 	kci_add_dummy
@@ -979,6 +1031,7 @@ kci_test_rtnl()
 	kci_test_macsec
 	kci_test_ipsec
 	kci_test_ipsec_offload
+	kci_test_fdb_get
 
 	kci_del_dummy
 }
-- 
2.1.4

Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[David Ahern]

On 12/14/18 10:43 AM, Roopa Prabhu wrote:
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f8bdb8ad..fcea76b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4021,6 +4021,111 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len;
>  }
>  
> +static int rtnl_fdb_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> +			struct netlink_ext_ack *extack)
> +{
> +	const struct net_device_ops *ops = NULL;
> +	struct net *net = sock_net(in_skb->sk);
> +	struct net_device *dev = NULL, *br_dev = NULL;
> +	struct nlattr *tb[NDA_MAX + 1];
> +	struct sk_buff *skb;
> +	struct ndmsg *ndm;
> +	int br_idx = 0;
> +	u8 *addr;
> +	u16 vid;
> +	int err;
> +
> +	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL, extack);

New stuff should go in with strict checking, so nlmsg_parse_strict and a
check for any unsupported attributes (NDA) or ndm entries in the request.

Also, please add an nla_policy for easier NDA attributes ...

> +	if (err < 0)
> +		return err;
> +
> +	ndm = nlmsg_data(nlh);
> +	if (ndm->ndm_ifindex) {
> +		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
> +		if (!dev) {
> +			NL_SET_ERR_MSG(extack, "unknown dev ifindex");

General comment on all of the extack messages: No abbreviations in the
message, and it is nice to always start with a capital letter. "Unknown
device index"

> +			return -ENODEV;
> +		}
> +	}
> +
> +	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
> +		NL_SET_ERR_MSG(extack, "invalid address");
> +		return -EINVAL;
> +	}
> +
> +	addr = nla_data(tb[NDA_LLADDR]);
> +
> +	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
> +	if (err)
> +		return err;
> +
> +	if (tb[NDA_MASTER]) {
> +		if (dev) {
> +			NL_SET_ERR_MSG(extack, "master and dev are mutually exclusive");
> +			return -EINVAL;
> +		}
> +
> +		br_idx = nla_get_u32(tb[NDA_MASTER]);

... so you know that NDA_MASTER is a u32.

> +		br_dev = __dev_get_by_index(net, br_idx);
> +		if (!br_dev) {
> +			NL_SET_ERR_MSG(extack, "invalid master ifindex");
> +			return -EINVAL;
> +		}
> +		ops = br_dev->netdev_ops;
> +	}
> +

Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[Roopa Prabhu]

On Fri, Dec 14, 2018 at 9:55 AM David Ahern <dsa@cumulusnetworks.com> wrote:
>
> On 12/14/18 10:43 AM, Roopa Prabhu wrote:
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index f8bdb8ad..fcea76b 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -4021,6 +4021,111 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
> >       return skb->len;
> >  }
> >
> > +static int rtnl_fdb_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> > +                     struct netlink_ext_ack *extack)
> > +{
> > +     const struct net_device_ops *ops = NULL;
> > +     struct net *net = sock_net(in_skb->sk);
> > +     struct net_device *dev = NULL, *br_dev = NULL;
> > +     struct nlattr *tb[NDA_MAX + 1];
> > +     struct sk_buff *skb;
> > +     struct ndmsg *ndm;
> > +     int br_idx = 0;
> > +     u8 *addr;
> > +     u16 vid;
> > +     int err;
> > +
> > +     err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL, extack);
>
> New stuff should go in with strict checking, so nlmsg_parse_strict and a
> check for any unsupported attributes (NDA) or ndm entries in the request.

makes sense.

>
> Also, please add an nla_policy for easier NDA attributes ...

yes agreed. NDA attrs have not had a policy.
maybe an incremental patch ?, NDA_* attributes are used in bridge
vxlan and neighbour code, need a common place to put the policy.
(suggestions ?)


>
> > +     if (err < 0)
> > +             return err;
> > +
> > +     ndm = nlmsg_data(nlh);
> > +     if (ndm->ndm_ifindex) {
> > +             dev = __dev_get_by_index(net, ndm->ndm_ifindex);
> > +             if (!dev) {
> > +                     NL_SET_ERR_MSG(extack, "unknown dev ifindex");
>
> General comment on all of the extack messages: No abbreviations in the
> message, and it is nice to always start with a capital letter. "Unknown
> device index"

ack,

will send a v2


>
> > +                     return -ENODEV;
> > +             }
> > +     }
> > +
> > +     if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
> > +             NL_SET_ERR_MSG(extack, "invalid address");
> > +             return -EINVAL;
> > +     }
> > +
> > +     addr = nla_data(tb[NDA_LLADDR]);
> > +
> > +     err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
> > +     if (err)
> > +             return err;
> > +
> > +     if (tb[NDA_MASTER]) {
> > +             if (dev) {
> > +                     NL_SET_ERR_MSG(extack, "master and dev are mutually exclusive");
> > +                     return -EINVAL;
> > +             }
> > +
> > +             br_idx = nla_get_u32(tb[NDA_MASTER]);
>
> ... so you know that NDA_MASTER is a u32.
>
> > +             br_dev = __dev_get_by_index(net, br_idx);
> > +             if (!br_dev) {
> > +                     NL_SET_ERR_MSG(extack, "invalid master ifindex");
> > +                     return -EINVAL;
> > +             }
> > +             ops = br_dev->netdev_ops;
> > +     }
> > +
>

Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[Nikolay Aleksandrov]

On 14/12/2018 19:43, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> This patch adds support for fdb get similar to
> route get. arguments can be any of the following (similar to fdb add/del/dump):
> [bridge, mac, vlan] or
> [bridge_port, mac, vlan, flags=[NTF_MASTER]] or
> [dev, mac, [vni|vlan], flags=[NTF_SELF]]
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  include/linux/netdevice.h |   7 ++-
>  net/core/rtnetlink.c      | 107 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 112 insertions(+), 2 deletions(-)
> 

Just saw there will be a v2, a few style nits and one suggestion below for it. :)

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 811632d..1377d08 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1387,7 +1387,12 @@ struct net_device_ops {
>  						struct net_device *dev,
>  						struct net_device *filter_dev,
>  						int *idx);
> -
> +	int			(*ndo_fdb_get)(struct sk_buff *skb,
> +					       struct nlattr *tb[],
> +					       struct net_device *dev,
> +					       const unsigned char *addr,
> +					       u16 vid, u32 portid, u32 seq,
> +					       struct netlink_ext_ack *extack);
>  	int			(*ndo_bridge_setlink)(struct net_device *dev,
>  						      struct nlmsghdr *nlh,
>  						      u16 flags,
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f8bdb8ad..fcea76b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4021,6 +4021,111 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len;
>  }
>  
> +static int rtnl_fdb_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> +			struct netlink_ext_ack *extack)
> +{
> +	const struct net_device_ops *ops = NULL;
> +	struct net *net = sock_net(in_skb->sk);
> +	struct net_device *dev = NULL, *br_dev = NULL;
^^
Reverse xmas, seems like this should be on top.

> +	struct nlattr *tb[NDA_MAX + 1];
> +	struct sk_buff *skb;
> +	struct ndmsg *ndm;
> +	int br_idx = 0;
> +	u8 *addr;
> +	u16 vid;
> +	int err;
> +
> +	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL, extack);
> +	if (err < 0)
> +		return err;
> +
> +	ndm = nlmsg_data(nlh);
> +	if (ndm->ndm_ifindex) {
> +		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
> +		if (!dev) {
> +			NL_SET_ERR_MSG(extack, "unknown dev ifindex");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
> +		NL_SET_ERR_MSG(extack, "invalid address");
> +		return -EINVAL;
> +	}
> +
> +	addr = nla_data(tb[NDA_LLADDR]);
> +
> +	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
> +	if (err)
> +		return err;
> +
> +	if (tb[NDA_MASTER]) {
> +		if (dev) {
> +			NL_SET_ERR_MSG(extack, "master and dev are mutually exclusive");
> +			return -EINVAL;
> +		}
> +
> +		br_idx = nla_get_u32(tb[NDA_MASTER]);
> +		br_dev = __dev_get_by_index(net, br_idx);
> +		if (!br_dev) {
> +			NL_SET_ERR_MSG(extack, "invalid master ifindex");
> +			return -EINVAL;
> +		}
> +		ops = br_dev->netdev_ops;
> +	}
> +
> +	if (dev) {
> +		if (!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) {
^^
Please put the NTF_MASTER check in ().

> +			if (!(dev->priv_flags & IFF_BRIDGE_PORT)) {
> +				NL_SET_ERR_MSG(extack, "dev is not a bridge port");
> +				return -EINVAL;
> +			}
> +			br_dev = netdev_master_upper_dev_get(dev);
> +			if (!br_dev) {
> +				NL_SET_ERR_MSG(extack, "master of dev not found");
> +				return -EINVAL;
> +			}
> +			ops = br_dev->netdev_ops;
> +		} else {
> +			if (!(ndm->ndm_flags & NTF_SELF)) {
> +				NL_SET_ERR_MSG(extack, "missing NTF_SELF");
> +				return -EINVAL;
> +			}
> +			ops = dev->netdev_ops;
> +		}
> +	}
> +
> +	if (!br_dev && !dev) {
> +		NL_SET_ERR_MSG(extack, "Unable to find device");^^
I think the error could be improved, if I'm not missing something this case
can happen only if ndm_ifindex = 0 and NDA_MASTER is missing which means
no device was specified in the request, the other cases where the respective
devices are not found are handled above. Perhaps something like
"No master or port device specified".

> +		return -ENODEV;
> +	}
> +
> +	if (!ops || !ops->ndo_fdb_get) {
> +		NL_SET_ERR_MSG(extack, "fdb get operation not supported by device");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	skb = nlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (!skb)
> +		return -ENOBUFS;
> +
> +	if (br_dev)
> +		err = ops->ndo_fdb_get(skb, tb, br_dev, addr, vid,
> +				       NETLINK_CB(in_skb).portid,
> +				       nlh->nlmsg_seq, extack);
> +	else
> +		err = ops->ndo_fdb_get(skb, tb, dev, addr, vid,
> +				       NETLINK_CB(in_skb).portid,
> +				       nlh->nlmsg_seq, extack);
> +	if (err)
> +		goto out;
> +
> +	return rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
> +out:
> +	kfree_skb(skb);
> +	return err;
> +}
> +
>  static int brport_nla_put_flag(struct sk_buff *skb, u32 flags, u32 mask,
>  			       unsigned int attrnum, unsigned int flag)
>  {
> @@ -5081,7 +5186,7 @@ void __init rtnetlink_init(void)
>  
>  	rtnl_register(PF_BRIDGE, RTM_NEWNEIGH, rtnl_fdb_add, NULL, 0);
>  	rtnl_register(PF_BRIDGE, RTM_DELNEIGH, rtnl_fdb_del, NULL, 0);
> -	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, NULL, rtnl_fdb_dump, 0);
> +	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
>  
>  	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
>  	rtnl_register(PF_BRIDGE, RTM_DELLINK, rtnl_bridge_dellink, NULL, 0);
> 

Re: [PATCH net-next 2/4] bridge: support for ndo_fdb_get

[Nikolay Aleksandrov]

On 14/12/2018 19:43, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> This patch implements ndo_fdb_get for the bridge
> fdb.
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  net/bridge/br_device.c  |  1 +
>  net/bridge/br_fdb.c     | 26 ++++++++++++++++++++++++++
>  net/bridge/br_private.h |  3 +++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 9f41a5d..013323b 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -403,6 +403,7 @@ static const struct net_device_ops br_netdev_ops = {
>  	.ndo_fdb_add		 = br_fdb_add,
>  	.ndo_fdb_del		 = br_fdb_delete,
>  	.ndo_fdb_dump		 = br_fdb_dump,
> +	.ndo_fdb_get		 = br_fdb_get,
>  	.ndo_bridge_getlink	 = br_getlink,
>  	.ndo_bridge_setlink	 = br_setlink,
>  	.ndo_bridge_dellink	 = br_dellink,
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 38b1d0d..fafff6c 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -773,6 +773,32 @@ int br_fdb_dump(struct sk_buff *skb,
>  	return err;
>  }
>  
> +int br_fdb_get(struct sk_buff *skb,
> +	       struct nlattr *tb[],
> +	       struct net_device *dev,
> +	       const unsigned char *addr,
> +	       u16 vid, u32 portid, u32 seq,
> +	       struct netlink_ext_ack *extack)
> +{
> +	struct net_bridge *br = netdev_priv(dev);
> +	struct net_bridge_fdb_entry *f;
> +	int err = 0;
> +
> +	rcu_read_lock();
> +	f = br_fdb_find_rcu(br, addr, vid);
> +	if (!f) {
> +		NL_SET_ERR_MSG(extack, "fdb entry not found");
> +		err = -ENOENT;
> +		goto errout;
> +	}
> +
> +	err = fdb_fill_info(skb, br, f, portid, seq,
> +			    RTM_NEWNEIGH, 0);
> +errout:
> +	rcu_read_unlock();
> +	return err;
> +}
> +
>  /* Update (create or replace) forwarding database entry */
>  static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
>  			 const u8 *addr, u16 state, u16 flags, u16 vid,
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index ff3dfb2..d240b3e 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -575,6 +575,9 @@ int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
>  	       const unsigned char *addr, u16 vid, u16 nlh_flags);
>  int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
>  		struct net_device *dev, struct net_device *fdev, int *idx);
> +int br_fdb_get(struct sk_buff *skb, struct nlattr *tb[], struct net_device *dev,
> +	       const unsigned char *addr, u16 vid, u32 portid, u32 seq,
> +	       struct netlink_ext_ack *extack);
>  int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
>  void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
>  int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> 

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Re: [PATCH net-next 0/4] rtnl fdb get

[David Miller]

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Fri, 14 Dec 2018 09:43:17 -0800

> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> This series adds support for rtnl fdb get similar to
> route get.

I like seeing testcases :-)

Please address David's feedback (I think patch #3's extack message needs
the capital letter treatment David mentioned as well), and respin this
series.

Thanks!

Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[Jakub Kicinski]

On Fri, 14 Dec 2018 10:09:07 -0800, Roopa Prabhu wrote:
> On Fri, Dec 14, 2018 at 9:55 AM David Ahern <dsa@cumulusnetworks.com> wrote:
> > On 12/14/18 10:43 AM, Roopa Prabhu wrote:  
> > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > > index f8bdb8ad..fcea76b 100644
> > > --- a/net/core/rtnetlink.c
> > > +++ b/net/core/rtnetlink.c
> > > @@ -4021,6 +4021,111 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
> > >       return skb->len;
> > >  }
> > >
> > > +static int rtnl_fdb_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> > > +                     struct netlink_ext_ack *extack)
> > > +{
> > > +     const struct net_device_ops *ops = NULL;
> > > +     struct net *net = sock_net(in_skb->sk);
> > > +     struct net_device *dev = NULL, *br_dev = NULL;
> > > +     struct nlattr *tb[NDA_MAX + 1];
> > > +     struct sk_buff *skb;
> > > +     struct ndmsg *ndm;
> > > +     int br_idx = 0;
> > > +     u8 *addr;
> > > +     u16 vid;
> > > +     int err;
> > > +
> > > +     err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL, extack);  
> >
> > New stuff should go in with strict checking, so nlmsg_parse_strict and a
> > check for any unsupported attributes (NDA) or ndm entries in the request.  
> 
> makes sense.

Oh, so we'd use the STRICT checking in doit for the first time?  I
better send that rename patch then..

> > Also, please add an nla_policy for easier NDA attributes ...  
> 
> yes agreed. NDA attrs have not had a policy.
> maybe an incremental patch ?, NDA_* attributes are used in bridge
> vxlan and neighbour code, need a common place to put the policy.
> (suggestions ?)

FWIW:

--->8-----

net: netlink: add helper to retrieve NETLINK_F_STRICT_CHK

Dumps can read state of the NETLINK_F_STRICT_CHK flag from
a field in the callback structure.  For non-dump GET requests
we need a way to access the state of that flag from a socket.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/linux/netlink.h  | 1 +
 net/netlink/af_netlink.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 4da90a6ab536..36bf43816e1a 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -117,6 +117,7 @@ extern void __netlink_clear_multicast_users(struct sock *sk, unsigned int group)
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 			const struct netlink_ext_ack *extack);
 extern int netlink_has_listeners(struct sock *sk, unsigned int group);
+extern bool netlink_strict_get_check(struct sk_buff *skb);
 
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 portid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 portid,
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 3c023d6120f6..8fa35df94c07 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1371,6 +1371,14 @@ int netlink_has_listeners(struct sock *sk, unsigned int group)
 }
 EXPORT_SYMBOL_GPL(netlink_has_listeners);
 
+bool netlink_strict_get_check(struct sk_buff *skb)
+{
+	const struct netlink_sock *nlk = nlk_sk(NETLINK_CB(skb).sk);
+
+	return nlk->flags & NETLINK_F_STRICT_CHK;
+}
+EXPORT_SYMBOL_GPL(netlink_strict_get_check);
+
 static int netlink_broadcast_deliver(struct sock *sk, struct sk_buff *skb)
 {
 	struct netlink_sock *nlk = nlk_sk(sk);
-- 
2.19.2

Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[David Ahern]

On 12/14/18 12:37 PM, Jakub Kicinski wrote:
> Oh, so we'd use the STRICT checking in doit for the first time?  I
> better send that rename patch then..

IMHO, no. The flag is for older userspace that could be sending junk in
the request. All new code should do strict checking without the flag set
to ensure only proper requests are handled.

Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[Jakub Kicinski]

On Fri, 14 Dec 2018 12:42:21 -0700, David Ahern wrote:
> On 12/14/18 12:37 PM, Jakub Kicinski wrote:
> > Oh, so we'd use the STRICT checking in doit for the first time?  I
> > better send that rename patch then..  
> 
> IMHO, no. The flag is for older userspace that could be sending junk in
> the request. All new code should do strict checking without the flag set
> to ensure only proper requests are handled.

I'm going back and forth on that in my head.  IDK if new user space
shouldn't be able to do a get request on an old kernel which doesn't
understand some of the attributes.  Grey area.. perhaps it needs to be
decided on case by case basis?  For my stats work I think returning too
many stats if what is affectively a filter is not understood may be a
good option.  Perhaps for fdb get it makes more sense to error out.
hmm..

Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[David Ahern]

On 12/14/18 12:54 PM, Jakub Kicinski wrote:
> On Fri, 14 Dec 2018 12:42:21 -0700, David Ahern wrote:
>> On 12/14/18 12:37 PM, Jakub Kicinski wrote:
>>> Oh, so we'd use the STRICT checking in doit for the first time?  I
>>> better send that rename patch then..  
>>
>> IMHO, no. The flag is for older userspace that could be sending junk in
>> the request. All new code should do strict checking without the flag set
>> to ensure only proper requests are handled.
> 
> I'm going back and forth on that in my head.  IDK if new user space
> shouldn't be able to do a get request on an old kernel which doesn't
> understand some of the attributes.  Grey area.. perhaps it needs to be
> decided on case by case basis?  For my stats work I think returning too
> many stats if what is affectively a filter is not understood may be a
> good option.  Perhaps for fdb get it makes more sense to error out.
> hmm..
> 

I am referring to new code as in what Roopa is doing here -- adding a
whole new feature (support for RTM_GETNEIGH for PF_BRIDGE). There is no
support today, so no way it impacts existing userspace.

In cases where there is a handler for the operation, then, yes, the
strict flag is needed for any new kernel side filtering to ensure the
request is parsed properly.

Re: [PATCH net-next 1/4] net: rtnetlink: support for fdb get

[Jakub Kicinski]

On Fri, 14 Dec 2018 12:58:13 -0700, David Ahern wrote:
> On 12/14/18 12:54 PM, Jakub Kicinski wrote:
> > On Fri, 14 Dec 2018 12:42:21 -0700, David Ahern wrote:  
> >> On 12/14/18 12:37 PM, Jakub Kicinski wrote:  
> >>> Oh, so we'd use the STRICT checking in doit for the first time?  I
> >>> better send that rename patch then..    
> >>
> >> IMHO, no. The flag is for older userspace that could be sending junk in
> >> the request. All new code should do strict checking without the flag set
> >> to ensure only proper requests are handled.  
> > 
> > I'm going back and forth on that in my head.  IDK if new user space
> > shouldn't be able to do a get request on an old kernel which doesn't
> > understand some of the attributes.  Grey area.. perhaps it needs to be
> > decided on case by case basis?  For my stats work I think returning too
> > many stats if what is affectively a filter is not understood may be a
> > good option.  Perhaps for fdb get it makes more sense to error out.
> > hmm..
> 
> I am referring to new code as in what Roopa is doing here -- adding a
> whole new feature (support for RTM_GETNEIGH for PF_BRIDGE). There is no
> support today, so no way it impacts existing userspace.
> 
> In cases where there is a handler for the operation, then, yes, the
> strict flag is needed for any new kernel side filtering to ensure the
> request is parsed properly.

Ack.  So for those new handlers we would never allow the behaviour of
ignoring unknown attributes?  Perhaps I'm over-thinking this, but maybe
we should then just require the STRICT flag on the socket, and if not
set return -EINVAL?  Slightly more consistent behaviour, and it gives
us a clean way out if someone has a strong use case for ignoring the
attributes.  

Just spit balling here, I'm happy either way.