All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v4 00/12] net: bridge: add flush filtering support
@ 2022-04-13 10:51 ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Hi,
This patch-set adds support to specify filtering conditions for a bulk
delete (flush) operation. This version uses a new nlmsghdr delete flag
called NLM_F_BULK in combination with a new ndo_fdb_del_bulk op which is
used to signal that the driver supports bulk deletes (that avoids
pushing common mac address checks to ndo_fdb_del implementations and
also has a different prototype and parsed attribute expectations, more
info in patch 03). The new delete flag can be used for any RTM_DEL*
type, implementations just need to be careful with older kernels which
are doing non-strict attribute parses. A new rtnl flag
(RTNL_FLAG_BULK_DEL_SUPPORTED) is used to show that the delete supports
NLM_F_BULK. A proper error is returned if bulk delete is not supported.
For old kernels I use the fact that mac address attribute (lladdr) is
mandatory in the classic fdb del case, but it's not allowed if bulk
deleting so older kernels will error out.

Patch 01 and 02 are minor rtnetlink cleanups to make the code easier to
read. They remove hardcoded values and use names instead. Patch 03 uses
BIT() for rtnl flags.
Patch 04 adds the new NLM_F_BULK delete request modifier, patch 05 adds
the new bulk delete flag and checks for it if the delete requests have
NLM_F_BULK set, it also warns if rtnl register is called with a non-delete
kind and the bulk delete flag is set.
Patch 06 adds the new ndo_fdb_del_bulk call. Patch 07 adds NLM_F_BULK
support to rtnl_fdb_del, on such request strict parsing is used only for
the supported attributes, and if the ndo is implemented it's called, the
NTF_SELF/MASTER rules are the same as for the standard rtnl_fdb_del.
Patch 08 implements bridge-specific minimal ndo_fdb_del_bulk call which
uses the current br_fdb_flush to delete all entries. Patch 09 adds
filtering support to the new bridge flush op which supports target
ifindex (port or bridge), vlan id and flags/state mask. Patch 10 adds
ndm state and flags mask attributes which will be used for filtering.
Patch 11 converts ndm state/flags and their masks to bridge-private flags
and fills them in the filter descriptor for matching. Finally patch 12
fills in the target ifindex (after validating it) and vlan id (already
validated by rtnl_fdb_flush) for matching. Flush filtering is needed
because user-space applications need a quick way to delete only a
specific set of entries, e.g. mlag implementations need a way to flush only
dynamic entries excluding externally learned ones or only externally
learned ones without static entries etc. Also apps usually want to target
only a specific vlan or port/vlan combination. The current 2 flush
operations (per port and bridge-wide) are not extensible and cannot
provide such filtering.

I decided against embedding new attrs into the old flush attributes for
multiple reasons - proper error handling on unsupported attributes,
older kernels silently flushing all, need for a second mechanism to
signal that the attribute should be parsed (e.g. using boolopts),
special treatment for permanent entries.

Examples:
$ bridge fdb flush dev bridge vlan 100 static
< flush all static entries on vlan 100 >
$ bridge fdb flush dev bridge vlan 1 dynamic
< flush all dynamic entries on vlan 1 >
$ bridge fdb flush dev bridge port ens16 vlan 1 dynamic
< flush all dynamic entries on port ens16 and vlan 1 >
$ bridge fdb flush dev ens16 vlan 1 dynamic master
< as above: flush all dynamic entries on port ens16 and vlan 1 >
$ bridge fdb flush dev bridge nooffloaded nopermanent self
< flush all non-offloaded and non-permanent entries >
$ bridge fdb flush dev bridge static noextern_learn
< flush all static entries which are not externally learned >
$ bridge fdb flush dev bridge permanent
< flush all permanent entries >
$ bridge fdb flush dev bridge port bridge permanent
< flush all permanent entries pointing to the bridge itself >

Example of a flush call with unsupported netlink attribute (NDA_DST):
$ bridge fdb flush dev bridge vlan 100 dynamic dst
Error: Unsupported attribute.

Example of a flush call on an older kernel:
$ bridge fdb flush dev bridge dynamic
Error: invalid address.

Example of calling PF_UNSPEC RTM_DELNEIGH which doesn't support bulk delete
with NLM_F_BULK set (ip neigh is changed to add the flag):
$ ip n del 192.168.122.5 lladdr 00:11:22:33:44:55 dev ens3
Error: Bulk delete is not supported.

Note that all flags have their negated version (static vs nostatic etc)
and there are some tricky cases to handle like "static" which in flag
terms means fdbs that have NUD_NOARP but *not* NUD_PERMANENT, so the
mask matches on both but we need only NUD_NOARP to be set. That's
because permanent entries have both set so we can't just match on
NUD_NOARP. Also note that this flush operation doesn't treat permanent
entries in a special way (fdb_delete vs fdb_delete_local), it will
delete them regardless if any port is using them. We can extend the api
with a flag to do that if needed in the future.

Patch-sets (in order):
 - Initial bulk del infra and fdb flush filtering (this set)
 - iproute2 support
 - selftests

v4: Add and check for rtnl del bulk supported flag when using
    NLM_F_BULK (new patch 05), patches 01 - 03 are also new minor cleanups
    to remove use of raw values and make code easier to read, don't
    rename br_fdb_flush in patch 08, set port ifindex as flush target if
    NDA_IFINDEX is missing and flush was called with port netdev and
    NTF_MASTER (patch 12).

v3: Add NLM_F_BULK delete modifier and ndo_fdb_del_bulk callback,
    patches 01 - 03 and 06 are new. Patch 04 is changed to implement
    bulk_del instead of flush, patches 05, 07 and 08 are adjusted to
    use NDA_ attributes

Thanks,
 Nik

Nikolay Aleksandrov (12):
  net: rtnetlink: add msg kind names
  net: rtnetlink: add helper to extract msg type's kind
  net: rtnetlink: use BIT for flag values
  net: netlink: add NLM_F_BULK delete request modifier
  net: rtnetlink: add bulk delete support flag
  net: add ndo_fdb_del_bulk
  net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
  net: bridge: fdb: add ndo_fdb_del_bulk
  net: bridge: fdb: add support for fine-grained flushing
  net: rtnetlink: add ndm flags and state mask attributes
  net: bridge: fdb: add support for flush filtering based on ndm flags
    and state
  net: bridge: fdb: add support for flush filtering based on ifindex and
    vlan

 include/linux/netdevice.h      |   9 ++
 include/net/rtnetlink.h        |  16 +++-
 include/uapi/linux/neighbour.h |   2 +
 include/uapi/linux/netlink.h   |   1 +
 net/bridge/br_device.c         |   1 +
 net/bridge/br_fdb.c            | 157 +++++++++++++++++++++++++++++++--
 net/bridge/br_netlink.c        |   9 +-
 net/bridge/br_private.h        |  18 +++-
 net/bridge/br_sysfs_br.c       |   6 +-
 net/core/rtnetlink.c           |  85 +++++++++++++-----
 10 files changed, 269 insertions(+), 35 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 00/12] net: bridge: add flush filtering support
@ 2022-04-13 10:51 ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Hi,
This patch-set adds support to specify filtering conditions for a bulk
delete (flush) operation. This version uses a new nlmsghdr delete flag
called NLM_F_BULK in combination with a new ndo_fdb_del_bulk op which is
used to signal that the driver supports bulk deletes (that avoids
pushing common mac address checks to ndo_fdb_del implementations and
also has a different prototype and parsed attribute expectations, more
info in patch 03). The new delete flag can be used for any RTM_DEL*
type, implementations just need to be careful with older kernels which
are doing non-strict attribute parses. A new rtnl flag
(RTNL_FLAG_BULK_DEL_SUPPORTED) is used to show that the delete supports
NLM_F_BULK. A proper error is returned if bulk delete is not supported.
For old kernels I use the fact that mac address attribute (lladdr) is
mandatory in the classic fdb del case, but it's not allowed if bulk
deleting so older kernels will error out.

Patch 01 and 02 are minor rtnetlink cleanups to make the code easier to
read. They remove hardcoded values and use names instead. Patch 03 uses
BIT() for rtnl flags.
Patch 04 adds the new NLM_F_BULK delete request modifier, patch 05 adds
the new bulk delete flag and checks for it if the delete requests have
NLM_F_BULK set, it also warns if rtnl register is called with a non-delete
kind and the bulk delete flag is set.
Patch 06 adds the new ndo_fdb_del_bulk call. Patch 07 adds NLM_F_BULK
support to rtnl_fdb_del, on such request strict parsing is used only for
the supported attributes, and if the ndo is implemented it's called, the
NTF_SELF/MASTER rules are the same as for the standard rtnl_fdb_del.
Patch 08 implements bridge-specific minimal ndo_fdb_del_bulk call which
uses the current br_fdb_flush to delete all entries. Patch 09 adds
filtering support to the new bridge flush op which supports target
ifindex (port or bridge), vlan id and flags/state mask. Patch 10 adds
ndm state and flags mask attributes which will be used for filtering.
Patch 11 converts ndm state/flags and their masks to bridge-private flags
and fills them in the filter descriptor for matching. Finally patch 12
fills in the target ifindex (after validating it) and vlan id (already
validated by rtnl_fdb_flush) for matching. Flush filtering is needed
because user-space applications need a quick way to delete only a
specific set of entries, e.g. mlag implementations need a way to flush only
dynamic entries excluding externally learned ones or only externally
learned ones without static entries etc. Also apps usually want to target
only a specific vlan or port/vlan combination. The current 2 flush
operations (per port and bridge-wide) are not extensible and cannot
provide such filtering.

I decided against embedding new attrs into the old flush attributes for
multiple reasons - proper error handling on unsupported attributes,
older kernels silently flushing all, need for a second mechanism to
signal that the attribute should be parsed (e.g. using boolopts),
special treatment for permanent entries.

Examples:
$ bridge fdb flush dev bridge vlan 100 static
< flush all static entries on vlan 100 >
$ bridge fdb flush dev bridge vlan 1 dynamic
< flush all dynamic entries on vlan 1 >
$ bridge fdb flush dev bridge port ens16 vlan 1 dynamic
< flush all dynamic entries on port ens16 and vlan 1 >
$ bridge fdb flush dev ens16 vlan 1 dynamic master
< as above: flush all dynamic entries on port ens16 and vlan 1 >
$ bridge fdb flush dev bridge nooffloaded nopermanent self
< flush all non-offloaded and non-permanent entries >
$ bridge fdb flush dev bridge static noextern_learn
< flush all static entries which are not externally learned >
$ bridge fdb flush dev bridge permanent
< flush all permanent entries >
$ bridge fdb flush dev bridge port bridge permanent
< flush all permanent entries pointing to the bridge itself >

Example of a flush call with unsupported netlink attribute (NDA_DST):
$ bridge fdb flush dev bridge vlan 100 dynamic dst
Error: Unsupported attribute.

Example of a flush call on an older kernel:
$ bridge fdb flush dev bridge dynamic
Error: invalid address.

Example of calling PF_UNSPEC RTM_DELNEIGH which doesn't support bulk delete
with NLM_F_BULK set (ip neigh is changed to add the flag):
$ ip n del 192.168.122.5 lladdr 00:11:22:33:44:55 dev ens3
Error: Bulk delete is not supported.

Note that all flags have their negated version (static vs nostatic etc)
and there are some tricky cases to handle like "static" which in flag
terms means fdbs that have NUD_NOARP but *not* NUD_PERMANENT, so the
mask matches on both but we need only NUD_NOARP to be set. That's
because permanent entries have both set so we can't just match on
NUD_NOARP. Also note that this flush operation doesn't treat permanent
entries in a special way (fdb_delete vs fdb_delete_local), it will
delete them regardless if any port is using them. We can extend the api
with a flag to do that if needed in the future.

Patch-sets (in order):
 - Initial bulk del infra and fdb flush filtering (this set)
 - iproute2 support
 - selftests

v4: Add and check for rtnl del bulk supported flag when using
    NLM_F_BULK (new patch 05), patches 01 - 03 are also new minor cleanups
    to remove use of raw values and make code easier to read, don't
    rename br_fdb_flush in patch 08, set port ifindex as flush target if
    NDA_IFINDEX is missing and flush was called with port netdev and
    NTF_MASTER (patch 12).

v3: Add NLM_F_BULK delete modifier and ndo_fdb_del_bulk callback,
    patches 01 - 03 and 06 are new. Patch 04 is changed to implement
    bulk_del instead of flush, patches 05, 07 and 08 are adjusted to
    use NDA_ attributes

Thanks,
 Nik

Nikolay Aleksandrov (12):
  net: rtnetlink: add msg kind names
  net: rtnetlink: add helper to extract msg type's kind
  net: rtnetlink: use BIT for flag values
  net: netlink: add NLM_F_BULK delete request modifier
  net: rtnetlink: add bulk delete support flag
  net: add ndo_fdb_del_bulk
  net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
  net: bridge: fdb: add ndo_fdb_del_bulk
  net: bridge: fdb: add support for fine-grained flushing
  net: rtnetlink: add ndm flags and state mask attributes
  net: bridge: fdb: add support for flush filtering based on ndm flags
    and state
  net: bridge: fdb: add support for flush filtering based on ifindex and
    vlan

 include/linux/netdevice.h      |   9 ++
 include/net/rtnetlink.h        |  16 +++-
 include/uapi/linux/neighbour.h |   2 +
 include/uapi/linux/netlink.h   |   1 +
 net/bridge/br_device.c         |   1 +
 net/bridge/br_fdb.c            | 157 +++++++++++++++++++++++++++++++--
 net/bridge/br_netlink.c        |   9 +-
 net/bridge/br_private.h        |  18 +++-
 net/bridge/br_sysfs_br.c       |   6 +-
 net/core/rtnetlink.c           |  85 +++++++++++++-----
 10 files changed, 269 insertions(+), 35 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 01/12] net: rtnetlink: add msg kind names
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add rtnl kind names instead of using raw values. We'll need to
check for DEL kind later to validate bulk flag support.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: new patch

 include/net/rtnetlink.h | 7 +++++++
 net/core/rtnetlink.c    | 6 +++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 9f48733bfd21..78712b51f3da 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -13,6 +13,13 @@ enum rtnl_link_flags {
 	RTNL_FLAG_DOIT_UNLOCKED = 1,
 };
 
