* [PATCH net] net: bridge: vlan: Fix dumping with ifindex
@ 2022-01-25 6:19 Benjamin Poirier
2022-01-25 8:24 ` Nikolay Aleksandrov
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Poirier @ 2022-01-25 6:19 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Roopa Prabhu, David S. Miller, Jakub Kicinski, bridge, netdev
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@nvidia.com>
---
net/bridge/br_vlan.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
The problem can be reproduced by using the following iproute2 patch and
running:
ip link add br0 type bridge
ip link add dummy0 master br0 type dummy
bridge vlan add dev dummy0 vid 100
./bridge/bridge -d vlan show dev dummy0
[infinite loop]
diff --git a/bridge/vlan.c b/bridge/vlan.c
index 8300f353..f206a671 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -1128,7 +1128,8 @@ static int vlan_show(int argc, char **argv, int subject)
if (show_details && subject == VLAN_SHOW_VLAN) {
__u32 dump_flags = show_stats ? BRIDGE_VLANDB_DUMPF_STATS : 0;
- if (rtnl_brvlandump_req(&rth, PF_BRIDGE, dump_flags) < 0) {
+ if (rtnl_brvlandump_req(&rth, PF_BRIDGE, dump_flags,
+ filter_index) < 0) {
perror("Cannot send dump request");
exit(1);
}
@@ -1240,7 +1241,8 @@ static int vlan_global_show(int argc, char **argv)
new_json_obj(json);
- if (rtnl_brvlandump_req(&rth, PF_BRIDGE, dump_flags) < 0) {
+ if (rtnl_brvlandump_req(&rth, PF_BRIDGE, dump_flags, filter_index)
+ < 0) {
perror("Cannot send dump request");
exit(1);
}
diff --git a/include/libnetlink.h b/include/libnetlink.h
index 9e4cc101..276b7fbf 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -69,7 +69,8 @@ int rtnl_neightbldump_req(struct rtnl_handle *rth, int family)
__attribute__((warn_unused_result));
int rtnl_mdbdump_req(struct rtnl_handle *rth, int family)
__attribute__((warn_unused_result));
-int rtnl_brvlandump_req(struct rtnl_handle *rth, int family, __u32 dump_flags)
+int rtnl_brvlandump_req(struct rtnl_handle *rth, int family, __u32 dump_flags,
+ int ifindex)
__attribute__((warn_unused_result));
int rtnl_netconfdump_req(struct rtnl_handle *rth, int family)
__attribute__((warn_unused_result));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 7e977a67..e0fce8e5 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -450,7 +450,8 @@ int rtnl_mdbdump_req(struct rtnl_handle *rth, int family)
return send(rth->fd, &req, sizeof(req), 0);
}
-int rtnl_brvlandump_req(struct rtnl_handle *rth, int family, __u32 dump_flags)
+int rtnl_brvlandump_req(struct rtnl_handle *rth, int family, __u32 dump_flags,
+ int ifindex)
{
struct {
struct nlmsghdr nlh;
@@ -462,6 +463,7 @@ int rtnl_brvlandump_req(struct rtnl_handle *rth, int family, __u32 dump_flags)
.nlh.nlmsg_flags = NLM_F_DUMP | NLM_F_REQUEST,
.nlh.nlmsg_seq = rth->dump = ++rth->seq,
.bvm.family = family,
+ .bvm.ifindex = ifindex,
};
addattr32(&req.nlh, sizeof(req), BRIDGE_VLANDB_DUMP_FLAGS, dump_flags);
--
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;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: bridge: vlan: Fix dumping with ifindex
2022-01-25 6:19 [PATCH net] net: bridge: vlan: Fix dumping with ifindex Benjamin Poirier
@ 2022-01-25 8:24 ` Nikolay Aleksandrov
2022-01-25 9:51 ` Nikolay Aleksandrov
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2022-01-25 8:24 UTC (permalink / raw)
To: Benjamin Poirier
Cc: Roopa Prabhu, David S. Miller, Jakub Kicinski, bridge, netdev
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@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@nvidia.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: bridge: vlan: Fix dumping with ifindex
2022-01-25 8:24 ` Nikolay Aleksandrov
@ 2022-01-25 9:51 ` Nikolay Aleksandrov
2022-01-26 2:54 ` Benjamin Poirier
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2022-01-25 9:51 UTC (permalink / raw)
To: Benjamin Poirier
Cc: Roopa Prabhu, David S. Miller, Jakub Kicinski, bridge, netdev
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@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@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) {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: bridge: vlan: Fix dumping with ifindex
2022-01-25 9:51 ` Nikolay Aleksandrov
@ 2022-01-26 2:54 ` Benjamin Poirier
2022-01-26 13:10 ` [PATCH net] net: bridge: vlan: fix single net device option dumping Nikolay Aleksandrov
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Poirier @ 2022-01-26 2:54 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Roopa Prabhu, David S. Miller, Jakub Kicinski, bridge, netdev
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@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@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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net] net: bridge: vlan: fix single net device option dumping
2022-01-26 2:54 ` Benjamin Poirier
@ 2022-01-26 13:10 ` Nikolay Aleksandrov
2022-01-27 14:00 ` patchwork-bot+netdevbpf
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2022-01-26 13:10 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, bridge, roopa, Nikolay Aleksandrov, Benjamin Poirier
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@nvidia.com>
Fixes: 8dcea187088b ("net: bridge: vlan: add rtm definitions and dump support")
Signed-off-by: Nikolay Aleksandrov <nikolay@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: bridge: vlan: fix single net device option dumping
2022-01-26 13:10 ` [PATCH net] net: bridge: vlan: fix single net device option dumping Nikolay Aleksandrov
@ 2022-01-27 14:00 ` patchwork-bot+netdevbpf
0 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-01-27 14:00 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, davem, kuba, bridge, roopa, bpoirier
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Wed, 26 Jan 2022 15:10:25 +0200 you wrote:
> 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).
>
> [...]
Here is the summary with links:
- [net] net: bridge: vlan: fix single net device option dumping
https://git.kernel.org/netdev/net/c/dcb2c5c6ca9b
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] 6+ messages in thread
end of thread, other threads:[~2022-01-27 14:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-25 6:19 [PATCH net] net: bridge: vlan: Fix dumping with ifindex Benjamin Poirier
2022-01-25 8:24 ` Nikolay Aleksandrov
2022-01-25 9:51 ` Nikolay Aleksandrov
2022-01-26 2:54 ` Benjamin Poirier
2022-01-26 13:10 ` [PATCH net] net: bridge: vlan: fix single net device option dumping Nikolay Aleksandrov
2022-01-27 14:00 ` patchwork-bot+netdevbpf
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).