* [PATCH net-next 0/6] net: bridge: factorize MDB new and del functions
@ 2017-05-17 21:27 Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 1/6] net: bridge: pass net_bridge_port to __br_mdb_add Vivien Didelot
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Vivien Didelot @ 2017-05-17 21:27 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, kernel, David S. Miller, Vivien Didelot
The br_mdb.c file implements both br_mdb_add and br_mdb_del functions, to
handle respectively the RTM_NEWMDB and RTM_DELMDB message types.
But these two functions are really similar. This patch series factorizes
them into a single br_mdb_do rtnl_doit_func function to reduce
boilerplate and simplify the MDB handling code.
Vivien Didelot (6):
net: bridge: pass net_bridge_port to __br_mdb_add
net: bridge: check multicast bridge only once
net: bridge: break if __br_mdb_del fails
net: bridge: add __br_mdb_do
net: bridge: get msgtype from nlmsghdr in mdb ops
net: bridge: add br_mdb_do
net/bridge/br_mdb.c | 107 +++++++++++++++-------------------------------------
1 file changed, 31 insertions(+), 76 deletions(-)
--
2.13.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next 1/6] net: bridge: pass net_bridge_port to __br_mdb_add
2017-05-17 21:27 [PATCH net-next 0/6] net: bridge: factorize MDB new and del functions Vivien Didelot
@ 2017-05-17 21:27 ` Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 2/6] net: bridge: check multicast bridge only once Vivien Didelot
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Vivien Didelot @ 2017-05-17 21:27 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Vivien Didelot,
Stephen Hemminger, moderated list:ETHERNET BRIDGE
The __br_mdb_add function uses exactly the same mechanism as its caller
br_mdb_add to retrieve the net_bridge_port pointer from br_mdb_entry.
Remove it and pass directly the net_bridge_port pointer to __br_mdb_add.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/bridge/br_mdb.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index b0845480a3ae..bef0331f46a4 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -542,25 +542,15 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
return 0;
}
-static int __br_mdb_add(struct net *net, struct net_bridge *br,
- struct br_mdb_entry *entry)
+static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
{
+ struct net_bridge *br = p->br;
struct br_ip ip;
- struct net_device *dev;
- struct net_bridge_port *p;
int ret;
if (!netif_running(br->dev) || br->multicast_disabled)
return -EINVAL;
- dev = __dev_get_by_index(net, entry->ifindex);
- if (!dev)
- return -ENODEV;
-
- p = br_port_get_rtnl(dev);
- if (!p || p->br != br || p->state == BR_STATE_DISABLED)
- return -EINVAL;
-
__mdb_entry_to_br_ip(entry, &ip);
spin_lock_bh(&br->multicast_lock);
@@ -602,13 +592,13 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, &vg->vlan_list, vlist) {
entry->vid = v->vid;
- err = __br_mdb_add(net, br, entry);
+ err = __br_mdb_add(p, entry);
if (err)
break;
__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
}
} else {
- err = __br_mdb_add(net, br, entry);
+ err = __br_mdb_add(p, entry);
if (!err)
__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
}
--
2.13.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next 2/6] net: bridge: check multicast bridge only once
2017-05-17 21:27 [PATCH net-next 0/6] net: bridge: factorize MDB new and del functions Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 1/6] net: bridge: pass net_bridge_port to __br_mdb_add Vivien Didelot
@ 2017-05-17 21:27 ` Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails Vivien Didelot
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Vivien Didelot @ 2017-05-17 21:27 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Vivien Didelot,
Stephen Hemminger, moderated list:ETHERNET BRIDGE
__br_mdb_add and __br_mdb_del both check that the bridge is up and
running and that multicast is enabled on it before doing any operation.
Since they can be called multiple times by br_mdb_add and br_mdb_del,
move the check in their caller functions so that it is done just once.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/bridge/br_mdb.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index bef0331f46a4..d20a01622b20 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -548,9 +548,6 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
struct br_ip ip;
int ret;
- if (!netif_running(br->dev) || br->multicast_disabled)
- return -EINVAL;
-
__mdb_entry_to_br_ip(entry, &ip);
spin_lock_bh(&br->multicast_lock);
@@ -577,6 +574,9 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
br = netdev_priv(dev);
+ if (!netif_running(br->dev) || br->multicast_disabled)
+ return -EINVAL;
+
/* If vlan filtering is enabled and VLAN is not specified
* install mdb entry on all vlans configured on the port.
*/
@@ -615,9 +615,6 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
struct br_ip ip;
int err = -EINVAL;
- if (!netif_running(br->dev) || br->multicast_disabled)
- return -EINVAL;
-
__mdb_entry_to_br_ip(entry, &ip);
spin_lock_bh(&br->multicast_lock);
@@ -672,6 +669,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
br = netdev_priv(dev);
+ if (!netif_running(br->dev) || br->multicast_disabled)
+ return -EINVAL;
+
/* If vlan filtering is enabled and VLAN is not specified
* delete mdb entry on all vlans configured on the port.
*/
--
2.13.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails
2017-05-17 21:27 [PATCH net-next 0/6] net: bridge: factorize MDB new and del functions Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 1/6] net: bridge: pass net_bridge_port to __br_mdb_add Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 2/6] net: bridge: check multicast bridge only once Vivien Didelot
@ 2017-05-17 21:27 ` Vivien Didelot
2017-05-18 13:45 ` Nikolay Aleksandrov
2017-05-17 21:27 ` [PATCH net-next 4/6] net: bridge: add __br_mdb_do Vivien Didelot
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Vivien Didelot @ 2017-05-17 21:27 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Vivien Didelot,
Stephen Hemminger, moderated list:ETHERNET BRIDGE
Be symmetric with br_mdb_add and break if __br_mdb_del returns an error.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/bridge/br_mdb.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index d20a01622b20..24eeeefb4179 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -688,8 +688,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
list_for_each_entry(v, &vg->vlan_list, vlist) {
entry->vid = v->vid;
err = __br_mdb_del(br, entry);
- if (!err)
- __br_mdb_notify(dev, p, entry, RTM_DELMDB);
+ if (err)
+ break;
+ __br_mdb_notify(dev, p, entry, RTM_DELMDB);
}
} else {
err = __br_mdb_del(br, entry);
--
2.13.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next 4/6] net: bridge: add __br_mdb_do
2017-05-17 21:27 [PATCH net-next 0/6] net: bridge: factorize MDB new and del functions Vivien Didelot
` (2 preceding siblings ...)
2017-05-17 21:27 ` [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails Vivien Didelot
@ 2017-05-17 21:27 ` Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 6/6] net: bridge: add br_mdb_do Vivien Didelot
5 siblings, 0 replies; 12+ messages in thread
From: Vivien Didelot @ 2017-05-17 21:27 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Vivien Didelot,
Stephen Hemminger, moderated list:ETHERNET BRIDGE
br_mdb_add and br_mdb_del call respectively __br_mdb_add and
__br_mdb_del and notify the message type on success.
Factorize this in a new __br_mdb_do function which add or delete a
br_mdb_entry depending on the message type, then notify it.
Note that the dev argument is p->br->dev, so no need to pass it twice.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/bridge/br_mdb.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 24eeeefb4179..a72d5e6f339f 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -556,6 +556,9 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
return ret;
}
+static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
+ int msgtype);
+
static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
struct netlink_ext_ack *extack)
{
@@ -592,15 +595,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, &vg->vlan_list, vlist) {
entry->vid = v->vid;
- err = __br_mdb_add(p, entry);
+ err = __br_mdb_do(p, entry, RTM_NEWMDB);
if (err)
break;
- __br_mdb_notify(dev, p, entry, RTM_NEWMDB);
}
} else {
- err = __br_mdb_add(p, entry);
- if (!err)
- __br_mdb_notify(dev, p, entry, RTM_NEWMDB);
+ err = __br_mdb_do(p, entry, RTM_NEWMDB);
}
return err;
@@ -651,6 +651,22 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
return err;
}
+static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
+ int msgtype)
+{
+ int err = -EINVAL;
+
+ if (msgtype == RTM_NEWMDB)
+ err = __br_mdb_add(p, entry);
+ else if (msgtype == RTM_DELMDB)
+ err = __br_mdb_del(p->br, entry);
+
+ if (!err)
+ __br_mdb_notify(p->br->dev, p, entry, msgtype);
+
+ return err;
+}
+
static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
struct netlink_ext_ack *extack)
{
@@ -687,15 +703,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, &vg->vlan_list, vlist) {
entry->vid = v->vid;
- err = __br_mdb_del(br, entry);
+ err = __br_mdb_do(p, entry, RTM_DELMDB);
if (err)
break;
- __br_mdb_notify(dev, p, entry, RTM_DELMDB);
}
} else {
- err = __br_mdb_del(br, entry);
- if (!err)
- __br_mdb_notify(dev, p, entry, RTM_DELMDB);
+ err = __br_mdb_do(p, entry, RTM_DELMDB);
}
return err;
--
2.13.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops
2017-05-17 21:27 [PATCH net-next 0/6] net: bridge: factorize MDB new and del functions Vivien Didelot
` (3 preceding siblings ...)
2017-05-17 21:27 ` [PATCH net-next 4/6] net: bridge: add __br_mdb_do Vivien Didelot
@ 2017-05-17 21:27 ` Vivien Didelot
2017-05-18 15:28 ` Nikolay Aleksandrov
2017-05-17 21:27 ` [PATCH net-next 6/6] net: bridge: add br_mdb_do Vivien Didelot
5 siblings, 1 reply; 12+ messages in thread
From: Vivien Didelot @ 2017-05-17 21:27 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Vivien Didelot,
Stephen Hemminger, moderated list:ETHERNET BRIDGE
Retrieve the message type from the nlmsghdr structure instead of
hardcoding it in both br_mdb_add and br_mdb_del.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/bridge/br_mdb.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index a72d5e6f339f..d280b20587cb 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -569,6 +569,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
struct net_bridge_port *p;
struct net_bridge_vlan *v;
struct net_bridge *br;
+ int msgtype = nlh->nlmsg_type;
int err;
err = br_mdb_parse(skb, nlh, &dev, &entry);
@@ -595,12 +596,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, &vg->vlan_list, vlist) {
entry->vid = v->vid;
- err = __br_mdb_do(p, entry, RTM_NEWMDB);
+ err = __br_mdb_do(p, entry, msgtype);
if (err)
break;
}
} else {
- err = __br_mdb_do(p, entry, RTM_NEWMDB);
+ err = __br_mdb_do(p, entry, msgtype);
}
return err;
@@ -677,6 +678,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
struct net_bridge_port *p;
struct net_bridge_vlan *v;
struct net_bridge *br;
+ int msgtype = nlh->nlmsg_type;
int err;
err = br_mdb_parse(skb, nlh, &dev, &entry);
@@ -703,12 +705,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, &vg->vlan_list, vlist) {
entry->vid = v->vid;
- err = __br_mdb_do(p, entry, RTM_DELMDB);
+ err = __br_mdb_do(p, entry, msgtype);
if (err)
break;
}
} else {
- err = __br_mdb_do(p, entry, RTM_DELMDB);
+ err = __br_mdb_do(p, entry, msgtype);
}
return err;
--
2.13.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next 6/6] net: bridge: add br_mdb_do
2017-05-17 21:27 [PATCH net-next 0/6] net: bridge: factorize MDB new and del functions Vivien Didelot
` (4 preceding siblings ...)
2017-05-17 21:27 ` [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops Vivien Didelot
@ 2017-05-17 21:27 ` Vivien Didelot
5 siblings, 0 replies; 12+ messages in thread
From: Vivien Didelot @ 2017-05-17 21:27 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Vivien Didelot,
Stephen Hemminger, moderated list:ETHERNET BRIDGE
Now that the two functions br_mdb_add and br_mdb_del are identical, keep
a single function named br_mdb_do and register it for both RTM_NEWMDB
and RTM_DELMDB message types.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
net/bridge/br_mdb.c | 61 +++++------------------------------------------------
1 file changed, 5 insertions(+), 56 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index d280b20587cb..1a1da25f3727 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -556,57 +556,6 @@ static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
return ret;
}
-static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
- int msgtype);
-
-static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
- struct netlink_ext_ack *extack)
-{
- struct net *net = sock_net(skb->sk);
- struct net_bridge_vlan_group *vg;
- struct net_device *dev, *pdev;
- struct br_mdb_entry *entry;
- struct net_bridge_port *p;
- struct net_bridge_vlan *v;
- struct net_bridge *br;
- int msgtype = nlh->nlmsg_type;
- int err;
-
- err = br_mdb_parse(skb, nlh, &dev, &entry);
- if (err < 0)
- return err;
-
- br = netdev_priv(dev);
-
- if (!netif_running(br->dev) || br->multicast_disabled)
- return -EINVAL;
-
- /* If vlan filtering is enabled and VLAN is not specified
- * install mdb entry on all vlans configured on the port.
- */
- pdev = __dev_get_by_index(net, entry->ifindex);
- if (!pdev)
- return -ENODEV;
-
- p = br_port_get_rtnl(pdev);
- if (!p || p->br != br || p->state == BR_STATE_DISABLED)
- return -EINVAL;
-
- vg = nbp_vlan_group(p);
- if (br_vlan_enabled(br) && vg && entry->vid == 0) {
- list_for_each_entry(v, &vg->vlan_list, vlist) {
- entry->vid = v->vid;
- err = __br_mdb_do(p, entry, msgtype);
- if (err)
- break;
- }
- } else {
- err = __br_mdb_do(p, entry, msgtype);
- }
-
- return err;
-}
-
static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
{
struct net_bridge_mdb_htable *mdb;
@@ -668,8 +617,8 @@ static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
return err;
}
-static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
- struct netlink_ext_ack *extack)
+static int br_mdb_do(struct sk_buff *skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
{
struct net *net = sock_net(skb->sk);
struct net_bridge_vlan_group *vg;
@@ -691,7 +640,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
return -EINVAL;
/* If vlan filtering is enabled and VLAN is not specified
- * delete mdb entry on all vlans configured on the port.
+ * add or delete mdb entry on all vlans configured on the port.
*/
pdev = __dev_get_by_index(net, entry->ifindex);
if (!pdev)
@@ -719,8 +668,8 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
void br_mdb_init(void)
{
rtnl_register(PF_BRIDGE, RTM_GETMDB, NULL, br_mdb_dump, NULL);
- rtnl_register(PF_BRIDGE, RTM_NEWMDB, br_mdb_add, NULL, NULL);
- rtnl_register(PF_BRIDGE, RTM_DELMDB, br_mdb_del, NULL, NULL);
+ rtnl_register(PF_BRIDGE, RTM_NEWMDB, br_mdb_do, NULL, NULL);
+ rtnl_register(PF_BRIDGE, RTM_DELMDB, br_mdb_do, NULL, NULL);
}
void br_mdb_uninit(void)
--
2.13.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails
2017-05-17 21:27 ` [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails Vivien Didelot
@ 2017-05-18 13:45 ` Nikolay Aleksandrov
2017-05-18 15:08 ` Vivien Didelot
0 siblings, 1 reply; 12+ messages in thread
From: Nikolay Aleksandrov @ 2017-05-18 13:45 UTC (permalink / raw)
To: Vivien Didelot, netdev
Cc: linux-kernel, kernel, David S. Miller, Stephen Hemminger,
moderated list:ETHERNET BRIDGE
On 5/18/17 12:27 AM, Vivien Didelot wrote:
> Be symmetric with br_mdb_add and break if __br_mdb_del returns an error.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
> net/bridge/br_mdb.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index d20a01622b20..24eeeefb4179 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -688,8 +688,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
> list_for_each_entry(v, &vg->vlan_list, vlist) {
> entry->vid = v->vid;
> err = __br_mdb_del(br, entry);
> - if (!err)
> - __br_mdb_notify(dev, p, entry, RTM_DELMDB);
> + if (err)
> + break;
> + __br_mdb_notify(dev, p, entry, RTM_DELMDB);
> }
> } else {
> err = __br_mdb_del(br, entry);
>
This can potentially break user-space scripts that rely on the best-effort
behaviour, this is the normal "delete without vid & enabled vlan filtering".
You can check the fdb delete code which does the same, this was intentional.
You can add an mdb entry without a vid to all vlans, add a vlan and then try
to remove it from all vlans where it is present - with this patch obviously
that will fail at the new vlan.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails
2017-05-18 13:45 ` Nikolay Aleksandrov
@ 2017-05-18 15:08 ` Vivien Didelot
2017-05-18 15:25 ` Nikolay Aleksandrov
0 siblings, 1 reply; 12+ messages in thread
From: Vivien Didelot @ 2017-05-18 15:08 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev
Cc: linux-kernel, kernel, David S. Miller, Stephen Hemminger,
moderated list:ETHERNET BRIDGE
Hi Nikolay,
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
>> err = __br_mdb_del(br, entry);
>> - if (!err)
>> - __br_mdb_notify(dev, p, entry, RTM_DELMDB);
>> + if (err)
>> + break;
>> + __br_mdb_notify(dev, p, entry, RTM_DELMDB);
>> }
>> } else {
>> err = __br_mdb_del(br, entry);
>>
>
> This can potentially break user-space scripts that rely on the best-effort
> behaviour, this is the normal "delete without vid & enabled vlan filtering".
> You can check the fdb delete code which does the same, this was intentional.
>
> You can add an mdb entry without a vid to all vlans, add a vlan and then try
> to remove it from all vlans where it is present - with this patch obviously
> that will fail at the new vlan.
OK good to know. That intention wasn't obvious. I can make __br_mdb_del
return void instead? What about the rest of the patchset if I do so?
Thanks,
Vivien
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails
2017-05-18 15:08 ` Vivien Didelot
@ 2017-05-18 15:25 ` Nikolay Aleksandrov
2017-05-18 15:45 ` Vivien Didelot
0 siblings, 1 reply; 12+ messages in thread
From: Nikolay Aleksandrov @ 2017-05-18 15:25 UTC (permalink / raw)
To: Vivien Didelot, netdev
Cc: linux-kernel, kernel, David S. Miller, Stephen Hemminger,
moderated list:ETHERNET BRIDGE
On 5/18/17 6:08 PM, Vivien Didelot wrote:
> Hi Nikolay,
>
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
>
>>> err = __br_mdb_del(br, entry);
>>> - if (!err)
>>> - __br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>> + if (err)
>>> + break;
>>> + __br_mdb_notify(dev, p, entry, RTM_DELMDB);
>>> }
>>> } else {
>>> err = __br_mdb_del(br, entry);
>>>
>>
>> This can potentially break user-space scripts that rely on the best-effort
>> behaviour, this is the normal "delete without vid & enabled vlan filtering".
>> You can check the fdb delete code which does the same, this was intentional.
>>
>> You can add an mdb entry without a vid to all vlans, add a vlan and then try
>> to remove it from all vlans where it is present - with this patch obviously
>> that will fail at the new vlan.
>
> OK good to know. That intention wasn't obvious. I can make __br_mdb_del
> return void instead? What about the rest of the patchset if I do so?
>
> Thanks,
>
> Vivien
>
If you make it return void we will not be able to return proper error value
when doing a single operation (the else case). About the rest I see only some
minor style issues, I'll comment on the respective patches. Another minor nit is
using switch() instead of if/else for the message types but that is really up to
you, I don't mind either way. :-)
Cheers,
Nik
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops
2017-05-17 21:27 ` [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops Vivien Didelot
@ 2017-05-18 15:28 ` Nikolay Aleksandrov
0 siblings, 0 replies; 12+ messages in thread
From: Nikolay Aleksandrov @ 2017-05-18 15:28 UTC (permalink / raw)
To: Vivien Didelot, netdev
Cc: linux-kernel, kernel, David S. Miller, Stephen Hemminger,
moderated list:ETHERNET BRIDGE
On 5/18/17 12:27 AM, Vivien Didelot wrote:
> Retrieve the message type from the nlmsghdr structure instead of
> hardcoding it in both br_mdb_add and br_mdb_del.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
> net/bridge/br_mdb.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index a72d5e6f339f..d280b20587cb 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -569,6 +569,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
> struct net_bridge_port *p;
> struct net_bridge_vlan *v;
> struct net_bridge *br;
> + int msgtype = nlh->nlmsg_type;
minor nits:
nlmsg_type is a u16, also please keep the order and arrange these from longest
to shortest
> int err;
>
> err = br_mdb_parse(skb, nlh, &dev, &entry);
> @@ -595,12 +596,12 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
> if (br_vlan_enabled(br) && vg && entry->vid == 0) {
> list_for_each_entry(v, &vg->vlan_list, vlist) {
> entry->vid = v->vid;
> - err = __br_mdb_do(p, entry, RTM_NEWMDB);
> + err = __br_mdb_do(p, entry, msgtype);
> if (err)
> break;
> }
> } else {
> - err = __br_mdb_do(p, entry, RTM_NEWMDB);
> + err = __br_mdb_do(p, entry, msgtype);
> }
>
> return err;
> @@ -677,6 +678,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
> struct net_bridge_port *p;
> struct net_bridge_vlan *v;
> struct net_bridge *br;
> + int msgtype = nlh->nlmsg_type;
same here
> int err;
>
> err = br_mdb_parse(skb, nlh, &dev, &entry);
> @@ -703,12 +705,12 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
> if (br_vlan_enabled(br) && vg && entry->vid == 0) {
> list_for_each_entry(v, &vg->vlan_list, vlist) {
> entry->vid = v->vid;
> - err = __br_mdb_do(p, entry, RTM_DELMDB);
> + err = __br_mdb_do(p, entry, msgtype);
> if (err)
> break;
> }
> } else {
> - err = __br_mdb_do(p, entry, RTM_DELMDB);
> + err = __br_mdb_do(p, entry, msgtype);
> }
>
> return err;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails
2017-05-18 15:25 ` Nikolay Aleksandrov
@ 2017-05-18 15:45 ` Vivien Didelot
0 siblings, 0 replies; 12+ messages in thread
From: Vivien Didelot @ 2017-05-18 15:45 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev
Cc: linux-kernel, kernel, David S. Miller, Stephen Hemminger,
moderated list:ETHERNET BRIDGE
Hi Nikolay,
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
>> OK good to know. That intention wasn't obvious. I can make __br_mdb_del
>> return void instead? What about the rest of the patchset if I do so?
>
> If you make it return void we will not be able to return proper error value
> when doing a single operation (the else case). About the rest I see only some
> minor style issues, I'll comment on the respective patches. Another minor nit is
> using switch() instead of if/else for the message types but that is really up to
> you, I don't mind either way. :-)
Ho OK I understand better the batch vs single delete operation now.
__br_mdb_do hardly makes sense now, because we don't know which case we
are handling... But factorizing br_mdb_do still makes sense. I'll come
up with something.
Thanks,
Vivien
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-05-18 15:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-17 21:27 [PATCH net-next 0/6] net: bridge: factorize MDB new and del functions Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 1/6] net: bridge: pass net_bridge_port to __br_mdb_add Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 2/6] net: bridge: check multicast bridge only once Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails Vivien Didelot
2017-05-18 13:45 ` Nikolay Aleksandrov
2017-05-18 15:08 ` Vivien Didelot
2017-05-18 15:25 ` Nikolay Aleksandrov
2017-05-18 15:45 ` Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 4/6] net: bridge: add __br_mdb_do Vivien Didelot
2017-05-17 21:27 ` [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops Vivien Didelot
2017-05-18 15:28 ` Nikolay Aleksandrov
2017-05-17 21:27 ` [PATCH net-next 6/6] net: bridge: add br_mdb_do Vivien Didelot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).