+enum rtnl_kinds {
+	RTNL_KIND_NEW,
+	RTNL_KIND_DEL,
+	RTNL_KIND_GET,
+	RTNL_KIND_SET
+};
+
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 int rtnl_register_module(struct module *owner, int protocol, int msgtype,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 4041b3e2e8ec..2c36c9dc9b62 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -5928,11 +5928,11 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 {
 	struct net *net = sock_net(skb->sk);
 	struct rtnl_link *link;
+	enum rtnl_kinds kind;
 	struct module *owner;
 	int err = -EOPNOTSUPP;
 	rtnl_doit_func doit;
 	unsigned int flags;
-	int kind;
 	int family;
 	int type;
 
@@ -5949,11 +5949,11 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
 	kind = type&3;
 
-	if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN))
+	if (kind != RTNL_KIND_GET && !netlink_net_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
 	rcu_read_lock();
-	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
+	if (kind == RTNL_KIND_GET && (nlh->nlmsg_flags & NLM_F_DUMP)) {
 		struct sock *rtnl;
 		rtnl_dumpit_func dumpit;
 		u32 min_dump_alloc = 0;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 01/12] net: rtnetlink: add msg kind names
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add rtnl kind names instead of using raw values. We'll need to
check for DEL kind later to validate bulk flag support.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: new patch

 include/net/rtnetlink.h | 7 +++++++
 net/core/rtnetlink.c    | 6 +++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 9f48733bfd21..78712b51f3da 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -13,6 +13,13 @@ enum rtnl_link_flags {
 	RTNL_FLAG_DOIT_UNLOCKED = 1,
 };
 
+enum rtnl_kinds {
+	RTNL_KIND_NEW,
+	RTNL_KIND_DEL,
+	RTNL_KIND_GET,
+	RTNL_KIND_SET
+};
+
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
 int rtnl_register_module(struct module *owner, int protocol, int msgtype,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 4041b3e2e8ec..2c36c9dc9b62 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -5928,11 +5928,11 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 {
 	struct net *net = sock_net(skb->sk);
 	struct rtnl_link *link;
+	enum rtnl_kinds kind;
 	struct module *owner;
 	int err = -EOPNOTSUPP;
 	rtnl_doit_func doit;
 	unsigned int flags;
-	int kind;
 	int family;
 	int type;
 
@@ -5949,11 +5949,11 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
 	kind = type&3;
 
-	if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN))
+	if (kind != RTNL_KIND_GET && !netlink_net_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
 	rcu_read_lock();
-	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
+	if (kind == RTNL_KIND_GET && (nlh->nlmsg_flags & NLM_F_DUMP)) {
 		struct sock *rtnl;
 		rtnl_dumpit_func dumpit;
 		u32 min_dump_alloc = 0;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 02/12] net: rtnetlink: add helper to extract msg type's kind
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add a helper which extracts the msg type's kind using the kind mask (0x3).

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: new patch

 include/net/rtnetlink.h | 6 ++++++
 net/core/rtnetlink.c    | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 78712b51f3da..c51c5ff7f7e2 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -19,6 +19,12 @@ enum rtnl_kinds {
 	RTNL_KIND_GET,
 	RTNL_KIND_SET
 };
+#define RTNL_KIND_MASK 0x3
+
+static inline enum rtnl_kinds rtnl_msgtype_kind(int msgtype)
+{
+	return msgtype & RTNL_KIND_MASK;
+}
 
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 2c36c9dc9b62..beda4a7da062 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -5947,7 +5947,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return 0;
 
 	family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
-	kind = type&3;
+	kind = rtnl_msgtype_kind(type);
 
 	if (kind != RTNL_KIND_GET && !netlink_net_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 02/12] net: rtnetlink: add helper to extract msg type's kind
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add a helper which extracts the msg type's kind using the kind mask (0x3).

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: new patch

 include/net/rtnetlink.h | 6 ++++++
 net/core/rtnetlink.c    | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 78712b51f3da..c51c5ff7f7e2 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -19,6 +19,12 @@ enum rtnl_kinds {
 	RTNL_KIND_GET,
 	RTNL_KIND_SET
 };
+#define RTNL_KIND_MASK 0x3
+
+static inline enum rtnl_kinds rtnl_msgtype_kind(int msgtype)
+{
+	return msgtype & RTNL_KIND_MASK;
+}
 
 void rtnl_register(int protocol, int msgtype,
 		   rtnl_doit_func, rtnl_dumpit_func, unsigned int flags);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 2c36c9dc9b62..beda4a7da062 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -5947,7 +5947,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return 0;
 
 	family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
-	kind = type&3;
+	kind = rtnl_msgtype_kind(type);
 
 	if (kind != RTNL_KIND_GET && !netlink_net_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 03/12] net: rtnetlink: use BIT for flag values
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Use BIT to define flag values.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: new patch

 include/net/rtnetlink.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index c51c5ff7f7e2..0bf622409aaa 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -10,7 +10,7 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *,
 typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
 
 enum rtnl_link_flags {
-	RTNL_FLAG_DOIT_UNLOCKED = 1,
+	RTNL_FLAG_DOIT_UNLOCKED = BIT(0),
 };
 
 enum rtnl_kinds {
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 03/12] net: rtnetlink: use BIT for flag values
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Use BIT to define flag values.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: new patch

 include/net/rtnetlink.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index c51c5ff7f7e2..0bf622409aaa 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -10,7 +10,7 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *,
 typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
 
 enum rtnl_link_flags {
-	RTNL_FLAG_DOIT_UNLOCKED = 1,
+	RTNL_FLAG_DOIT_UNLOCKED = BIT(0),
 };
 
 enum rtnl_kinds {
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add a new delete request modifier called NLM_F_BULK which, when
supported, would cause the request to delete multiple objects. The flag
is a convenient way to signal that a multiple delete operation is
requested which can be gradually added to different delete requests. In
order to make sure older kernels will error out if the operation is not
supported instead of doing something unintended we have to break a
required condition when implementing support for this flag, f.e. for
neighbors we will omit the mandatory mac address attribute.
Initially it will be used to add flush with filtering support for bridge
fdbs, but it also opens the door to add similar support to others.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 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 4c0cde075c27..855dffb4c1c3 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -72,6 +72,7 @@ struct nlmsghdr {
 
 /* Modifiers to DELETE request */
 #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
+#define NLM_F_BULK	0x200	/* Delete multiple objects	*/
 
 /* Flags for ACK message */
 #define NLM_F_CAPPED	0x100	/* request was capped */
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add a new delete request modifier called NLM_F_BULK which, when
supported, would cause the request to delete multiple objects. The flag
is a convenient way to signal that a multiple delete operation is
requested which can be gradually added to different delete requests. In
order to make sure older kernels will error out if the operation is not
supported instead of doing something unintended we have to break a
required condition when implementing support for this flag, f.e. for
neighbors we will omit the mandatory mac address attribute.
Initially it will be used to add flush with filtering support for bridge
fdbs, but it also opens the door to add similar support to others.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 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 4c0cde075c27..855dffb4c1c3 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -72,6 +72,7 @@ struct nlmsghdr {
 
 /* Modifiers to DELETE request */
 #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
+#define NLM_F_BULK	0x200	/* Delete multiple objects	*/
 
 /* Flags for ACK message */
 #define NLM_F_CAPPED	0x100	/* request was capped */
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to
verify that the delete operation allows bulk object deletion. Also emit
a warning if anyone tries to set it for non-delete kind.

Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: new patch

 include/net/rtnetlink.h | 3 ++-
 net/core/rtnetlink.c    | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 0bf622409aaa..bf8bb3357825 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *,
 typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
 
 enum rtnl_link_flags {
-	RTNL_FLAG_DOIT_UNLOCKED = BIT(0),
+	RTNL_FLAG_DOIT_UNLOCKED		= BIT(0),
+	RTNL_FLAG_BULK_DEL_SUPPORTED	= BIT(1),
 };
 
 enum rtnl_kinds {
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index beda4a7da062..63c7df52a667 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner,
 	if (dumpit)
 		link->dumpit = dumpit;
 
+	WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL &&
+		(flags & RTNL_FLAG_BULK_DEL_SUPPORTED));
 	link->flags |= flags;
 
 	/* publish protocol:msgtype */
@@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	flags = link->flags;
+	if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) &&
+	    !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) {
+		NL_SET_ERR_MSG(extack, "Bulk delete is not supported");
+		goto err_unlock;
+	}
+
 	if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
 		doit = link->doit;
 		rcu_read_unlock();
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to
verify that the delete operation allows bulk object deletion. Also emit
a warning if anyone tries to set it for non-delete kind.

Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: new patch

 include/net/rtnetlink.h | 3 ++-
 net/core/rtnetlink.c    | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 0bf622409aaa..bf8bb3357825 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *,
 typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
 
 enum rtnl_link_flags {
-	RTNL_FLAG_DOIT_UNLOCKED = BIT(0),
+	RTNL_FLAG_DOIT_UNLOCKED		= BIT(0),
+	RTNL_FLAG_BULK_DEL_SUPPORTED	= BIT(1),
 };
 
 enum rtnl_kinds {
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index beda4a7da062..63c7df52a667 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner,
 	if (dumpit)
 		link->dumpit = dumpit;
 
+	WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL &&
+		(flags & RTNL_FLAG_BULK_DEL_SUPPORTED));
 	link->flags |= flags;
 
 	/* publish protocol:msgtype */
@@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	flags = link->flags;
+	if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) &&
+	    !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) {
+		NL_SET_ERR_MSG(extack, "Bulk delete is not supported");
+		goto err_unlock;
+	}
+
 	if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
 		doit = link->doit;
 		rcu_read_unlock();
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 06/12] net: add ndo_fdb_del_bulk
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add a new netdev op called ndo_fdb_del_bulk, it will be later used for
driver-specific bulk delete implementation dispatched from rtnetlink. The
first user will be the bridge, we need it to signal to rtnetlink from
the driver that we support bulk delete operation (NLM_F_BULK).

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 include/linux/netdevice.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 28ea4f8269d4..a602f29365b0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1260,6 +1260,10 @@ struct netdev_net_notifier {
  *		      struct net_device *dev,
  *		      const unsigned char *addr, u16 vid)
  *	Deletes the FDB entry from dev coresponding to addr.
+ * int (*ndo_fdb_del_bulk)(struct ndmsg *ndm, struct nlattr *tb[],
+ *			   struct net_device *dev,
+ *			   u16 vid,
+ *			   struct netlink_ext_ack *extack);
  * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
  *		       struct net_device *dev, struct net_device *filter_dev,
  *		       int *idx)
@@ -1510,6 +1514,11 @@ struct net_device_ops {
 					       struct net_device *dev,
 					       const unsigned char *addr,
 					       u16 vid);
+	int			(*ndo_fdb_del_bulk)(struct ndmsg *ndm,
+						    struct nlattr *tb[],
+						    struct net_device *dev,
+						    u16 vid,
+						    struct netlink_ext_ack *extack);
 	int			(*ndo_fdb_dump)(struct sk_buff *skb,
 						struct netlink_callback *cb,
 						struct net_device *dev,
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 06/12] net: add ndo_fdb_del_bulk
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add a new netdev op called ndo_fdb_del_bulk, it will be later used for
driver-specific bulk delete implementation dispatched from rtnetlink. The
first user will be the bridge, we need it to signal to rtnetlink from
the driver that we support bulk delete operation (NLM_F_BULK).

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 include/linux/netdevice.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 28ea4f8269d4..a602f29365b0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1260,6 +1260,10 @@ struct netdev_net_notifier {
  *		      struct net_device *dev,
  *		      const unsigned char *addr, u16 vid)
  *	Deletes the FDB entry from dev coresponding to addr.
+ * int (*ndo_fdb_del_bulk)(struct ndmsg *ndm, struct nlattr *tb[],
+ *			   struct net_device *dev,
+ *			   u16 vid,
+ *			   struct netlink_ext_ack *extack);
  * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
  *		       struct net_device *dev, struct net_device *filter_dev,
  *		       int *idx)
@@ -1510,6 +1514,11 @@ struct net_device_ops {
 					       struct net_device *dev,
 					       const unsigned char *addr,
 					       u16 vid);
+	int			(*ndo_fdb_del_bulk)(struct ndmsg *ndm,
+						    struct nlattr *tb[],
+						    struct net_device *dev,
+						    u16 vid,
+						    struct netlink_ext_ack *extack);
 	int			(*ndo_fdb_dump)(struct sk_buff *skb,
 						struct netlink_callback *cb,
 						struct net_device *dev,
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

When NLM_F_BULK is specified in a fdb del message we need to handle it
differently. First since this is a new call we can strictly validate the
passed attributes, at first only ifindex and vlan are allowed as these
will be the initially supported filter attributes, any other attribute
is rejected. The mac address is no longer mandatory, but we use it
to error out in older kernels because it cannot be specified with bulk
request (the attribute is not allowed) and then we have to dispatch
the call to ndo_fdb_del_bulk if the device supports it. The del bulk
callback can do further validation of the attributes if necessary.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED

 net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 19 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 63c7df52a667..520d50fcaaea 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
 }
 EXPORT_SYMBOL(ndo_dflt_fdb_del);
 
+static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
+	[NDA_VLAN]	= { .type = NLA_U16 },
+	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
+};
+
 static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 			struct netlink_ext_ack *extack)
 {
+	bool del_bulk = !!(nlh->nlmsg_flags & NLM_F_BULK);
 	struct net *net = sock_net(skb->sk);
+	const struct net_device_ops *ops;
 	struct ndmsg *ndm;
 	struct nlattr *tb[NDA_MAX+1];
 	struct net_device *dev;
-	__u8 *addr;
+	__u8 *addr = NULL;
 	int err;
 	u16 vid;
 
 	if (!netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX, NULL,
-				     extack);
+	if (!del_bulk) {
+		err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX,
+					     NULL, extack);
+	} else {
+		err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX,
+				  fdb_del_bulk_policy, extack);
+	}
 	if (err < 0)
 		return err;
 
@@ -4200,9 +4212,12 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -ENODEV;
 	}
 
-	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
-		NL_SET_ERR_MSG(extack, "invalid address");
-		return -EINVAL;
+	if (!del_bulk) {
+		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]);
 	}
 
 	if (dev->type != ARPHRD_ETHER) {
@@ -4210,8 +4225,6 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 	}
 
-	addr = nla_data(tb[NDA_LLADDR]);
-
 	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
 	if (err)
 		return err;
@@ -4222,10 +4235,16 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
 	    netif_is_bridge_port(dev)) {
 		struct net_device *br_dev = netdev_master_upper_dev_get(dev);
-		const struct net_device_ops *ops = br_dev->netdev_ops;
 
-		if (ops->ndo_fdb_del)
-			err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
+		ops = br_dev->netdev_ops;
+		if (!del_bulk) {
+			if (ops->ndo_fdb_del)
+				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
+		} else {
+			if (ops->ndo_fdb_del_bulk)
+				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
+							    extack);
+		}
 
 		if (err)
 			goto out;
@@ -4235,15 +4254,24 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	/* Embedded bridge, macvlan, and any other device support */
 	if (ndm->ndm_flags & NTF_SELF) {
-		if (dev->netdev_ops->ndo_fdb_del)
-			err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr,
-							   vid);
-		else
-			err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
+		ops = dev->netdev_ops;
+		if (!del_bulk) {
+			if (ops->ndo_fdb_del)
+				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
+			else
+				err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
+		} else {
+			/* in case err was cleared by NTF_MASTER call */
+			err = -EOPNOTSUPP;
+			if (ops->ndo_fdb_del_bulk)
+				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
+							    extack);
+		}
 
 		if (!err) {
-			rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
-					ndm->ndm_state);
+			if (!del_bulk)
+				rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
+						ndm->ndm_state);
 			ndm->ndm_flags &= ~NTF_SELF;
 		}
 	}
@@ -6145,7 +6173,8 @@ void __init rtnetlink_init(void)
 	rtnl_register(PF_UNSPEC, RTM_DELLINKPROP, rtnl_dellinkprop, NULL, 0);
 
 	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_DELNEIGH, rtnl_fdb_del, NULL,
+		      RTNL_FLAG_BULK_DEL_SUPPORTED);
 	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
 
 	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

When NLM_F_BULK is specified in a fdb del message we need to handle it
differently. First since this is a new call we can strictly validate the
passed attributes, at first only ifindex and vlan are allowed as these
will be the initially supported filter attributes, any other attribute
is rejected. The mac address is no longer mandatory, but we use it
to error out in older kernels because it cannot be specified with bulk
request (the attribute is not allowed) and then we have to dispatch
the call to ndo_fdb_del_bulk if the device supports it. The del bulk
callback can do further validation of the attributes if necessary.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED

 net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 19 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 63c7df52a667..520d50fcaaea 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
 }
 EXPORT_SYMBOL(ndo_dflt_fdb_del);
 
+static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
+	[NDA_VLAN]	= { .type = NLA_U16 },
+	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
+};
+
 static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 			struct netlink_ext_ack *extack)
 {
+	bool del_bulk = !!(nlh->nlmsg_flags & NLM_F_BULK);
 	struct net *net = sock_net(skb->sk);
+	const struct net_device_ops *ops;
 	struct ndmsg *ndm;
 	struct nlattr *tb[NDA_MAX+1];
 	struct net_device *dev;
-	__u8 *addr;
+	__u8 *addr = NULL;
 	int err;
 	u16 vid;
 
 	if (!netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX, NULL,
-				     extack);
+	if (!del_bulk) {
+		err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX,
+					     NULL, extack);
+	} else {
+		err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX,
+				  fdb_del_bulk_policy, extack);
+	}
 	if (err < 0)
 		return err;
 
@@ -4200,9 +4212,12 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -ENODEV;
 	}
 
-	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
-		NL_SET_ERR_MSG(extack, "invalid address");
-		return -EINVAL;
+	if (!del_bulk) {
+		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]);
 	}
 
 	if (dev->type != ARPHRD_ETHER) {
@@ -4210,8 +4225,6 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 	}
 
