Nikolay Aleksandrov
2022-Jan-25 09:51 UTC
[Bridge] [PATCH net] net: bridge: vlan: Fix dumping with ifindex
On 25/01/2022 10:24, Nikolay Aleksandrov wrote:> On 25/01/2022 08:19, Benjamin Poirier wrote: >> Specifying ifindex in a RTM_GETVLAN dump leads to an infinite repetition >> of the same entries. netlink_dump() normally calls the dump function >> repeatedly until it returns 0 which br_vlan_rtm_dump() never does in >> that case. >> >> Fixes: 8dcea187088b ("net: bridge: vlan: add rtm definitions and dump support") >> Signed-off-by: Benjamin Poirier <bpoirier at nvidia.com> >> --- >> net/bridge/br_vlan.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> > [snip] >> >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c >> index 84ba456a78cc..2e606f2b9a4d 100644 >> --- a/net/bridge/br_vlan.c >> +++ b/net/bridge/br_vlan.c >> @@ -2013,7 +2013,7 @@ static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >> dump_flags = nla_get_u32(dtb[BRIDGE_VLANDB_DUMP_FLAGS]); >> >> rcu_read_lock(); >> - if (bvm->ifindex) { >> + if (bvm->ifindex && !s_idx) { >> dev = dev_get_by_index_rcu(net, bvm->ifindex); >> if (!dev) { >> err = -ENODEV; >> @@ -2022,7 +2022,9 @@ static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) >> err = br_vlan_dump_dev(dev, skb, cb, dump_flags); >> if (err && err != -EMSGSIZE) >> goto out_err; >> - } else { >> + else if (!err) >> + idx++; >> + } else if (!bvm->ifindex) { >> for_each_netdev_rcu(net, dev) { >> if (idx < s_idx) >> goto skip; > > Acked-by: Nikolay Aleksandrov <nikolay at nvidia.com>Actually I'd prefer an alternative that would encapsulate handling the single device dump in its block, avoid all the "else if"s and is simpler (untested): diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 84ba456a78cc..43201260e37b 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -2020,7 +2020,8 @@ static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) goto out_err; } err = br_vlan_dump_dev(dev, skb, cb, dump_flags); - if (err && err != -EMSGSIZE) + /* if the dump completed without an error we return 0 here */ + if (err != -EMSGSIZE) goto out_err; } else { for_each_netdev_rcu(net, dev) {
Benjamin Poirier
2022-Jan-26 02:54 UTC
[Bridge] [PATCH net] net: bridge: vlan: Fix dumping with ifindex
On 2022-01-25 11:51 +0200, Nikolay Aleksandrov wrote:> On 25/01/2022 10:24, Nikolay Aleksandrov wrote: > > On 25/01/2022 08:19, Benjamin Poirier wrote: > >> Specifying ifindex in a RTM_GETVLAN dump leads to an infinite repetition > >> of the same entries. netlink_dump() normally calls the dump function > >> repeatedly until it returns 0 which br_vlan_rtm_dump() never does in > >> that case. > >> > >> Fixes: 8dcea187088b ("net: bridge: vlan: add rtm definitions and dump support") > >> Signed-off-by: Benjamin Poirier <bpoirier at nvidia.com> > >> --- > >> net/bridge/br_vlan.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > > [snip] > >> > >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > >> index 84ba456a78cc..2e606f2b9a4d 100644 > >> --- a/net/bridge/br_vlan.c > >> +++ b/net/bridge/br_vlan.c > >> @@ -2013,7 +2013,7 @@ static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) > >> dump_flags = nla_get_u32(dtb[BRIDGE_VLANDB_DUMP_FLAGS]); > >> > >> rcu_read_lock(); > >> - if (bvm->ifindex) { > >> + if (bvm->ifindex && !s_idx) { > >> dev = dev_get_by_index_rcu(net, bvm->ifindex); > >> if (!dev) { > >> err = -ENODEV; > >> @@ -2022,7 +2022,9 @@ static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) > >> err = br_vlan_dump_dev(dev, skb, cb, dump_flags); > >> if (err && err != -EMSGSIZE) > >> goto out_err; > >> - } else { > >> + else if (!err) > >> + idx++; > >> + } else if (!bvm->ifindex) { > >> for_each_netdev_rcu(net, dev) { > >> if (idx < s_idx) > >> goto skip; > > > > Acked-by: Nikolay Aleksandrov <nikolay at nvidia.com> > > Actually I'd prefer an alternative that would encapsulate handling the single > device dump in its block, avoid all the "else if"s and is simpler (untested): > > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > index 84ba456a78cc..43201260e37b 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -2020,7 +2020,8 @@ static int br_vlan_rtm_dump(struct sk_buff *skb, struct netlink_callback *cb) > goto out_err; > } > err = br_vlan_dump_dev(dev, skb, cb, dump_flags); > - if (err && err != -EMSGSIZE) > + /* if the dump completed without an error we return 0 here */ > + if (err != -EMSGSIZE) > goto out_err; > } else { > for_each_netdev_rcu(net, dev) {LGTM, thank you.
Nikolay Aleksandrov
2022-Jan-26 13:10 UTC
[Bridge] [PATCH net] net: bridge: vlan: fix single net device option dumping
When dumping vlan options for a single net device we send the same
entries infinitely because user-space expects a 0 return at the end but
we keep returning skb->len and restarting the dump on retry. Fix it by
returning the value from br_vlan_dump_dev() if it completed or there was
an error. The only case that must return skb->len is when the dump was
incomplete and needs to continue (-EMSGSIZE).
Reported-by: Benjamin Poirier <bpoirier at nvidia.com>
Fixes: 8dcea187088b ("net: bridge: vlan: add rtm definitions and dump
support")
Signed-off-by: Nikolay Aleksandrov <nikolay at nvidia.com>
---
net/bridge/br_vlan.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 84ba456a78cc..43201260e37b 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -2020,7 +2020,8 @@ static int br_vlan_rtm_dump(struct sk_buff *skb, struct
netlink_callback *cb)
goto out_err;
}
err = br_vlan_dump_dev(dev, skb, cb, dump_flags);
- if (err && err != -EMSGSIZE)
+ /* if the dump completed without an error we return 0 here */
+ if (err != -EMSGSIZE)
goto out_err;
} else {
for_each_netdev_rcu(net, dev) {
--
2.34.1