[PATCH] ipv4: Fix off-by-one in route dump counter without netlink strict checking

[PATCH] ipv4: Fix off-by-one in route dump counter without netlink strict checking

[Stefano Brivio]

In commit ee28906fd7a1 ("ipv4: Dump route exceptions if requested") I
added a counter of per-node dumped routes (including actual routes and
exceptions), analogous to the existing counter for dumped nodes. Dumping
exceptions means we need to also keep track of how many routes are dumped
for each node: this would be just one route per node, without exceptions.

When netlink strict checking is not enabled, we dump both routes and
exceptions at the same time: the RTM_F_CLONED flag is not used as a
filter. In this case, the per-node counter 'i_fa' is incremented by one
to track the single dumped route, then also incremented by one for each
exception dumped, and then stored as netlink callback argument as skip
counter, 's_fa', to be used when a partial dump operation restarts.

The per-node counter needs to be increased by one also when we skip a
route (exception) due to a previous non-zero skip counter, because it
needs to match the existing skip counter, if we are dumping both routes
and exceptions. I missed this, and only incremented the counter, for
regular routes, if the previous skip counter was zero. This means that,
in case of a mixed dump, partial dump operations after the first one
will start with a mismatching skip counter value, one less than expected.

This means in turn that the first exception for a given node is skipped
every time a partial dump operation restarts, if netlink strict checking
is not enabled (iproute < 5.0).

It turns out I didn't repeat the test in its final version, commit
de755a85130e ("selftests: pmtu: Introduce list_flush_ipv4_exception test
case"), which also counts the number of route exceptions returned, with
iproute2 versions < 5.0 -- I was instead using the equivalent of the IPv6
test as it was before commit b964641e9925 ("selftests: pmtu: Make
list_flush_ipv6_exception test more demanding").

Always increment the per-node counter by one if we previously dumped
a regular route, so that it matches the current skip counter.

Fixes: ee28906fd7a1 ("ipv4: Dump route exceptions if requested")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 net/ipv4/fib_trie.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 4400f5051977..2b2b3d291ab0 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -2126,14 +2126,20 @@ static int fn_trie_dump_leaf(struct key_vector *l, struct fib_table *tb,
 				goto next;
 		}
 
-		if (filter->dump_routes && !s_fa) {
-			err = fib_dump_info(skb, NETLINK_CB(cb->skb).portid,
-					    cb->nlh->nlmsg_seq, RTM_NEWROUTE,
-					    tb->tb_id, fa->fa_type,
-					    xkey, KEYLENGTH - fa->fa_slen,
-					    fa->fa_tos, fi, flags);
-			if (err < 0)
-				goto stop;
+		if (filter->dump_routes) {
+			if (!s_fa) {
+				err = fib_dump_info(skb,
+						    NETLINK_CB(cb->skb).portid,
+						    cb->nlh->nlmsg_seq,
+						    RTM_NEWROUTE,
+						    tb->tb_id, fa->fa_type,
+						    xkey,
+						    KEYLENGTH - fa->fa_slen,
+						    fa->fa_tos, fi, flags);
+				if (err < 0)
+					goto stop;
+			}
+
 			i_fa++;
 		}
 
-- 
2.20.1

Re: [PATCH] ipv4: Fix off-by-one in route dump counter without netlink strict checking

[Stefano Brivio]

On Sat, 29 Jun 2019 19:55:08 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> Always increment the per-node counter by one if we previously dumped
> a regular route, so that it matches the current skip counter.
> 
> Fixes: ee28906fd7a1 ("ipv4: Dump route exceptions if requested")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Sorry David, this was meant for net-next.

-- 
Stefano

Re: [PATCH] ipv4: Fix off-by-one in route dump counter without netlink strict checking

[David Miller]

From: Stefano Brivio <sbrivio@redhat.com>
Date: Sat, 29 Jun 2019 19:55:08 +0200

> In commit ee28906fd7a1 ("ipv4: Dump route exceptions if requested") I
> added a counter of per-node dumped routes (including actual routes and
> exceptions), analogous to the existing counter for dumped nodes. Dumping
> exceptions means we need to also keep track of how many routes are dumped
> for each node: this would be just one route per node, without exceptions.
> 
> When netlink strict checking is not enabled, we dump both routes and
> exceptions at the same time: the RTM_F_CLONED flag is not used as a
> filter. In this case, the per-node counter 'i_fa' is incremented by one
> to track the single dumped route, then also incremented by one for each
> exception dumped, and then stored as netlink callback argument as skip
> counter, 's_fa', to be used when a partial dump operation restarts.
> 
> The per-node counter needs to be increased by one also when we skip a
> route (exception) due to a previous non-zero skip counter, because it
> needs to match the existing skip counter, if we are dumping both routes
> and exceptions. I missed this, and only incremented the counter, for
> regular routes, if the previous skip counter was zero. This means that,
> in case of a mixed dump, partial dump operations after the first one
> will start with a mismatching skip counter value, one less than expected.
> 
> This means in turn that the first exception for a given node is skipped
> every time a partial dump operation restarts, if netlink strict checking
> is not enabled (iproute < 5.0).
> 
> It turns out I didn't repeat the test in its final version, commit
> de755a85130e ("selftests: pmtu: Introduce list_flush_ipv4_exception test
> case"), which also counts the number of route exceptions returned, with
> iproute2 versions < 5.0 -- I was instead using the equivalent of the IPv6
> test as it was before commit b964641e9925 ("selftests: pmtu: Make
> list_flush_ipv6_exception test more demanding").
> 
> Always increment the per-node counter by one if we previously dumped
> a regular route, so that it matches the current skip counter.
> 
> Fixes: ee28906fd7a1 ("ipv4: Dump route exceptions if requested")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Applied to net-next, thanks for fixing this.