-	addr = nla_data(tb[NDA_LLADDR]);
-
 	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
 	if (err)
 		return err;
@@ -4222,10 +4235,16 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
 	    netif_is_bridge_port(dev)) {
 		struct net_device *br_dev = netdev_master_upper_dev_get(dev);
-		const struct net_device_ops *ops = br_dev->netdev_ops;
 
-		if (ops->ndo_fdb_del)
-			err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
+		ops = br_dev->netdev_ops;
+		if (!del_bulk) {
+			if (ops->ndo_fdb_del)
+				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
+		} else {
+			if (ops->ndo_fdb_del_bulk)
+				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
+							    extack);
+		}
 
 		if (err)
 			goto out;
@@ -4235,15 +4254,24 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	/* Embedded bridge, macvlan, and any other device support */
 	if (ndm->ndm_flags & NTF_SELF) {
-		if (dev->netdev_ops->ndo_fdb_del)
-			err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr,
-							   vid);
-		else
-			err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
+		ops = dev->netdev_ops;
+		if (!del_bulk) {
+			if (ops->ndo_fdb_del)
+				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
+			else
+				err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
+		} else {
+			/* in case err was cleared by NTF_MASTER call */
+			err = -EOPNOTSUPP;
+			if (ops->ndo_fdb_del_bulk)
+				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
+							    extack);
+		}
 
 		if (!err) {
-			rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
-					ndm->ndm_state);
+			if (!del_bulk)
+				rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
+						ndm->ndm_state);
 			ndm->ndm_flags &= ~NTF_SELF;
 		}
 	}
@@ -6145,7 +6173,8 @@ void __init rtnetlink_init(void)
 	rtnl_register(PF_UNSPEC, RTM_DELLINKPROP, rtnl_dellinkprop, NULL, 0);
 
 	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_DELNEIGH, rtnl_fdb_del, NULL,
+		      RTNL_FLAG_BULK_DEL_SUPPORTED);
 	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
 
 	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 08/12] net: bridge: fdb: add ndo_fdb_del_bulk
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add a minimal ndo_fdb_del_bulk implementation which flushes all entries.
Support for more fine-grained filtering will be added in the following
patches.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: don't rename br_fdb_flush

 net/bridge/br_device.c  |  1 +
 net/bridge/br_fdb.c     | 23 +++++++++++++++++++++++
 net/bridge/br_private.h |  3 +++
 3 files changed, 27 insertions(+)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 8d6bab244c4a..58a4f70e01e3 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -465,6 +465,7 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_fix_features        = br_fix_features,
 	.ndo_fdb_add		 = br_fdb_add,
 	.ndo_fdb_del		 = br_fdb_delete,
+	.ndo_fdb_del_bulk	 = br_fdb_delete_bulk,
 	.ndo_fdb_dump		 = br_fdb_dump,
 	.ndo_fdb_get		 = br_fdb_get,
 	.ndo_bridge_getlink	 = br_getlink,
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 6ccda68bd473..363985f1a540 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -572,6 +572,29 @@ void br_fdb_flush(struct net_bridge *br)
 	spin_unlock_bh(&br->hash_lock);
 }
 
+int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
+		       struct net_device *dev, u16 vid,
+		       struct netlink_ext_ack *extack)
+{
+	struct net_bridge_port *p = NULL;
+	struct net_bridge *br;
+
+	if (netif_is_bridge_master(dev)) {
+		br = netdev_priv(dev);
+	} else {
+		p = br_port_get_rtnl(dev);
+		if (!p) {
+			NL_SET_ERR_MSG_MOD(extack, "Device is not a bridge port");
+			return -EINVAL;
+		}
+		br = p->br;
+	}
+
+	br_fdb_flush(br);
+
+	return 0;
+}
+
 /* Flush all entries referring to a specific port.
  * if do_all is set also flush static entries
  * if vid is set delete all entries that match the vlan_id
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 6e62af2e07e9..f37d49bf5637 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -781,6 +781,9 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 
 int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 		  struct net_device *dev, const unsigned char *addr, u16 vid);
+int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
+		       struct net_device *dev, u16 vid,
+		       struct netlink_ext_ack *extack);
 int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
 	       const unsigned char *addr, u16 vid, u16 nlh_flags,
 	       struct netlink_ext_ack *extack);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 08/12] net: bridge: fdb: add ndo_fdb_del_bulk
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add a minimal ndo_fdb_del_bulk implementation which flushes all entries.
Support for more fine-grained filtering will be added in the following
patches.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v4: don't rename br_fdb_flush

 net/bridge/br_device.c  |  1 +
 net/bridge/br_fdb.c     | 23 +++++++++++++++++++++++
 net/bridge/br_private.h |  3 +++
 3 files changed, 27 insertions(+)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 8d6bab244c4a..58a4f70e01e3 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -465,6 +465,7 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_fix_features        = br_fix_features,
 	.ndo_fdb_add		 = br_fdb_add,
 	.ndo_fdb_del		 = br_fdb_delete,
+	.ndo_fdb_del_bulk	 = br_fdb_delete_bulk,
 	.ndo_fdb_dump		 = br_fdb_dump,
 	.ndo_fdb_get		 = br_fdb_get,
 	.ndo_bridge_getlink	 = br_getlink,
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 6ccda68bd473..363985f1a540 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -572,6 +572,29 @@ void br_fdb_flush(struct net_bridge *br)
 	spin_unlock_bh(&br->hash_lock);
 }
 
+int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
+		       struct net_device *dev, u16 vid,
+		       struct netlink_ext_ack *extack)
+{
+	struct net_bridge_port *p = NULL;
+	struct net_bridge *br;
+
+	if (netif_is_bridge_master(dev)) {
+		br = netdev_priv(dev);
+	} else {
+		p = br_port_get_rtnl(dev);
+		if (!p) {
+			NL_SET_ERR_MSG_MOD(extack, "Device is not a bridge port");
+			return -EINVAL;
+		}
+		br = p->br;
+	}
+
+	br_fdb_flush(br);
+
+	return 0;
+}
+
 /* Flush all entries referring to a specific port.
  * if do_all is set also flush static entries
  * if vid is set delete all entries that match the vlan_id
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 6e62af2e07e9..f37d49bf5637 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -781,6 +781,9 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 
 int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 		  struct net_device *dev, const unsigned char *addr, u16 vid);
+int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
+		       struct net_device *dev, u16 vid,
+		       struct netlink_ext_ack *extack);
 int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
 	       const unsigned char *addr, u16 vid, u16 nlh_flags,
 	       struct netlink_ext_ack *extack);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 09/12] net: bridge: fdb: add support for fine-grained flushing
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add the ability to specify exactly which fdbs to be flushed. They are
described by a new structure - net_bridge_fdb_flush_desc. Currently it
can match on port/bridge ifindex, vlan id and fdb flags. It is used to
describe the existing dynamic fdb flush operation. Note that this flush
operation doesn't treat permanent entries in a special way (fdb_delete vs
fdb_delete_local), it will delete them regardless if any port is using
them, so currently it can't directly replace deletes which need to handle
that case, although we can extend it later for that too.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: changed the flush matches func for better readability (Ido)

 net/bridge/br_fdb.c      | 41 ++++++++++++++++++++++++++++++++--------
 net/bridge/br_netlink.c  |  9 +++++++--
 net/bridge/br_private.h  | 10 +++++++++-
 net/bridge/br_sysfs_br.c |  6 +++++-
 4 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 363985f1a540..45d02f2264db 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -558,24 +558,49 @@ void br_fdb_cleanup(struct work_struct *work)
 	mod_delayed_work(system_long_wq, &br->gc_work, work_delay);
 }
 
-/* Completely flush all dynamic entries in forwarding database.*/
-void br_fdb_flush(struct net_bridge *br)
+static bool __fdb_flush_matches(const struct net_bridge *br,
+				const struct net_bridge_fdb_entry *f,
+				const struct net_bridge_fdb_flush_desc *desc)
+{
+	const struct net_bridge_port *dst = READ_ONCE(f->dst);
+	int port_ifidx = dst ? dst->dev->ifindex : br->dev->ifindex;
+
+	if (desc->vlan_id && desc->vlan_id != f->key.vlan_id)
+		return false;
+	if (desc->port_ifindex && desc->port_ifindex != port_ifidx)
+		return false;
+	if (desc->flags_mask && (f->flags & desc->flags_mask) != desc->flags)
+		return false;
+
+	return true;
+}
+
+/* Flush forwarding database entries matching the description */
+void br_fdb_flush(struct net_bridge *br,
+		  const struct net_bridge_fdb_flush_desc *desc)
 {
 	struct net_bridge_fdb_entry *f;
-	struct hlist_node *tmp;
 
-	spin_lock_bh(&br->hash_lock);
-	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
-		if (!test_bit(BR_FDB_STATIC, &f->flags))
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
+		if (!__fdb_flush_matches(br, f, desc))
+			continue;
+
+		spin_lock_bh(&br->hash_lock);
+		if (!hlist_unhashed(&f->fdb_node))
 			fdb_delete(br, f, true);
+		spin_unlock_bh(&br->hash_lock);
 	}
-	spin_unlock_bh(&br->hash_lock);
+	rcu_read_unlock();
 }
 
 int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		       struct net_device *dev, u16 vid,
 		       struct netlink_ext_ack *extack)
 {
+	struct net_bridge_fdb_flush_desc desc = {
+		.flags_mask = BR_FDB_STATIC
+	};
 	struct net_bridge_port *p = NULL;
 	struct net_bridge *br;
 
@@ -590,7 +615,7 @@ int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		br = p->br;
 	}
 
-	br_fdb_flush(br);
+	br_fdb_flush(br, &desc);
 
 	return 0;
 }
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 200ad05b296f..bb01776d2d88 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1326,8 +1326,13 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 		br_recalculate_fwd_mask(br);
 	}
 
-	if (data[IFLA_BR_FDB_FLUSH])
-		br_fdb_flush(br);
+	if (data[IFLA_BR_FDB_FLUSH]) {
+		struct net_bridge_fdb_flush_desc desc = {
+			.flags_mask = BR_FDB_STATIC
+		};
+
+		br_fdb_flush(br, &desc);
+	}
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	if (data[IFLA_BR_MCAST_ROUTER]) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f37d49bf5637..4d2a809546fb 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -274,6 +274,13 @@ struct net_bridge_fdb_entry {
 	struct rcu_head			rcu;
 };
 
+struct net_bridge_fdb_flush_desc {
+	unsigned long			flags;
+	unsigned long			flags_mask;
+	int				port_ifindex;
+	u16				vlan_id;
+};
+
 #define MDB_PG_FLAGS_PERMANENT	BIT(0)
 #define MDB_PG_FLAGS_OFFLOAD	BIT(1)
 #define MDB_PG_FLAGS_FAST_LEAVE	BIT(2)
@@ -759,7 +766,8 @@ int br_fdb_init(void);
 void br_fdb_fini(void);
 int br_fdb_hash_init(struct net_bridge *br);
 void br_fdb_hash_fini(struct net_bridge *br);
-void br_fdb_flush(struct net_bridge *br);
+void br_fdb_flush(struct net_bridge *br,
+		  const struct net_bridge_fdb_flush_desc *desc);
 void br_fdb_find_delete_local(struct net_bridge *br,
 			      const struct net_bridge_port *p,
 			      const unsigned char *addr, u16 vid);
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 3f7ca88c2aa3..612e367fff20 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -344,7 +344,11 @@ static DEVICE_ATTR_RW(group_addr);
 static int set_flush(struct net_bridge *br, unsigned long val,
 		     struct netlink_ext_ack *extack)
 {
-	br_fdb_flush(br);
+	struct net_bridge_fdb_flush_desc desc = {
+		.flags_mask = BR_FDB_STATIC
+	};
+
+	br_fdb_flush(br, &desc);
 	return 0;
 }
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 09/12] net: bridge: fdb: add support for fine-grained flushing
@ 2022-04-13 10:51   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:51 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add the ability to specify exactly which fdbs to be flushed. They are
described by a new structure - net_bridge_fdb_flush_desc. Currently it
can match on port/bridge ifindex, vlan id and fdb flags. It is used to
describe the existing dynamic fdb flush operation. Note that this flush
operation doesn't treat permanent entries in a special way (fdb_delete vs
fdb_delete_local), it will delete them regardless if any port is using
them, so currently it can't directly replace deletes which need to handle
that case, although we can extend it later for that too.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: changed the flush matches func for better readability (Ido)

 net/bridge/br_fdb.c      | 41 ++++++++++++++++++++++++++++++++--------
 net/bridge/br_netlink.c  |  9 +++++++--
 net/bridge/br_private.h  | 10 +++++++++-
 net/bridge/br_sysfs_br.c |  6 +++++-
 4 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 363985f1a540..45d02f2264db 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -558,24 +558,49 @@ void br_fdb_cleanup(struct work_struct *work)
 	mod_delayed_work(system_long_wq, &br->gc_work, work_delay);
 }
 
-/* Completely flush all dynamic entries in forwarding database.*/
-void br_fdb_flush(struct net_bridge *br)
+static bool __fdb_flush_matches(const struct net_bridge *br,
+				const struct net_bridge_fdb_entry *f,
+				const struct net_bridge_fdb_flush_desc *desc)
+{
+	const struct net_bridge_port *dst = READ_ONCE(f->dst);
+	int port_ifidx = dst ? dst->dev->ifindex : br->dev->ifindex;
+
+	if (desc->vlan_id && desc->vlan_id != f->key.vlan_id)
+		return false;
+	if (desc->port_ifindex && desc->port_ifindex != port_ifidx)
+		return false;
+	if (desc->flags_mask && (f->flags & desc->flags_mask) != desc->flags)
+		return false;
+
+	return true;
+}
+
+/* Flush forwarding database entries matching the description */
+void br_fdb_flush(struct net_bridge *br,
+		  const struct net_bridge_fdb_flush_desc *desc)
 {
 	struct net_bridge_fdb_entry *f;
-	struct hlist_node *tmp;
 
-	spin_lock_bh(&br->hash_lock);
-	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
-		if (!test_bit(BR_FDB_STATIC, &f->flags))
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
+		if (!__fdb_flush_matches(br, f, desc))
+			continue;
+
+		spin_lock_bh(&br->hash_lock);
+		if (!hlist_unhashed(&f->fdb_node))
 			fdb_delete(br, f, true);
+		spin_unlock_bh(&br->hash_lock);
 	}
-	spin_unlock_bh(&br->hash_lock);
+	rcu_read_unlock();
 }
 
 int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		       struct net_device *dev, u16 vid,
 		       struct netlink_ext_ack *extack)
 {
+	struct net_bridge_fdb_flush_desc desc = {
+		.flags_mask = BR_FDB_STATIC
+	};
 	struct net_bridge_port *p = NULL;
 	struct net_bridge *br;
 
@@ -590,7 +615,7 @@ int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		br = p->br;
 	}
 
-	br_fdb_flush(br);
+	br_fdb_flush(br, &desc);
 
 	return 0;
 }
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 200ad05b296f..bb01776d2d88 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1326,8 +1326,13 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 		br_recalculate_fwd_mask(br);
 	}
 
-	if (data[IFLA_BR_FDB_FLUSH])
-		br_fdb_flush(br);
+	if (data[IFLA_BR_FDB_FLUSH]) {
+		struct net_bridge_fdb_flush_desc desc = {
+			.flags_mask = BR_FDB_STATIC
+		};
+
+		br_fdb_flush(br, &desc);
+	}
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	if (data[IFLA_BR_MCAST_ROUTER]) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f37d49bf5637..4d2a809546fb 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -274,6 +274,13 @@ struct net_bridge_fdb_entry {
 	struct rcu_head			rcu;
 };
 
+struct net_bridge_fdb_flush_desc {
+	unsigned long			flags;
+	unsigned long			flags_mask;
+	int				port_ifindex;
+	u16				vlan_id;
+};
+
 #define MDB_PG_FLAGS_PERMANENT	BIT(0)
 #define MDB_PG_FLAGS_OFFLOAD	BIT(1)
 #define MDB_PG_FLAGS_FAST_LEAVE	BIT(2)
@@ -759,7 +766,8 @@ int br_fdb_init(void);
 void br_fdb_fini(void);
 int br_fdb_hash_init(struct net_bridge *br);
 void br_fdb_hash_fini(struct net_bridge *br);
-void br_fdb_flush(struct net_bridge *br);
+void br_fdb_flush(struct net_bridge *br,
+		  const struct net_bridge_fdb_flush_desc *desc);
 void br_fdb_find_delete_local(struct net_bridge *br,
 			      const struct net_bridge_port *p,
 			      const unsigned char *addr, u16 vid);
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 3f7ca88c2aa3..612e367fff20 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -344,7 +344,11 @@ static DEVICE_ATTR_RW(group_addr);
 static int set_flush(struct net_bridge *br, unsigned long val,
 		     struct netlink_ext_ack *extack)
 {
-	br_fdb_flush(br);
+	struct net_bridge_fdb_flush_desc desc = {
+		.flags_mask = BR_FDB_STATIC
+	};
+
+	br_fdb_flush(br, &desc);
 	return 0;
 }
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 10/12] net: rtnetlink: add ndm flags and state mask attributes
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:52   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:52 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add ndm flags/state masks which will be used for bulk delete filtering.
All of these are used by the bridge and vxlan drivers. Also minimal attr
policy validation is added, it is up to ndo_fdb_del_bulk implementers to
further validate them.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 include/uapi/linux/neighbour.h | 2 ++
 net/core/rtnetlink.c           | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index db05fb55055e..39c565e460c7 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -32,6 +32,8 @@ enum {
 	NDA_NH_ID,
 	NDA_FDB_EXT_ATTRS,
 	NDA_FLAGS_EXT,
+	NDA_NDM_STATE_MASK,
+	NDA_NDM_FLAGS_MASK,
 	__NDA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 520d50fcaaea..ab7fb9a16da9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4172,6 +4172,8 @@ EXPORT_SYMBOL(ndo_dflt_fdb_del);
 static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
 	[NDA_VLAN]	= { .type = NLA_U16 },
 	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
+	[NDA_NDM_STATE_MASK]	= { .type = NLA_U16  },
+	[NDA_NDM_FLAGS_MASK]	= { .type = NLA_U8 },
 };
 
 static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 10/12] net: rtnetlink: add ndm flags and state mask attributes
@ 2022-04-13 10:52   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:52 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add ndm flags/state masks which will be used for bulk delete filtering.
All of these are used by the bridge and vxlan drivers. Also minimal attr
policy validation is added, it is up to ndo_fdb_del_bulk implementers to
further validate them.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 include/uapi/linux/neighbour.h | 2 ++
 net/core/rtnetlink.c           | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index db05fb55055e..39c565e460c7 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -32,6 +32,8 @@ enum {
 	NDA_NH_ID,
 	NDA_FDB_EXT_ATTRS,
 	NDA_FLAGS_EXT,
+	NDA_NDM_STATE_MASK,
+	NDA_NDM_FLAGS_MASK,
 	__NDA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 520d50fcaaea..ab7fb9a16da9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4172,6 +4172,8 @@ EXPORT_SYMBOL(ndo_dflt_fdb_del);
 static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
 	[NDA_VLAN]	= { .type = NLA_U16 },
 	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
+	[NDA_NDM_STATE_MASK]	= { .type = NLA_U16  },
+	[NDA_NDM_FLAGS_MASK]	= { .type = NLA_U8 },
 };
 
 static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 11/12] net: bridge: fdb: add support for flush filtering based on ndm flags and state
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:52   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:52 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add support for fdb flush filtering based on ndm flags and state. NDM
state and flags are mapped to bridge-specific flags and matched
according to the specified masks. NTF_USE is used to represent
added_by_user flag since it sets it on fdb add and we don't have a 1:1
mapping for it. Only allowed bits can be set, NTF_SELF and NTF_MASTER are
ignored.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: ignore NTF_USE/NTF_MASTER and reject unknown flags
v3: NDFA -> NDA attributes

 net/bridge/br_fdb.c     | 58 ++++++++++++++++++++++++++++++++++++++---
 net/bridge/br_private.h |  5 ++++
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 45d02f2264db..74d759d09f94 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -594,13 +594,40 @@ void br_fdb_flush(struct net_bridge *br,
 	rcu_read_unlock();
 }
 
+static unsigned long __ndm_state_to_fdb_flags(u16 ndm_state)
+{
+	unsigned long flags = 0;
+
+	if (ndm_state & NUD_PERMANENT)
+		__set_bit(BR_FDB_LOCAL, &flags);
+	if (ndm_state & NUD_NOARP)
+		__set_bit(BR_FDB_STATIC, &flags);
+
+	return flags;
+}
+
+static unsigned long __ndm_flags_to_fdb_flags(u8 ndm_flags)
+{
+	unsigned long flags = 0;
+
+	if (ndm_flags & NTF_USE)
+		__set_bit(BR_FDB_ADDED_BY_USER, &flags);
+	if (ndm_flags & NTF_EXT_LEARNED)
+		__set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &flags);
+	if (ndm_flags & NTF_OFFLOADED)
+		__set_bit(BR_FDB_OFFLOADED, &flags);
+	if (ndm_flags & NTF_STICKY)
+		__set_bit(BR_FDB_STICKY, &flags);
+
+	return flags;
+}
+
 int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		       struct net_device *dev, u16 vid,
 		       struct netlink_ext_ack *extack)
 {
-	struct net_bridge_fdb_flush_desc desc = {
-		.flags_mask = BR_FDB_STATIC
-	};
+	u8 ndm_flags = ndm->ndm_flags & ~FDB_FLUSH_IGNORED_NDM_FLAGS;
+	struct net_bridge_fdb_flush_desc desc = {};
 	struct net_bridge_port *p = NULL;
 	struct net_bridge *br;
 
@@ -615,6 +642,31 @@ int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		br = p->br;
 	}
 
+	if (ndm_flags & ~FDB_FLUSH_ALLOWED_NDM_FLAGS) {
+		NL_SET_ERR_MSG(extack, "Unsupported fdb flush ndm flag bits set");
+		return -EINVAL;
+	}
+	if (ndm->ndm_state & ~FDB_FLUSH_ALLOWED_NDM_STATES) {
+		NL_SET_ERR_MSG(extack, "Unsupported fdb flush ndm state bits set");
+		return -EINVAL;
+	}
+
+	desc.flags |= __ndm_state_to_fdb_flags(ndm->ndm_state);
+	desc.flags |= __ndm_flags_to_fdb_flags(ndm_flags);
+	if (tb[NDA_NDM_STATE_MASK]) {
+		u16 ndm_state_mask = nla_get_u16(tb[NDA_NDM_STATE_MASK]);
+
+		desc.flags_mask |= __ndm_state_to_fdb_flags(ndm_state_mask);
+	}
+	if (tb[NDA_NDM_FLAGS_MASK]) {
+		u8 ndm_flags_mask = nla_get_u8(tb[NDA_NDM_FLAGS_MASK]);
+
+		desc.flags_mask |= __ndm_flags_to_fdb_flags(ndm_flags_mask);
+	}
+
+	br_debug(br, "flushing port ifindex: %d vlan id: %u flags: 0x%lx flags mask: 0x%lx\n",
+		 desc.port_ifindex, desc.vlan_id, desc.flags, desc.flags_mask);
+
 	br_fdb_flush(br, &desc);
 
 	return 0;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4d2a809546fb..353dd4a6da7c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -762,6 +762,11 @@ static inline void br_netpoll_disable(struct net_bridge_port *p)
 #endif
 
 /* br_fdb.c */
+#define FDB_FLUSH_IGNORED_NDM_FLAGS (NTF_MASTER | NTF_SELF)
+#define FDB_FLUSH_ALLOWED_NDM_STATES (NUD_PERMANENT | NUD_NOARP)
+#define FDB_FLUSH_ALLOWED_NDM_FLAGS (NTF_USE | NTF_EXT_LEARNED | \
+				     NTF_STICKY | NTF_OFFLOADED)
+
 int br_fdb_init(void);
 void br_fdb_fini(void);
 int br_fdb_hash_init(struct net_bridge *br);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 11/12] net: bridge: fdb: add support for flush filtering based on ndm flags and state
@ 2022-04-13 10:52   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:52 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add support for fdb flush filtering based on ndm flags and state. NDM
state and flags are mapped to bridge-specific flags and matched
according to the specified masks. NTF_USE is used to represent
added_by_user flag since it sets it on fdb add and we don't have a 1:1
mapping for it. Only allowed bits can be set, NTF_SELF and NTF_MASTER are
ignored.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: ignore NTF_USE/NTF_MASTER and reject unknown flags
v3: NDFA -> NDA attributes

 net/bridge/br_fdb.c     | 58 ++++++++++++++++++++++++++++++++++++++---
 net/bridge/br_private.h |  5 ++++
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 45d02f2264db..74d759d09f94 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -594,13 +594,40 @@ void br_fdb_flush(struct net_bridge *br,
 	rcu_read_unlock();
 }
 
+static unsigned long __ndm_state_to_fdb_flags(u16 ndm_state)
+{
+	unsigned long flags = 0;
+
+	if (ndm_state & NUD_PERMANENT)
+		__set_bit(BR_FDB_LOCAL, &flags);
+	if (ndm_state & NUD_NOARP)
+		__set_bit(BR_FDB_STATIC, &flags);
+
+	return flags;
+}
+
+static unsigned long __ndm_flags_to_fdb_flags(u8 ndm_flags)
+{
+	unsigned long flags = 0;
+
+	if (ndm_flags & NTF_USE)
+		__set_bit(BR_FDB_ADDED_BY_USER, &flags);
+	if (ndm_flags & NTF_EXT_LEARNED)
+		__set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &flags);
+	if (ndm_flags & NTF_OFFLOADED)
+		__set_bit(BR_FDB_OFFLOADED, &flags);
+	if (ndm_flags & NTF_STICKY)
+		__set_bit(BR_FDB_STICKY, &flags);
+
+	return flags;
+}
+
 int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		       struct net_device *dev, u16 vid,
 		       struct netlink_ext_ack *extack)
 {
-	struct net_bridge_fdb_flush_desc desc = {
-		.flags_mask = BR_FDB_STATIC
-	};
+	u8 ndm_flags = ndm->ndm_flags & ~FDB_FLUSH_IGNORED_NDM_FLAGS;
+	struct net_bridge_fdb_flush_desc desc = {};
 	struct net_bridge_port *p = NULL;
 	struct net_bridge *br;
 
@@ -615,6 +642,31 @@ int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		br = p->br;
 	}
 
+	if (ndm_flags & ~FDB_FLUSH_ALLOWED_NDM_FLAGS) {
+		NL_SET_ERR_MSG(extack, "Unsupported fdb flush ndm flag bits set");
+		return -EINVAL;
+	}
+	if (ndm->ndm_state & ~FDB_FLUSH_ALLOWED_NDM_STATES) {
+		NL_SET_ERR_MSG(extack, "Unsupported fdb flush ndm state bits set");
+		return -EINVAL;
+	}
+
+	desc.flags |= __ndm_state_to_fdb_flags(ndm->ndm_state);
+	desc.flags |= __ndm_flags_to_fdb_flags(ndm_flags);
+	if (tb[NDA_NDM_STATE_MASK]) {
+		u16 ndm_state_mask = nla_get_u16(tb[NDA_NDM_STATE_MASK]);
+
+		desc.flags_mask |= __ndm_state_to_fdb_flags(ndm_state_mask);
+	}
+	if (tb[NDA_NDM_FLAGS_MASK]) {
+		u8 ndm_flags_mask = nla_get_u8(tb[NDA_NDM_FLAGS_MASK]);
+
+		desc.flags_mask |= __ndm_flags_to_fdb_flags(ndm_flags_mask);
+	}
+
+	br_debug(br, "flushing port ifindex: %d vlan id: %u flags: 0x%lx flags mask: 0x%lx\n",
+		 desc.port_ifindex, desc.vlan_id, desc.flags, desc.flags_mask);
+
 	br_fdb_flush(br, &desc);
 
 	return 0;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4d2a809546fb..353dd4a6da7c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -762,6 +762,11 @@ static inline void br_netpoll_disable(struct net_bridge_port *p)
 #endif
 
 /* br_fdb.c */
+#define FDB_FLUSH_IGNORED_NDM_FLAGS (NTF_MASTER | NTF_SELF)
+#define FDB_FLUSH_ALLOWED_NDM_STATES (NUD_PERMANENT | NUD_NOARP)
+#define FDB_FLUSH_ALLOWED_NDM_FLAGS (NTF_USE | NTF_EXT_LEARNED | \
+				     NTF_STICKY | NTF_OFFLOADED)
+
 int br_fdb_init(void);
 void br_fdb_fini(void);
 int br_fdb_hash_init(struct net_bridge *br);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH net-next v4 12/12] net: bridge: fdb: add support for flush filtering based on ifindex and vlan
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 10:52   ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:52 UTC (permalink / raw)
  To: netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge, Nikolay Aleksandrov

Add support for fdb flush filtering based on destination ifindex and
vlan id. The ifindex must either match a port's device ifindex or the
bridge's. The vlan support is trivial since it's already validated by
rtnl_fdb_del, we just need to fill it in.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: validate ifindex and fill in vlan id
v3: NDFA -> NDA attributes
v4: use port's ifindex if NTF_MASTER is used and NDA_IFINDEX is not
    specified

 net/bridge/br_fdb.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 74d759d09f94..1a3d583fbc8e 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -622,12 +622,44 @@ static unsigned long __ndm_flags_to_fdb_flags(u8 ndm_flags)
 	return flags;
 }
 
+static int __fdb_flush_validate_ifindex(const struct net_bridge *br,
+					int ifindex,
+					struct netlink_ext_ack *extack)
+{
+	const struct net_device *dev;
+
+	dev = __dev_get_by_index(dev_net(br->dev), ifindex);
+	if (!dev) {
+		NL_SET_ERR_MSG_MOD(extack, "Unknown flush device ifindex");
+		return -ENODEV;
+	}
+	if (!netif_is_bridge_master(dev) && !netif_is_bridge_port(dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Flush device is not a bridge or bridge port");
+		return -EINVAL;
+	}
+	if (netif_is_bridge_master(dev) && dev != br->dev) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Flush bridge device does not match target bridge device");
+		return -EINVAL;
+	}
+	if (netif_is_bridge_port(dev)) {
+		struct net_bridge_port *p = br_port_get_rtnl(dev);
+
+		if (p->br != br) {
+			NL_SET_ERR_MSG_MOD(extack, "Port belongs to a different bridge device");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		       struct net_device *dev, u16 vid,
 		       struct netlink_ext_ack *extack)
 {
 	u8 ndm_flags = ndm->ndm_flags & ~FDB_FLUSH_IGNORED_NDM_FLAGS;
-	struct net_bridge_fdb_flush_desc desc = {};
+	struct net_bridge_fdb_flush_desc desc = { .vlan_id = vid };
 	struct net_bridge_port *p = NULL;
 	struct net_bridge *br;
 
@@ -663,6 +695,17 @@ int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 
 		desc.flags_mask |= __ndm_flags_to_fdb_flags(ndm_flags_mask);
 	}
+	if (tb[NDA_IFINDEX]) {
+		int err, ifidx = nla_get_s32(tb[NDA_IFINDEX]);
+
+		err = __fdb_flush_validate_ifindex(br, ifidx, extack);
+		if (err)
+			return err;
+		desc.port_ifindex = ifidx;
+	} else if (p) {
+		/* flush was invoked with port device and NTF_MASTER */
+		desc.port_ifindex = p->dev->ifindex;
+	}
 
 	br_debug(br, "flushing port ifindex: %d vlan id: %u flags: 0x%lx flags mask: 0x%lx\n",
 		 desc.port_ifindex, desc.vlan_id, desc.flags, desc.flags_mask);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [Bridge] [PATCH net-next v4 12/12] net: bridge: fdb: add support for flush filtering based on ifindex and vlan
@ 2022-04-13 10:52   ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 10:52 UTC (permalink / raw)
  To: netdev; +Cc: Nikolay Aleksandrov, dsahern, bridge, idosch, roopa, kuba, davem

Add support for fdb flush filtering based on destination ifindex and
vlan id. The ifindex must either match a port's device ifindex or the
bridge's. The vlan support is trivial since it's already validated by
rtnl_fdb_del, we just need to fill it in.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
v2: validate ifindex and fill in vlan id
v3: NDFA -> NDA attributes
v4: use port's ifindex if NTF_MASTER is used and NDA_IFINDEX is not
    specified

 net/bridge/br_fdb.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 74d759d09f94..1a3d583fbc8e 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -622,12 +622,44 @@ static unsigned long __ndm_flags_to_fdb_flags(u8 ndm_flags)
 	return flags;
 }
 
+static int __fdb_flush_validate_ifindex(const struct net_bridge *br,
+					int ifindex,
+					struct netlink_ext_ack *extack)
+{
+	const struct net_device *dev;
+
+	dev = __dev_get_by_index(dev_net(br->dev), ifindex);
+	if (!dev) {
+		NL_SET_ERR_MSG_MOD(extack, "Unknown flush device ifindex");
+		return -ENODEV;
+	}
+	if (!netif_is_bridge_master(dev) && !netif_is_bridge_port(dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Flush device is not a bridge or bridge port");
+		return -EINVAL;
+	}
+	if (netif_is_bridge_master(dev) && dev != br->dev) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Flush bridge device does not match target bridge device");
+		return -EINVAL;
+	}
+	if (netif_is_bridge_port(dev)) {
+		struct net_bridge_port *p = br_port_get_rtnl(dev);
+
+		if (p->br != br) {
+			NL_SET_ERR_MSG_MOD(extack, "Port belongs to a different bridge device");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 		       struct net_device *dev, u16 vid,
 		       struct netlink_ext_ack *extack)
 {
 	u8 ndm_flags = ndm->ndm_flags & ~FDB_FLUSH_IGNORED_NDM_FLAGS;
-	struct net_bridge_fdb_flush_desc desc = {};
+	struct net_bridge_fdb_flush_desc desc = { .vlan_id = vid };
 	struct net_bridge_port *p = NULL;
 	struct net_bridge *br;
 
@@ -663,6 +695,17 @@ int br_fdb_delete_bulk(struct ndmsg *ndm, struct nlattr *tb[],
 
 		desc.flags_mask |= __ndm_flags_to_fdb_flags(ndm_flags_mask);
 	}
+	if (tb[NDA_IFINDEX]) {
+		int err, ifidx = nla_get_s32(tb[NDA_IFINDEX]);
+
+		err = __fdb_flush_validate_ifindex(br, ifidx, extack);
+		if (err)
+			return err;
+		desc.port_ifindex = ifidx;
+	} else if (p) {
+		/* flush was invoked with port device and NTF_MASTER */
+		desc.port_ifindex = p->dev->ifindex;
+	}
 
 	br_debug(br, "flushing port ifindex: %d vlan id: %u flags: 0x%lx flags mask: 0x%lx\n",
 		 desc.port_ifindex, desc.vlan_id, desc.flags, desc.flags_mask);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 00/12] net: bridge: add flush filtering support
  2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 11:50   ` patchwork-bot+netdevbpf
  -1 siblings, 0 replies; 46+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-04-13 11:50 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, dsahern, roopa, idosch, kuba, davem, bridge

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 13 Apr 2022 13:51:50 +0300 you wrote:
> Hi,
> This patch-set adds support to specify filtering conditions for a bulk
> delete (flush) operation. This version uses a new nlmsghdr delete flag
> called NLM_F_BULK in combination with a new ndo_fdb_del_bulk op which is
> used to signal that the driver supports bulk deletes (that avoids
> pushing common mac address checks to ndo_fdb_del implementations and
> also has a different prototype and parsed attribute expectations, more
> info in patch 03). The new delete flag can be used for any RTM_DEL*
> type, implementations just need to be careful with older kernels which
> are doing non-strict attribute parses. A new rtnl flag
> (RTNL_FLAG_BULK_DEL_SUPPORTED) is used to show that the delete supports
> NLM_F_BULK. A proper error is returned if bulk delete is not supported.
> For old kernels I use the fact that mac address attribute (lladdr) is
> mandatory in the classic fdb del case, but it's not allowed if bulk
> deleting so older kernels will error out.
> 
> [...]

Here is the summary with links:
  - [net-next,v4,01/12] net: rtnetlink: add msg kind names
    https://git.kernel.org/netdev/net-next/c/12dc5c2cb7b2
  - [net-next,v4,02/12] net: rtnetlink: add helper to extract msg type's kind
    https://git.kernel.org/netdev/net-next/c/2e9ea3e30f69
  - [net-next,v4,03/12] net: rtnetlink: use BIT for flag values
    https://git.kernel.org/netdev/net-next/c/0569e31f1bc2
  - [net-next,v4,04/12] net: netlink: add NLM_F_BULK delete request modifier
    https://git.kernel.org/netdev/net-next/c/545528d78855
  - [net-next,v4,05/12] net: rtnetlink: add bulk delete support flag
    https://git.kernel.org/netdev/net-next/c/a6cec0bcd342
  - [net-next,v4,06/12] net: add ndo_fdb_del_bulk
    https://git.kernel.org/netdev/net-next/c/1306d5362a59
  - [net-next,v4,07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
    https://git.kernel.org/netdev/net-next/c/9e83425993f3
  - [net-next,v4,08/12] net: bridge: fdb: add ndo_fdb_del_bulk
    https://git.kernel.org/netdev/net-next/c/edaef1917224
  - [net-next,v4,09/12] net: bridge: fdb: add support for fine-grained flushing
    https://git.kernel.org/netdev/net-next/c/1f78ee14eeac
  - [net-next,v4,10/12] net: rtnetlink: add ndm flags and state mask attributes
    https://git.kernel.org/netdev/net-next/c/ea2c0f9e3fc2
  - [net-next,v4,11/12] net: bridge: fdb: add support for flush filtering based on ndm flags and state
    https://git.kernel.org/netdev/net-next/c/564445fb4f0f
  - [net-next,v4,12/12] net: bridge: fdb: add support for flush filtering based on ifindex and vlan
    https://git.kernel.org/netdev/net-next/c/0dbe886a4d8d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 00/12] net: bridge: add flush filtering support
@ 2022-04-13 11:50   ` patchwork-bot+netdevbpf
  0 siblings, 0 replies; 46+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-04-13 11:50 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, dsahern, bridge, idosch, roopa, kuba, davem

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 13 Apr 2022 13:51:50 +0300 you wrote:
> Hi,
> This patch-set adds support to specify filtering conditions for a bulk
> delete (flush) operation. This version uses a new nlmsghdr delete flag
> called NLM_F_BULK in combination with a new ndo_fdb_del_bulk op which is
> used to signal that the driver supports bulk deletes (that avoids
> pushing common mac address checks to ndo_fdb_del implementations and
> also has a different prototype and parsed attribute expectations, more
> info in patch 03). The new delete flag can be used for any RTM_DEL*
> type, implementations just need to be careful with older kernels which
> are doing non-strict attribute parses. A new rtnl flag
> (RTNL_FLAG_BULK_DEL_SUPPORTED) is used to show that the delete supports
> NLM_F_BULK. A proper error is returned if bulk delete is not supported.
> For old kernels I use the fact that mac address attribute (lladdr) is
> mandatory in the classic fdb del case, but it's not allowed if bulk
> deleting so older kernels will error out.
> 
> [...]

Here is the summary with links:
  - [net-next,v4,01/12] net: rtnetlink: add msg kind names
    https://git.kernel.org/netdev/net-next/c/12dc5c2cb7b2
  - [net-next,v4,02/12] net: rtnetlink: add helper to extract msg type's kind
    https://git.kernel.org/netdev/net-next/c/2e9ea3e30f69
  - [net-next,v4,03/12] net: rtnetlink: use BIT for flag values
    https://git.kernel.org/netdev/net-next/c/0569e31f1bc2
  - [net-next,v4,04/12] net: netlink: add NLM_F_BULK delete request modifier
    https://git.kernel.org/netdev/net-next/c/545528d78855
  - [net-next,v4,05/12] net: rtnetlink: add bulk delete support flag
    https://git.kernel.org/netdev/net-next/c/a6cec0bcd342
  - [net-next,v4,06/12] net: add ndo_fdb_del_bulk
    https://git.kernel.org/netdev/net-next/c/1306d5362a59
  - [net-next,v4,07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
    https://git.kernel.org/netdev/net-next/c/9e83425993f3
  - [net-next,v4,08/12] net: bridge: fdb: add ndo_fdb_del_bulk
    https://git.kernel.org/netdev/net-next/c/edaef1917224
  - [net-next,v4,09/12] net: bridge: fdb: add support for fine-grained flushing
    https://git.kernel.org/netdev/net-next/c/1f78ee14eeac
  - [net-next,v4,10/12] net: rtnetlink: add ndm flags and state mask attributes
    https://git.kernel.org/netdev/net-next/c/ea2c0f9e3fc2
  - [net-next,v4,11/12] net: bridge: fdb: add support for flush filtering based on ndm flags and state
    https://git.kernel.org/netdev/net-next/c/564445fb4f0f
  - [net-next,v4,12/12] net: bridge: fdb: add support for flush filtering based on ifindex and vlan
    https://git.kernel.org/netdev/net-next/c/0dbe886a4d8d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag
  2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 12:06     ` Ido Schimmel
  -1 siblings, 0 replies; 46+ messages in thread
From: Ido Schimmel @ 2022-04-13 12:06 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, dsahern, roopa, kuba, davem, bridge

On Wed, Apr 13, 2022 at 01:51:55PM +0300, Nikolay Aleksandrov wrote:
> Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to
> verify that the delete operation allows bulk object deletion. Also emit
> a warning if anyone tries to set it for non-delete kind.
> 
> Suggested-by: David Ahern <dsahern@kernel.org>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
> v4: new patch
> 
>  include/net/rtnetlink.h | 3 ++-
>  net/core/rtnetlink.c    | 8 ++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> index 0bf622409aaa..bf8bb3357825 100644
> --- a/include/net/rtnetlink.h
> +++ b/include/net/rtnetlink.h
> @@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *,
>  typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
>  
>  enum rtnl_link_flags {
> -	RTNL_FLAG_DOIT_UNLOCKED = BIT(0),
> +	RTNL_FLAG_DOIT_UNLOCKED		= BIT(0),
> +	RTNL_FLAG_BULK_DEL_SUPPORTED	= BIT(1),
>  };
>  
>  enum rtnl_kinds {
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index beda4a7da062..63c7df52a667 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner,
>  	if (dumpit)
>  		link->dumpit = dumpit;
>  
> +	WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL &&
> +		(flags & RTNL_FLAG_BULK_DEL_SUPPORTED));
>  	link->flags |= flags;
>  
>  	/* publish protocol:msgtype */
> @@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	}
>  
>  	flags = link->flags;
> +	if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) &&
> +	    !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) {
> +		NL_SET_ERR_MSG(extack, "Bulk delete is not supported");
> +		goto err_unlock;

If a buggy user space application is sending messages with NLM_F_BULK
set (unintentionally), will it break on newer kernel? I couldn't find
where the kernel was validating that reserved flags are not used (I
suspect it doesn't).

Assuming the above is correct and of interest, maybe just emit a warning
via extack and drop the goto? Alternatively, we can see if anyone
complains which might never happen

> +	}
> +
>  	if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
>  		doit = link->doit;
>  		rcu_read_unlock();
> -- 
> 2.35.1
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag
@ 2022-04-13 12:06     ` Ido Schimmel
  0 siblings, 0 replies; 46+ messages in thread
From: Ido Schimmel @ 2022-04-13 12:06 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, dsahern, bridge, roopa, kuba, davem

On Wed, Apr 13, 2022 at 01:51:55PM +0300, Nikolay Aleksandrov wrote:
> Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to
> verify that the delete operation allows bulk object deletion. Also emit
> a warning if anyone tries to set it for non-delete kind.
> 
> Suggested-by: David Ahern <dsahern@kernel.org>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
> v4: new patch
> 
>  include/net/rtnetlink.h | 3 ++-
>  net/core/rtnetlink.c    | 8 ++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> index 0bf622409aaa..bf8bb3357825 100644
> --- a/include/net/rtnetlink.h
> +++ b/include/net/rtnetlink.h
> @@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *,
>  typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
>  
>  enum rtnl_link_flags {
> -	RTNL_FLAG_DOIT_UNLOCKED = BIT(0),
> +	RTNL_FLAG_DOIT_UNLOCKED		= BIT(0),
> +	RTNL_FLAG_BULK_DEL_SUPPORTED	= BIT(1),
>  };
>  
>  enum rtnl_kinds {
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index beda4a7da062..63c7df52a667 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner,
>  	if (dumpit)
>  		link->dumpit = dumpit;
>  
> +	WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL &&
> +		(flags & RTNL_FLAG_BULK_DEL_SUPPORTED));
>  	link->flags |= flags;
>  
>  	/* publish protocol:msgtype */
> @@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	}
>  
>  	flags = link->flags;
> +	if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) &&
> +	    !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) {
> +		NL_SET_ERR_MSG(extack, "Bulk delete is not supported");
> +		goto err_unlock;

If a buggy user space application is sending messages with NLM_F_BULK
set (unintentionally), will it break on newer kernel? I couldn't find
where the kernel was validating that reserved flags are not used (I
suspect it doesn't).

Assuming the above is correct and of interest, maybe just emit a warning
via extack and drop the goto? Alternatively, we can see if anyone
complains which might never happen

> +	}
> +
>  	if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
>  		doit = link->doit;
>  		rcu_read_unlock();
> -- 
> 2.35.1
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
  2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 12:20     ` Ido Schimmel
  -1 siblings, 0 replies; 46+ messages in thread
From: Ido Schimmel @ 2022-04-13 12:20 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, dsahern, roopa, kuba, davem, bridge

On Wed, Apr 13, 2022 at 01:51:57PM +0300, Nikolay Aleksandrov wrote:
> When NLM_F_BULK is specified in a fdb del message we need to handle it
> differently. First since this is a new call we can strictly validate the
> passed attributes, at first only ifindex and vlan are allowed as these
> will be the initially supported filter attributes, any other attribute
> is rejected. The mac address is no longer mandatory, but we use it
> to error out in older kernels because it cannot be specified with bulk
> request (the attribute is not allowed) and then we have to dispatch
> the call to ndo_fdb_del_bulk if the device supports it. The del bulk
> callback can do further validation of the attributes if necessary.
> 
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
> v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED
> 
>  net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 63c7df52a667..520d50fcaaea 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
>  }
>  EXPORT_SYMBOL(ndo_dflt_fdb_del);
>  
> +static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
> +	[NDA_VLAN]	= { .type = NLA_U16 },

In earlier versions br_vlan_valid_id() was used to validate the VLAN,
but I don't see it anymore. Maybe use 

NLA_POLICY_RANGE(1, VLAN_N_VID - 2)

?

I realize that invalid values won't do anything, but I think it's better
to only allow valid ranges.

> +	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
> +};
> +
>  static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			struct netlink_ext_ack *extack)
>  {
> +	bool del_bulk = !!(nlh->nlmsg_flags & NLM_F_BULK);
>  	struct net *net = sock_net(skb->sk);
> +	const struct net_device_ops *ops;
>  	struct ndmsg *ndm;
>  	struct nlattr *tb[NDA_MAX+1];
>  	struct net_device *dev;
> -	__u8 *addr;
> +	__u8 *addr = NULL;
>  	int err;
>  	u16 vid;
>  
>  	if (!netlink_capable(skb, CAP_NET_ADMIN))
>  		return -EPERM;
>  
> -	err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX, NULL,
> -				     extack);
> +	if (!del_bulk) {
> +		err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX,
> +					     NULL, extack);
> +	} else {
> +		err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX,
> +				  fdb_del_bulk_policy, extack);
> +	}
>  	if (err < 0)
>  		return err;
>  
> @@ -4200,9 +4212,12 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return -ENODEV;
>  	}
>  
> -	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
> -		NL_SET_ERR_MSG(extack, "invalid address");
> -		return -EINVAL;
> +	if (!del_bulk) {
> +		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]);
>  	}
>  
>  	if (dev->type != ARPHRD_ETHER) {
> @@ -4210,8 +4225,6 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return -EINVAL;
>  	}
>  
> -	addr = nla_data(tb[NDA_LLADDR]);
> -
>  	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
>  	if (err)
>  		return err;
> @@ -4222,10 +4235,16 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>  	    netif_is_bridge_port(dev)) {
>  		struct net_device *br_dev = netdev_master_upper_dev_get(dev);
> -		const struct net_device_ops *ops = br_dev->netdev_ops;
>  
> -		if (ops->ndo_fdb_del)
> -			err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +		ops = br_dev->netdev_ops;
> +		if (!del_bulk) {
> +			if (ops->ndo_fdb_del)
> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +		} else {
> +			if (ops->ndo_fdb_del_bulk)
> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
> +							    extack);
> +		}
>  
>  		if (err)
>  			goto out;
> @@ -4235,15 +4254,24 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  
>  	/* Embedded bridge, macvlan, and any other device support */
>  	if (ndm->ndm_flags & NTF_SELF) {
> -		if (dev->netdev_ops->ndo_fdb_del)
> -			err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr,
> -							   vid);
> -		else
> -			err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
> +		ops = dev->netdev_ops;
> +		if (!del_bulk) {
> +			if (ops->ndo_fdb_del)
> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +			else
> +				err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
> +		} else {
> +			/* in case err was cleared by NTF_MASTER call */
> +			err = -EOPNOTSUPP;
> +			if (ops->ndo_fdb_del_bulk)
> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
> +							    extack);
> +		}
>  
>  		if (!err) {
> -			rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
> -					ndm->ndm_state);
> +			if (!del_bulk)
> +				rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
> +						ndm->ndm_state);
>  			ndm->ndm_flags &= ~NTF_SELF;
>  		}
>  	}
> @@ -6145,7 +6173,8 @@ void __init rtnetlink_init(void)
>  	rtnl_register(PF_UNSPEC, RTM_DELLINKPROP, rtnl_dellinkprop, NULL, 0);
>  
>  	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_DELNEIGH, rtnl_fdb_del, NULL,
> +		      RTNL_FLAG_BULK_DEL_SUPPORTED);
>  	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
>  
>  	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
> -- 
> 2.35.1
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
@ 2022-04-13 12:20     ` Ido Schimmel
  0 siblings, 0 replies; 46+ messages in thread
From: Ido Schimmel @ 2022-04-13 12:20 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, dsahern, bridge, roopa, kuba, davem

On Wed, Apr 13, 2022 at 01:51:57PM +0300, Nikolay Aleksandrov wrote:
> When NLM_F_BULK is specified in a fdb del message we need to handle it
> differently. First since this is a new call we can strictly validate the
> passed attributes, at first only ifindex and vlan are allowed as these
> will be the initially supported filter attributes, any other attribute
> is rejected. The mac address is no longer mandatory, but we use it
> to error out in older kernels because it cannot be specified with bulk
> request (the attribute is not allowed) and then we have to dispatch
> the call to ndo_fdb_del_bulk if the device supports it. The del bulk
> callback can do further validation of the attributes if necessary.
> 
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
> v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED
> 
>  net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 63c7df52a667..520d50fcaaea 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
>  }
>  EXPORT_SYMBOL(ndo_dflt_fdb_del);
>  
> +static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
> +	[NDA_VLAN]	= { .type = NLA_U16 },

In earlier versions br_vlan_valid_id() was used to validate the VLAN,
but I don't see it anymore. Maybe use 

NLA_POLICY_RANGE(1, VLAN_N_VID - 2)

?

I realize that invalid values won't do anything, but I think it's better
to only allow valid ranges.

> +	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
> +};
> +
>  static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			struct netlink_ext_ack *extack)
>  {
> +	bool del_bulk = !!(nlh->nlmsg_flags & NLM_F_BULK);
>  	struct net *net = sock_net(skb->sk);
> +	const struct net_device_ops *ops;
>  	struct ndmsg *ndm;
>  	struct nlattr *tb[NDA_MAX+1];
>  	struct net_device *dev;
> -	__u8 *addr;
> +	__u8 *addr = NULL;
>  	int err;
>  	u16 vid;
>  
>  	if (!netlink_capable(skb, CAP_NET_ADMIN))
>  		return -EPERM;
>  
> -	err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX, NULL,
> -				     extack);
> +	if (!del_bulk) {
> +		err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX,
> +					     NULL, extack);
> +	} else {
> +		err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX,
> +				  fdb_del_bulk_policy, extack);
> +	}
>  	if (err < 0)
>  		return err;
>  
> @@ -4200,9 +4212,12 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return -ENODEV;
>  	}
>  
> -	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
> -		NL_SET_ERR_MSG(extack, "invalid address");
> -		return -EINVAL;
> +	if (!del_bulk) {
> +		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]);
>  	}
>  
>  	if (dev->type != ARPHRD_ETHER) {
> @@ -4210,8 +4225,6 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return -EINVAL;
>  	}
>  
> -	addr = nla_data(tb[NDA_LLADDR]);
> -
>  	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
>  	if (err)
>  		return err;
> @@ -4222,10 +4235,16 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>  	    netif_is_bridge_port(dev)) {
>  		struct net_device *br_dev = netdev_master_upper_dev_get(dev);
> -		const struct net_device_ops *ops = br_dev->netdev_ops;
>  
> -		if (ops->ndo_fdb_del)
> -			err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +		ops = br_dev->netdev_ops;
> +		if (!del_bulk) {
> +			if (ops->ndo_fdb_del)
> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +		} else {
> +			if (ops->ndo_fdb_del_bulk)
> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
> +							    extack);
> +		}
>  
>  		if (err)
>  			goto out;
> @@ -4235,15 +4254,24 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  
>  	/* Embedded bridge, macvlan, and any other device support */
>  	if (ndm->ndm_flags & NTF_SELF) {
> -		if (dev->netdev_ops->ndo_fdb_del)
> -			err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr,
> -							   vid);
> -		else
> -			err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
> +		ops = dev->netdev_ops;
> +		if (!del_bulk) {
> +			if (ops->ndo_fdb_del)
> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
> +			else
> +				err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
> +		} else {
> +			/* in case err was cleared by NTF_MASTER call */
> +			err = -EOPNOTSUPP;
> +			if (ops->ndo_fdb_del_bulk)
> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
> +							    extack);
> +		}
>  
>  		if (!err) {
> -			rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
> -					ndm->ndm_state);
> +			if (!del_bulk)
> +				rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
> +						ndm->ndm_state);
>  			ndm->ndm_flags &= ~NTF_SELF;
>  		}
>  	}
> @@ -6145,7 +6173,8 @@ void __init rtnetlink_init(void)
>  	rtnl_register(PF_UNSPEC, RTM_DELLINKPROP, rtnl_dellinkprop, NULL, 0);
>  
>  	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_DELNEIGH, rtnl_fdb_del, NULL,
> +		      RTNL_FLAG_BULK_DEL_SUPPORTED);
>  	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
>  
>  	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
> -- 
> 2.35.1
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag
  2022-04-13 12:06     ` [Bridge] " Ido Schimmel
@ 2022-04-13 12:21       ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 12:21 UTC (permalink / raw)
  To: Ido Schimmel; +Cc: netdev, dsahern, roopa, kuba, davem, bridge

On 13/04/2022 15:06, Ido Schimmel wrote:
> On Wed, Apr 13, 2022 at 01:51:55PM +0300, Nikolay Aleksandrov wrote:
>> Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to
>> verify that the delete operation allows bulk object deletion. Also emit
>> a warning if anyone tries to set it for non-delete kind.
>>
>> Suggested-by: David Ahern <dsahern@kernel.org>
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>> v4: new patch
>>
>>  include/net/rtnetlink.h | 3 ++-
>>  net/core/rtnetlink.c    | 8 ++++++++
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
>> index 0bf622409aaa..bf8bb3357825 100644
>> --- a/include/net/rtnetlink.h
>> +++ b/include/net/rtnetlink.h
>> @@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *,
>>  typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
>>  
>>  enum rtnl_link_flags {
>> -	RTNL_FLAG_DOIT_UNLOCKED = BIT(0),
>> +	RTNL_FLAG_DOIT_UNLOCKED		= BIT(0),
>> +	RTNL_FLAG_BULK_DEL_SUPPORTED	= BIT(1),
>>  };
>>  
>>  enum rtnl_kinds {
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index beda4a7da062..63c7df52a667 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner,
>>  	if (dumpit)
>>  		link->dumpit = dumpit;
>>  
>> +	WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL &&
>> +		(flags & RTNL_FLAG_BULK_DEL_SUPPORTED));
>>  	link->flags |= flags;
>>  
>>  	/* publish protocol:msgtype */
>> @@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  	}
>>  
>>  	flags = link->flags;
>> +	if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) &&
>> +	    !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) {
>> +		NL_SET_ERR_MSG(extack, "Bulk delete is not supported");
>> +		goto err_unlock;
> 
> If a buggy user space application is sending messages with NLM_F_BULK
> set (unintentionally), will it break on newer kernel? I couldn't find
> where the kernel was validating that reserved flags are not used (I
> suspect it doesn't).

Correct, it doesn't.

> 
> Assuming the above is correct and of interest, maybe just emit a warning
> via extack and drop the goto? Alternatively, we can see if anyone
> complains which might never happen
> 

TBH I prefer to error out on an unsupported flag, but I get the problem. These
weren't validated before and we start checking now. The problem is that we'll
return an extack without an error, but the delete might also remove something.
Hrm.. perhaps we can rephrase the error in that case (since it becomes a warning
in iproute2 terms):
 "NLM_F_BULK flag is set but bulk delete operation is not supported"
So it will warn the user it has an unsupported flag.

WDYT ?

IMO we should bite the bullet and keep the error though. :)

>> +	}
>> +
>>  	if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
>>  		doit = link->doit;
>>  		rcu_read_unlock();
>> -- 
>> 2.35.1
>>


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag
@ 2022-04-13 12:21       ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 12:21 UTC (permalink / raw)
  To: Ido Schimmel; +Cc: netdev, dsahern, bridge, roopa, kuba, davem

On 13/04/2022 15:06, Ido Schimmel wrote:
> On Wed, Apr 13, 2022 at 01:51:55PM +0300, Nikolay Aleksandrov wrote:
>> Add a new rtnl flag (RTNL_FLAG_BULK_DEL_SUPPORTED) which is used to
>> verify that the delete operation allows bulk object deletion. Also emit
>> a warning if anyone tries to set it for non-delete kind.
>>
>> Suggested-by: David Ahern <dsahern@kernel.org>
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>> v4: new patch
>>
>>  include/net/rtnetlink.h | 3 ++-
>>  net/core/rtnetlink.c    | 8 ++++++++
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
>> index 0bf622409aaa..bf8bb3357825 100644
>> --- a/include/net/rtnetlink.h
>> +++ b/include/net/rtnetlink.h
>> @@ -10,7 +10,8 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *,
>>  typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
>>  
>>  enum rtnl_link_flags {
>> -	RTNL_FLAG_DOIT_UNLOCKED = BIT(0),
>> +	RTNL_FLAG_DOIT_UNLOCKED		= BIT(0),
>> +	RTNL_FLAG_BULK_DEL_SUPPORTED	= BIT(1),
>>  };
>>  
>>  enum rtnl_kinds {
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index beda4a7da062..63c7df52a667 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -249,6 +249,8 @@ static int rtnl_register_internal(struct module *owner,
>>  	if (dumpit)
>>  		link->dumpit = dumpit;
>>  
>> +	WARN_ON(rtnl_msgtype_kind(msgtype) != RTNL_KIND_DEL &&
>> +		(flags & RTNL_FLAG_BULK_DEL_SUPPORTED));
>>  	link->flags |= flags;
>>  
>>  	/* publish protocol:msgtype */
>> @@ -6009,6 +6011,12 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  	}
>>  
>>  	flags = link->flags;
>> +	if (kind == RTNL_KIND_DEL && (nlh->nlmsg_flags & NLM_F_BULK) &&
>> +	    !(flags & RTNL_FLAG_BULK_DEL_SUPPORTED)) {
>> +		NL_SET_ERR_MSG(extack, "Bulk delete is not supported");
>> +		goto err_unlock;
> 
> If a buggy user space application is sending messages with NLM_F_BULK
> set (unintentionally), will it break on newer kernel? I couldn't find
> where the kernel was validating that reserved flags are not used (I
> suspect it doesn't).

Correct, it doesn't.

> 
> Assuming the above is correct and of interest, maybe just emit a warning
> via extack and drop the goto? Alternatively, we can see if anyone
> complains which might never happen
> 

TBH I prefer to error out on an unsupported flag, but I get the problem. These
weren't validated before and we start checking now. The problem is that we'll
return an extack without an error, but the delete might also remove something.
Hrm.. perhaps we can rephrase the error in that case (since it becomes a warning
in iproute2 terms):
 "NLM_F_BULK flag is set but bulk delete operation is not supported"
So it will warn the user it has an unsupported flag.

WDYT ?

IMO we should bite the bullet and keep the error though. :)

>> +	}
>> +
>>  	if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
>>  		doit = link->doit;
>>  		rcu_read_unlock();
>> -- 
>> 2.35.1
>>


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
  2022-04-13 12:20     ` [Bridge] " Ido Schimmel
@ 2022-04-13 12:21       ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 12:21 UTC (permalink / raw)
  To: Ido Schimmel; +Cc: netdev, dsahern, roopa, kuba, davem, bridge

On 13/04/2022 15:20, Ido Schimmel wrote:
> On Wed, Apr 13, 2022 at 01:51:57PM +0300, Nikolay Aleksandrov wrote:
>> When NLM_F_BULK is specified in a fdb del message we need to handle it
>> differently. First since this is a new call we can strictly validate the
>> passed attributes, at first only ifindex and vlan are allowed as these
>> will be the initially supported filter attributes, any other attribute
>> is rejected. The mac address is no longer mandatory, but we use it
>> to error out in older kernels because it cannot be specified with bulk
>> request (the attribute is not allowed) and then we have to dispatch
>> the call to ndo_fdb_del_bulk if the device supports it. The del bulk
>> callback can do further validation of the attributes if necessary.
>>
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>> v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED
>>
>>  net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
>>  1 file changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 63c7df52a667..520d50fcaaea 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
>>  }
>>  EXPORT_SYMBOL(ndo_dflt_fdb_del);
>>  
>> +static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
>> +	[NDA_VLAN]	= { .type = NLA_U16 },
> 
> In earlier versions br_vlan_valid_id() was used to validate the VLAN,
> but I don't see it anymore. Maybe use 
> 
> NLA_POLICY_RANGE(1, VLAN_N_VID - 2)
> 
> ?
> 
> I realize that invalid values won't do anything, but I think it's better
> to only allow valid ranges.
> 

It's already validated below, see fdb_vid_parse().


>> +	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
>> +};
>> +
>>  static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  			struct netlink_ext_ack *extack)
>>  {
>> +	bool del_bulk = !!(nlh->nlmsg_flags & NLM_F_BULK);
>>  	struct net *net = sock_net(skb->sk);
>> +	const struct net_device_ops *ops;
>>  	struct ndmsg *ndm;
>>  	struct nlattr *tb[NDA_MAX+1];
>>  	struct net_device *dev;
>> -	__u8 *addr;
>> +	__u8 *addr = NULL;
>>  	int err;
>>  	u16 vid;
>>  
>>  	if (!netlink_capable(skb, CAP_NET_ADMIN))
>>  		return -EPERM;
>>  
>> -	err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX, NULL,
>> -				     extack);
>> +	if (!del_bulk) {
>> +		err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX,
>> +					     NULL, extack);
>> +	} else {
>> +		err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX,
>> +				  fdb_del_bulk_policy, extack);
>> +	}
>>  	if (err < 0)
>>  		return err;
>>  
>> @@ -4200,9 +4212,12 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  		return -ENODEV;
>>  	}
>>  
>> -	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
>> -		NL_SET_ERR_MSG(extack, "invalid address");
>> -		return -EINVAL;
>> +	if (!del_bulk) {
>> +		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]);
>>  	}
>>  
>>  	if (dev->type != ARPHRD_ETHER) {
>> @@ -4210,8 +4225,6 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  		return -EINVAL;
>>  	}
>>  
>> -	addr = nla_data(tb[NDA_LLADDR]);
>> -
>>  	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
>>  	if (err)
>>  		return err;
>> @@ -4222,10 +4235,16 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>>  	    netif_is_bridge_port(dev)) {
>>  		struct net_device *br_dev = netdev_master_upper_dev_get(dev);
>> -		const struct net_device_ops *ops = br_dev->netdev_ops;
>>  
>> -		if (ops->ndo_fdb_del)
>> -			err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +		ops = br_dev->netdev_ops;
>> +		if (!del_bulk) {
>> +			if (ops->ndo_fdb_del)
>> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +		} else {
>> +			if (ops->ndo_fdb_del_bulk)
>> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
>> +							    extack);
>> +		}
>>  
>>  		if (err)
>>  			goto out;
>> @@ -4235,15 +4254,24 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  
>>  	/* Embedded bridge, macvlan, and any other device support */
>>  	if (ndm->ndm_flags & NTF_SELF) {
>> -		if (dev->netdev_ops->ndo_fdb_del)
>> -			err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr,
>> -							   vid);
>> -		else
>> -			err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
>> +		ops = dev->netdev_ops;
>> +		if (!del_bulk) {
>> +			if (ops->ndo_fdb_del)
>> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +			else
>> +				err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
>> +		} else {
>> +			/* in case err was cleared by NTF_MASTER call */
>> +			err = -EOPNOTSUPP;
>> +			if (ops->ndo_fdb_del_bulk)
>> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
>> +							    extack);
>> +		}
>>  
>>  		if (!err) {
>> -			rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
>> -					ndm->ndm_state);
>> +			if (!del_bulk)
>> +				rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
>> +						ndm->ndm_state);
>>  			ndm->ndm_flags &= ~NTF_SELF;
>>  		}
>>  	}
>> @@ -6145,7 +6173,8 @@ void __init rtnetlink_init(void)
>>  	rtnl_register(PF_UNSPEC, RTM_DELLINKPROP, rtnl_dellinkprop, NULL, 0);
>>  
>>  	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_DELNEIGH, rtnl_fdb_del, NULL,
>> +		      RTNL_FLAG_BULK_DEL_SUPPORTED);
>>  	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
>>  
>>  	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
>> -- 
>> 2.35.1
>>


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
@ 2022-04-13 12:21       ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-04-13 12:21 UTC (permalink / raw)
  To: Ido Schimmel; +Cc: netdev, dsahern, bridge, roopa, kuba, davem

On 13/04/2022 15:20, Ido Schimmel wrote:
> On Wed, Apr 13, 2022 at 01:51:57PM +0300, Nikolay Aleksandrov wrote:
>> When NLM_F_BULK is specified in a fdb del message we need to handle it
>> differently. First since this is a new call we can strictly validate the
>> passed attributes, at first only ifindex and vlan are allowed as these
>> will be the initially supported filter attributes, any other attribute
>> is rejected. The mac address is no longer mandatory, but we use it
>> to error out in older kernels because it cannot be specified with bulk
>> request (the attribute is not allowed) and then we have to dispatch
>> the call to ndo_fdb_del_bulk if the device supports it. The del bulk
>> callback can do further validation of the attributes if necessary.
>>
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>> v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED
>>
>>  net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
>>  1 file changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 63c7df52a667..520d50fcaaea 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
>>  }
>>  EXPORT_SYMBOL(ndo_dflt_fdb_del);
>>  
>> +static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
>> +	[NDA_VLAN]	= { .type = NLA_U16 },
> 
> In earlier versions br_vlan_valid_id() was used to validate the VLAN,
> but I don't see it anymore. Maybe use 
> 
> NLA_POLICY_RANGE(1, VLAN_N_VID - 2)
> 
> ?
> 
> I realize that invalid values won't do anything, but I think it's better
> to only allow valid ranges.
> 

It's already validated below, see fdb_vid_parse().


>> +	[NDA_IFINDEX]	= NLA_POLICY_MIN(NLA_S32, 1),
>> +};
>> +
>>  static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  			struct netlink_ext_ack *extack)
>>  {
>> +	bool del_bulk = !!(nlh->nlmsg_flags & NLM_F_BULK);
>>  	struct net *net = sock_net(skb->sk);
>> +	const struct net_device_ops *ops;
>>  	struct ndmsg *ndm;
>>  	struct nlattr *tb[NDA_MAX+1];
>>  	struct net_device *dev;
>> -	__u8 *addr;
>> +	__u8 *addr = NULL;
>>  	int err;
>>  	u16 vid;
>>  
>>  	if (!netlink_capable(skb, CAP_NET_ADMIN))
>>  		return -EPERM;
>>  
>> -	err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX, NULL,
>> -				     extack);
>> +	if (!del_bulk) {
>> +		err = nlmsg_parse_deprecated(nlh, sizeof(*ndm), tb, NDA_MAX,
>> +					     NULL, extack);
>> +	} else {
>> +		err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX,
>> +				  fdb_del_bulk_policy, extack);
>> +	}
>>  	if (err < 0)
>>  		return err;
>>  
>> @@ -4200,9 +4212,12 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  		return -ENODEV;
>>  	}
>>  
>> -	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
>> -		NL_SET_ERR_MSG(extack, "invalid address");
>> -		return -EINVAL;
>> +	if (!del_bulk) {
>> +		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]);
>>  	}
>>  
>>  	if (dev->type != ARPHRD_ETHER) {
>> @@ -4210,8 +4225,6 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  		return -EINVAL;
>>  	}
>>  
>> -	addr = nla_data(tb[NDA_LLADDR]);
>> -
>>  	err = fdb_vid_parse(tb[NDA_VLAN], &vid, extack);
>>  	if (err)
>>  		return err;
>> @@ -4222,10 +4235,16 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>>  	    netif_is_bridge_port(dev)) {
>>  		struct net_device *br_dev = netdev_master_upper_dev_get(dev);
>> -		const struct net_device_ops *ops = br_dev->netdev_ops;
>>  
>> -		if (ops->ndo_fdb_del)
>> -			err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +		ops = br_dev->netdev_ops;
>> +		if (!del_bulk) {
>> +			if (ops->ndo_fdb_del)
>> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +		} else {
>> +			if (ops->ndo_fdb_del_bulk)
>> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
>> +							    extack);
>> +		}
>>  
>>  		if (err)
>>  			goto out;
>> @@ -4235,15 +4254,24 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  
>>  	/* Embedded bridge, macvlan, and any other device support */
>>  	if (ndm->ndm_flags & NTF_SELF) {
>> -		if (dev->netdev_ops->ndo_fdb_del)
>> -			err = dev->netdev_ops->ndo_fdb_del(ndm, tb, dev, addr,
>> -							   vid);
>> -		else
>> -			err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
>> +		ops = dev->netdev_ops;
>> +		if (!del_bulk) {
>> +			if (ops->ndo_fdb_del)
>> +				err = ops->ndo_fdb_del(ndm, tb, dev, addr, vid);
>> +			else
>> +				err = ndo_dflt_fdb_del(ndm, tb, dev, addr, vid);
>> +		} else {
>> +			/* in case err was cleared by NTF_MASTER call */
>> +			err = -EOPNOTSUPP;
>> +			if (ops->ndo_fdb_del_bulk)
>> +				err = ops->ndo_fdb_del_bulk(ndm, tb, dev, vid,
>> +							    extack);
>> +		}
>>  
>>  		if (!err) {
>> -			rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
>> -					ndm->ndm_state);
>> +			if (!del_bulk)
>> +				rtnl_fdb_notify(dev, addr, vid, RTM_DELNEIGH,
>> +						ndm->ndm_state);
>>  			ndm->ndm_flags &= ~NTF_SELF;
>>  		}
>>  	}
>> @@ -6145,7 +6173,8 @@ void __init rtnetlink_init(void)
>>  	rtnl_register(PF_UNSPEC, RTM_DELLINKPROP, rtnl_dellinkprop, NULL, 0);
>>  
>>  	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_DELNEIGH, rtnl_fdb_del, NULL,
>> +		      RTNL_FLAG_BULK_DEL_SUPPORTED);
>>  	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, rtnl_fdb_get, rtnl_fdb_dump, 0);
>>  
>>  	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);
>> -- 
>> 2.35.1
>>


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
  2022-04-13 12:21       ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-13 12:35         ` Ido Schimmel
  -1 siblings, 0 replies; 46+ messages in thread
From: Ido Schimmel @ 2022-04-13 12:35 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, dsahern, roopa, kuba, davem, bridge

On Wed, Apr 13, 2022 at 03:21:54PM +0300, Nikolay Aleksandrov wrote:
> On 13/04/2022 15:20, Ido Schimmel wrote:
> > On Wed, Apr 13, 2022 at 01:51:57PM +0300, Nikolay Aleksandrov wrote:
> >> When NLM_F_BULK is specified in a fdb del message we need to handle it
> >> differently. First since this is a new call we can strictly validate the
> >> passed attributes, at first only ifindex and vlan are allowed as these
> >> will be the initially supported filter attributes, any other attribute
> >> is rejected. The mac address is no longer mandatory, but we use it
> >> to error out in older kernels because it cannot be specified with bulk
> >> request (the attribute is not allowed) and then we have to dispatch
> >> the call to ndo_fdb_del_bulk if the device supports it. The del bulk
> >> callback can do further validation of the attributes if necessary.
> >>
> >> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> >> ---
> >> v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED
> >>
> >>  net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 48 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> >> index 63c7df52a667..520d50fcaaea 100644
> >> --- a/net/core/rtnetlink.c
> >> +++ b/net/core/rtnetlink.c
> >> @@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
> >>  }
> >>  EXPORT_SYMBOL(ndo_dflt_fdb_del);
> >>  
> >> +static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
> >> +	[NDA_VLAN]	= { .type = NLA_U16 },
> > 
> > In earlier versions br_vlan_valid_id() was used to validate the VLAN,
> > but I don't see it anymore. Maybe use 
> > 
> > NLA_POLICY_RANGE(1, VLAN_N_VID - 2)
> > 
> > ?
> > 
> > I realize that invalid values won't do anything, but I think it's better
> > to only allow valid ranges.
> > 
> 
> It's already validated below, see fdb_vid_parse().

Sorry, missed it :)

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del
@ 2022-04-13 12:35         ` Ido Schimmel
  0 siblings, 0 replies; 46+ messages in thread
From: Ido Schimmel @ 2022-04-13 12:35 UTC (permalink / raw)
  To: Nikolay Aleksandrov; +Cc: netdev, dsahern, bridge, roopa, kuba, davem

On Wed, Apr 13, 2022 at 03:21:54PM +0300, Nikolay Aleksandrov wrote:
> On 13/04/2022 15:20, Ido Schimmel wrote:
> > On Wed, Apr 13, 2022 at 01:51:57PM +0300, Nikolay Aleksandrov wrote:
> >> When NLM_F_BULK is specified in a fdb del message we need to handle it
> >> differently. First since this is a new call we can strictly validate the
> >> passed attributes, at first only ifindex and vlan are allowed as these
> >> will be the initially supported filter attributes, any other attribute
> >> is rejected. The mac address is no longer mandatory, but we use it
> >> to error out in older kernels because it cannot be specified with bulk
> >> request (the attribute is not allowed) and then we have to dispatch
> >> the call to ndo_fdb_del_bulk if the device supports it. The del bulk
> >> callback can do further validation of the attributes if necessary.
> >>
> >> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> >> ---
> >> v4: mark PF_BRIDGE/RTM_DELNEIGH with RTNL_FLAG_BULK_DEL_SUPPORTED
> >>
> >>  net/core/rtnetlink.c | 67 +++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 48 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> >> index 63c7df52a667..520d50fcaaea 100644
> >> --- a/net/core/rtnetlink.c
> >> +++ b/net/core/rtnetlink.c
> >> @@ -4169,22 +4169,34 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
> >>  }
> >>  EXPORT_SYMBOL(ndo_dflt_fdb_del);
> >>  
> >> +static const struct nla_policy fdb_del_bulk_policy[NDA_MAX + 1] = {
> >> +	[NDA_VLAN]	= { .type = NLA_U16 },
> > 
> > In earlier versions br_vlan_valid_id() was used to validate the VLAN,
> > but I don't see it anymore. Maybe use 
> > 
> > NLA_POLICY_RANGE(1, VLAN_N_VID - 2)
> > 
> > ?
> > 
> > I realize that invalid values won't do anything, but I think it's better
> > to only allow valid ranges.
> > 
> 
> It's already validated below, see fdb_vid_parse().

Sorry, missed it :)

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag
  2022-04-13 12:21       ` [Bridge] " Nikolay Aleksandrov
@ 2022-04-14  0:42         ` David Ahern
  -1 siblings, 0 replies; 46+ messages in thread
From: David Ahern @ 2022-04-14  0:42 UTC (permalink / raw)
  To: Nikolay Aleksandrov, Ido Schimmel; +Cc: netdev, roopa, kuba, davem, bridge

On 4/13/22 6:21 AM, Nikolay Aleksandrov wrote:
>> If a buggy user space application is sending messages with NLM_F_BULK
>> set (unintentionally), will it break on newer kernel? I couldn't find
>> where the kernel was validating that reserved flags are not used (I
>> suspect it doesn't).
> 
> Correct, it doesn't.
> 
>>
>> Assuming the above is correct and of interest, maybe just emit a warning
>> via extack and drop the goto? Alternatively, we can see if anyone
>> complains which might never happen
>>
> 
> TBH I prefer to error out on an unsupported flag, but I get the problem. These
> weren't validated before and we start checking now. The problem is that we'll
> return an extack without an error, but the delete might also remove something.
> Hrm.. perhaps we can rephrase the error in that case (since it becomes a warning
> in iproute2 terms):
>  "NLM_F_BULK flag is set but bulk delete operation is not supported"
> So it will warn the user it has an unsupported flag.
> 
> WDYT ?
> 
> IMO we should bite the bullet and keep the error though. :)
> 

I agree. The check across the board for BULK flag on any DELETE requests
should tell us pretty quick if someone is setting that flag when it
should not be.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag
@ 2022-04-14  0:42         ` David Ahern
  0 siblings, 0 replies; 46+ messages in thread
From: David Ahern @ 2022-04-14  0:42 UTC (permalink / raw)
  To: Nikolay Aleksandrov, Ido Schimmel; +Cc: netdev, bridge, davem, kuba, roopa

On 4/13/22 6:21 AM, Nikolay Aleksandrov wrote:
>> If a buggy user space application is sending messages with NLM_F_BULK
>> set (unintentionally), will it break on newer kernel? I couldn't find
>> where the kernel was validating that reserved flags are not used (I
>> suspect it doesn't).
> 
> Correct, it doesn't.
> 
>>
>> Assuming the above is correct and of interest, maybe just emit a warning
>> via extack and drop the goto? Alternatively, we can see if anyone
>> complains which might never happen
>>
> 
> TBH I prefer to error out on an unsupported flag, but I get the problem. These
> weren't validated before and we start checking now. The problem is that we'll
> return an extack without an error, but the delete might also remove something.
> Hrm.. perhaps we can rephrase the error in that case (since it becomes a warning
> in iproute2 terms):
>  "NLM_F_BULK flag is set but bulk delete operation is not supported"
> So it will warn the user it has an unsupported flag.
> 
> WDYT ?
> 
> IMO we should bite the bullet and keep the error though. :)
> 

I agree. The check across the board for BULK flag on any DELETE requests
should tell us pretty quick if someone is setting that flag when it
should not be.

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier
  2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
@ 2022-09-20  7:49     ` Nicolas Dichtel
  -1 siblings, 0 replies; 46+ messages in thread
From: Nicolas Dichtel @ 2022-09-20  7:49 UTC (permalink / raw)
  To: Nikolay Aleksandrov, netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge


Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit :
> Add a new delete request modifier called NLM_F_BULK which, when
> supported, would cause the request to delete multiple objects. The flag
> is a convenient way to signal that a multiple delete operation is
> requested which can be gradually added to different delete requests. In
> order to make sure older kernels will error out if the operation is not
> supported instead of doing something unintended we have to break a
> required condition when implementing support for this flag, f.e. for
> neighbors we will omit the mandatory mac address attribute.
> Initially it will be used to add flush with filtering support for bridge
> fdbs, but it also opens the door to add similar support to others.
> 
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>  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 4c0cde075c27..855dffb4c1c3 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -72,6 +72,7 @@ struct nlmsghdr {
>  
>  /* Modifiers to DELETE request */
>  #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
> +#define NLM_F_BULK	0x200	/* Delete multiple objects	*/
Sorry to reply to an old patch, but FWIW, this patch broke the uAPI.
One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is
conceptually wrong but it was working. After this patch, the kernel returns an
error (EOPNOTSUPP).

Here is the patch series:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b

We probably can't do anything now, but to avoid this in the future, I see only
two options:
 - enforce flags validation depending on the operation (but this may break some
   existing apps)
 - stop adding new flags that overlap between NEW and DEL operations (by adding
   a comment or defining dummy flags).

Any thoughts?

Regards,
Nicolas

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier
@ 2022-09-20  7:49     ` Nicolas Dichtel
  0 siblings, 0 replies; 46+ messages in thread
From: Nicolas Dichtel @ 2022-09-20  7:49 UTC (permalink / raw)
  To: Nikolay Aleksandrov, netdev; +Cc: dsahern, bridge, idosch, roopa, kuba, davem


Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit :
> Add a new delete request modifier called NLM_F_BULK which, when
> supported, would cause the request to delete multiple objects. The flag
> is a convenient way to signal that a multiple delete operation is
> requested which can be gradually added to different delete requests. In
> order to make sure older kernels will error out if the operation is not
> supported instead of doing something unintended we have to break a
> required condition when implementing support for this flag, f.e. for
> neighbors we will omit the mandatory mac address attribute.
> Initially it will be used to add flush with filtering support for bridge
> fdbs, but it also opens the door to add similar support to others.
> 
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>  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 4c0cde075c27..855dffb4c1c3 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -72,6 +72,7 @@ struct nlmsghdr {
>  
>  /* Modifiers to DELETE request */
>  #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
> +#define NLM_F_BULK	0x200	/* Delete multiple objects	*/
Sorry to reply to an old patch, but FWIW, this patch broke the uAPI.
One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is
conceptually wrong but it was working. After this patch, the kernel returns an
error (EOPNOTSUPP).

Here is the patch series:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b

We probably can't do anything now, but to avoid this in the future, I see only
two options:
 - enforce flags validation depending on the operation (but this may break some
   existing apps)
 - stop adding new flags that overlap between NEW and DEL operations (by adding
   a comment or defining dummy flags).

Any thoughts?

Regards,
Nicolas

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier
  2022-09-20  7:49     ` [Bridge] " Nicolas Dichtel
@ 2022-09-20  9:05       ` Nikolay Aleksandrov
  -1 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-09-20  9:05 UTC (permalink / raw)
  To: nicolas.dichtel, netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge

On 20/09/2022 10:49, Nicolas Dichtel wrote:
> 
> Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit :
>> Add a new delete request modifier called NLM_F_BULK which, when
>> supported, would cause the request to delete multiple objects. The flag
>> is a convenient way to signal that a multiple delete operation is
>> requested which can be gradually added to different delete requests. In
>> order to make sure older kernels will error out if the operation is not
>> supported instead of doing something unintended we have to break a
>> required condition when implementing support for this flag, f.e. for
>> neighbors we will omit the mandatory mac address attribute.
>> Initially it will be used to add flush with filtering support for bridge
>> fdbs, but it also opens the door to add similar support to others.
>>
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>>  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 4c0cde075c27..855dffb4c1c3 100644
>> --- a/include/uapi/linux/netlink.h
>> +++ b/include/uapi/linux/netlink.h
>> @@ -72,6 +72,7 @@ struct nlmsghdr {
>>  
>>  /* Modifiers to DELETE request */
>>  #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
>> +#define NLM_F_BULK	0x200	/* Delete multiple objects	*/
> Sorry to reply to an old patch, but FWIW, this patch broke the uAPI.
> One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is
> conceptually wrong but it was working. After this patch, the kernel returns an
> error (EOPNOTSUPP).
> 
> Here is the patch series:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b
> 
> We probably can't do anything now, but to avoid this in the future, I see only
> two options:
>  - enforce flags validation depending on the operation (but this may break some
>    existing apps)
>  - stop adding new flags that overlap between NEW and DEL operations (by adding
>    a comment or defining dummy flags).
> 
> Any thoughts?
> 

Personally I'd prefer to enforce validation so we don't lose the flags because of buggy user-space
applications, but we can break someone (who arguably should fix their app though). We already had
that discussion while the set was under review[1] and just to be a bit more confident I also
tried searching for open-source buggy users, but didn't find any.

> Regards,
> Nicolas

[1] https://lore.kernel.org/netdev/97774474-65a3-fa45-e0b9-8db6c748da28@kernel.org/t/#m23018ce831dae16d42cb9c393c7c6bad1bc621c3

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier
@ 2022-09-20  9:05       ` Nikolay Aleksandrov
  0 siblings, 0 replies; 46+ messages in thread
From: Nikolay Aleksandrov @ 2022-09-20  9:05 UTC (permalink / raw)
  To: nicolas.dichtel, netdev; +Cc: dsahern, bridge, idosch, roopa, kuba, davem

On 20/09/2022 10:49, Nicolas Dichtel wrote:
> 
> Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit :
>> Add a new delete request modifier called NLM_F_BULK which, when
>> supported, would cause the request to delete multiple objects. The flag
>> is a convenient way to signal that a multiple delete operation is
>> requested which can be gradually added to different delete requests. In
>> order to make sure older kernels will error out if the operation is not
>> supported instead of doing something unintended we have to break a
>> required condition when implementing support for this flag, f.e. for
>> neighbors we will omit the mandatory mac address attribute.
>> Initially it will be used to add flush with filtering support for bridge
>> fdbs, but it also opens the door to add similar support to others.
>>
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>>  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 4c0cde075c27..855dffb4c1c3 100644
>> --- a/include/uapi/linux/netlink.h
>> +++ b/include/uapi/linux/netlink.h
>> @@ -72,6 +72,7 @@ struct nlmsghdr {
>>  
>>  /* Modifiers to DELETE request */
>>  #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
>> +#define NLM_F_BULK	0x200	/* Delete multiple objects	*/
> Sorry to reply to an old patch, but FWIW, this patch broke the uAPI.
> One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is
> conceptually wrong but it was working. After this patch, the kernel returns an
> error (EOPNOTSUPP).
> 
> Here is the patch series:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b
> 
> We probably can't do anything now, but to avoid this in the future, I see only
> two options:
>  - enforce flags validation depending on the operation (but this may break some
>    existing apps)
>  - stop adding new flags that overlap between NEW and DEL operations (by adding
>    a comment or defining dummy flags).
> 
> Any thoughts?
> 

Personally I'd prefer to enforce validation so we don't lose the flags because of buggy user-space
applications, but we can break someone (who arguably should fix their app though). We already had
that discussion while the set was under review[1] and just to be a bit more confident I also
tried searching for open-source buggy users, but didn't find any.

> Regards,
> Nicolas

[1] https://lore.kernel.org/netdev/97774474-65a3-fa45-e0b9-8db6c748da28@kernel.org/t/#m23018ce831dae16d42cb9c393c7c6bad1bc621c3

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier
  2022-09-20  9:05       ` [Bridge] " Nikolay Aleksandrov
@ 2022-09-21  6:43         ` Nicolas Dichtel
  -1 siblings, 0 replies; 46+ messages in thread
From: Nicolas Dichtel @ 2022-09-21  6:43 UTC (permalink / raw)
  To: Nikolay Aleksandrov, netdev; +Cc: dsahern, roopa, idosch, kuba, davem, bridge


Le 20/09/2022 à 11:05, Nikolay Aleksandrov a écrit :
> On 20/09/2022 10:49, Nicolas Dichtel wrote:
>>
>> Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit :
>>> Add a new delete request modifier called NLM_F_BULK which, when
>>> supported, would cause the request to delete multiple objects. The flag
>>> is a convenient way to signal that a multiple delete operation is
>>> requested which can be gradually added to different delete requests. In
>>> order to make sure older kernels will error out if the operation is not
>>> supported instead of doing something unintended we have to break a
>>> required condition when implementing support for this flag, f.e. for
>>> neighbors we will omit the mandatory mac address attribute.
>>> Initially it will be used to add flush with filtering support for bridge
>>> fdbs, but it also opens the door to add similar support to others.
>>>
>>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>>> ---
>>>  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 4c0cde075c27..855dffb4c1c3 100644
>>> --- a/include/uapi/linux/netlink.h
>>> +++ b/include/uapi/linux/netlink.h
>>> @@ -72,6 +72,7 @@ struct nlmsghdr {
>>>  
>>>  /* Modifiers to DELETE request */
>>>  #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
>>> +#define NLM_F_BULK	0x200	/* Delete multiple objects	*/
>> Sorry to reply to an old patch, but FWIW, this patch broke the uAPI.
>> One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is
>> conceptually wrong but it was working. After this patch, the kernel returns an
>> error (EOPNOTSUPP).
>>
>> Here is the patch series:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b
>>
>> We probably can't do anything now, but to avoid this in the future, I see only
>> two options:
>>  - enforce flags validation depending on the operation (but this may break some
>>    existing apps)
>>  - stop adding new flags that overlap between NEW and DEL operations (by adding
>>    a comment or defining dummy flags).
>>
>> Any thoughts?
>>
> 
> Personally I'd prefer to enforce validation so we don't lose the flags because of buggy user-space
> applications, but we can break someone (who arguably should fix their app though). We already had
> that discussion while the set was under review[1] and just to be a bit more confident I also
Thanks for the link. Finally, someone has (almost) complained :D

> tried searching for open-source buggy users, but didn't find any.
The trend seems to let someone else add another specific flag if needed. Thus,
it seems that checking flags is the way to go.
The pro is that if someone complains, the patch could be reverted, which is not
the case for a new feature like this bulk for example.


Regards,
Nicolas

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [Bridge] [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier
@ 2022-09-21  6:43         ` Nicolas Dichtel
  0 siblings, 0 replies; 46+ messages in thread
From: Nicolas Dichtel @ 2022-09-21  6:43 UTC (permalink / raw)
  To: Nikolay Aleksandrov, netdev; +Cc: dsahern, bridge, idosch, roopa, kuba, davem


Le 20/09/2022 à 11:05, Nikolay Aleksandrov a écrit :
> On 20/09/2022 10:49, Nicolas Dichtel wrote:
>>
>> Le 13/04/2022 à 12:51, Nikolay Aleksandrov a écrit :
>>> Add a new delete request modifier called NLM_F_BULK which, when
>>> supported, would cause the request to delete multiple objects. The flag
>>> is a convenient way to signal that a multiple delete operation is
>>> requested which can be gradually added to different delete requests. In
>>> order to make sure older kernels will error out if the operation is not
>>> supported instead of doing something unintended we have to break a
>>> required condition when implementing support for this flag, f.e. for
>>> neighbors we will omit the mandatory mac address attribute.
>>> Initially it will be used to add flush with filtering support for bridge
>>> fdbs, but it also opens the door to add similar support to others.
>>>
>>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>>> ---
>>>  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 4c0cde075c27..855dffb4c1c3 100644
>>> --- a/include/uapi/linux/netlink.h
>>> +++ b/include/uapi/linux/netlink.h
>>> @@ -72,6 +72,7 @@ struct nlmsghdr {
>>>  
>>>  /* Modifiers to DELETE request */
>>>  #define NLM_F_NONREC	0x100	/* Do not delete recursively	*/
>>> +#define NLM_F_BULK	0x200	/* Delete multiple objects	*/
>> Sorry to reply to an old patch, but FWIW, this patch broke the uAPI.
>> One of our applications was using NLM_F_EXCL with RTM_DELTFILTER. This is
>> conceptually wrong but it was working. After this patch, the kernel returns an
>> error (EOPNOTSUPP).
>>
>> Here is the patch series:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=92716869375b
>>
>> We probably can't do anything now, but to avoid this in the future, I see only
>> two options:
>>  - enforce flags validation depending on the operation (but this may break some
>>    existing apps)
>>  - stop adding new flags that overlap between NEW and DEL operations (by adding
>>    a comment or defining dummy flags).
>>
>> Any thoughts?
>>
> 
> Personally I'd prefer to enforce validation so we don't lose the flags because of buggy user-space
> applications, but we can break someone (who arguably should fix their app though). We already had
> that discussion while the set was under review[1] and just to be a bit more confident I also
Thanks for the link. Finally, someone has (almost) complained :D

> tried searching for open-source buggy users, but didn't find any.
The trend seems to let someone else add another specific flag if needed. Thus,
it seems that checking flags is the way to go.
The pro is that if someone complains, the patch could be reverted, which is not
the case for a new feature like this bulk for example.


Regards,
Nicolas

^ permalink raw reply	[flat|nested] 46+ messages in thread

end of thread, other threads:[~2022-09-21  6:43 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-13 10:51 [PATCH net-next v4 00/12] net: bridge: add flush filtering support Nikolay Aleksandrov
2022-04-13 10:51 ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:51 ` [PATCH net-next v4 01/12] net: rtnetlink: add msg kind names Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:51 ` [PATCH net-next v4 02/12] net: rtnetlink: add helper to extract msg type's kind Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:51 ` [PATCH net-next v4 03/12] net: rtnetlink: use BIT for flag values Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:51 ` [PATCH net-next v4 04/12] net: netlink: add NLM_F_BULK delete request modifier Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-09-20  7:49   ` Nicolas Dichtel
2022-09-20  7:49     ` [Bridge] " Nicolas Dichtel
2022-09-20  9:05     ` Nikolay Aleksandrov
2022-09-20  9:05       ` [Bridge] " Nikolay Aleksandrov
2022-09-21  6:43       ` Nicolas Dichtel
2022-09-21  6:43         ` [Bridge] " Nicolas Dichtel
2022-04-13 10:51 ` [PATCH net-next v4 05/12] net: rtnetlink: add bulk delete support flag Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 12:06   ` Ido Schimmel
2022-04-13 12:06     ` [Bridge] " Ido Schimmel
2022-04-13 12:21     ` Nikolay Aleksandrov
2022-04-13 12:21       ` [Bridge] " Nikolay Aleksandrov
2022-04-14  0:42       ` David Ahern
2022-04-14  0:42         ` [Bridge] " David Ahern
2022-04-13 10:51 ` [PATCH net-next v4 06/12] net: add ndo_fdb_del_bulk Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:51 ` [PATCH net-next v4 07/12] net: rtnetlink: add NLM_F_BULK support to rtnl_fdb_del Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 12:20   ` Ido Schimmel
2022-04-13 12:20     ` [Bridge] " Ido Schimmel
2022-04-13 12:21     ` Nikolay Aleksandrov
2022-04-13 12:21       ` [Bridge] " Nikolay Aleksandrov
2022-04-13 12:35       ` Ido Schimmel
2022-04-13 12:35         ` [Bridge] " Ido Schimmel
2022-04-13 10:51 ` [PATCH net-next v4 08/12] net: bridge: fdb: add ndo_fdb_del_bulk Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:51 ` [PATCH net-next v4 09/12] net: bridge: fdb: add support for fine-grained flushing Nikolay Aleksandrov
2022-04-13 10:51   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:52 ` [PATCH net-next v4 10/12] net: rtnetlink: add ndm flags and state mask attributes Nikolay Aleksandrov
2022-04-13 10:52   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:52 ` [PATCH net-next v4 11/12] net: bridge: fdb: add support for flush filtering based on ndm flags and state Nikolay Aleksandrov
2022-04-13 10:52   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 10:52 ` [PATCH net-next v4 12/12] net: bridge: fdb: add support for flush filtering based on ifindex and vlan Nikolay Aleksandrov
2022-04-13 10:52   ` [Bridge] " Nikolay Aleksandrov
2022-04-13 11:50 ` [PATCH net-next v4 00/12] net: bridge: add flush filtering support patchwork-bot+netdevbpf
2022-04-13 11:50   ` [Bridge] " patchwork-bot+netdevbpf

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.