[PATCH net] fib_rules: fix fib rule dumps across multiple skbs

[PATCH net] fib_rules: fix fib rule dumps across multiple skbs

[Roopa Prabhu]

From: Wilson Kok <wkok@cumulusnetworks.com>

dump_rules returns skb length and not error.
But when family == AF_UNSPEC, the caller of dump_rules
assumes that it returns an error. Hence, when family == AF_UNSPEC,
we continue trying to dump on -EMSGSIZE errors resulting in
incorrect dump idx carried between skbs belonging to the same dump.
This results in fib rule dump always only dumping rules that fit
into the first skb.

This patch fixes dump_rules to return error so that we exit correctly
and idx is correctly maintained between skbs that are part of the
same dump.

Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 net/core/fib_rules.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index bf77e36..365de66 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -631,15 +631,17 @@ static int dump_rules(struct sk_buff *skb, struct netlink_callback *cb,
 {
 	int idx = 0;
 	struct fib_rule *rule;
+	int err = 0;
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(rule, &ops->rules_list, list) {
 		if (idx < cb->args[1])
 			goto skip;
 
-		if (fib_nl_fill_rule(skb, rule, NETLINK_CB(cb->skb).portid,
-				     cb->nlh->nlmsg_seq, RTM_NEWRULE,
-				     NLM_F_MULTI, ops) < 0)
+		err = fib_nl_fill_rule(skb, rule, NETLINK_CB(cb->skb).portid,
+				       cb->nlh->nlmsg_seq, RTM_NEWRULE,
+				       NLM_F_MULTI, ops);
+		if (err)
 			break;
 skip:
 		idx++;
@@ -648,7 +650,7 @@ skip:
 	cb->args[1] = idx;
 	rules_ops_put(ops);
 
-	return skb->len;
+	return err;
 }
 
 static int fib_nl_dumprule(struct sk_buff *skb, struct netlink_callback *cb)
@@ -664,7 +666,9 @@ static int fib_nl_dumprule(struct sk_buff *skb, struct netlink_callback *cb)
 		if (ops == NULL)
 			return -EAFNOSUPPORT;
 
-		return dump_rules(skb, cb, ops);
+		dump_rules(skb, cb, ops);
+
+		return skb->len;
 	}
 
 	rcu_read_lock();
-- 
1.9.1

Re: [PATCH net] fib_rules: fix fib rule dumps across multiple skbs

[David Miller]

From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Tue, 22 Sep 2015 21:40:22 -0700

> From: Wilson Kok <wkok@cumulusnetworks.com>
> 
> dump_rules returns skb length and not error.
> But when family == AF_UNSPEC, the caller of dump_rules
> assumes that it returns an error. Hence, when family == AF_UNSPEC,
> we continue trying to dump on -EMSGSIZE errors resulting in
> incorrect dump idx carried between skbs belonging to the same dump.
> This results in fib rule dump always only dumping rules that fit
> into the first skb.
> 
> This patch fixes dump_rules to return error so that we exit correctly
> and idx is correctly maintained between skbs that are part of the
> same dump.
> 
> Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Applied and queued up for -stable, thanks.

Re: [PATCH net] fib_rules: fix fib rule dumps across multiple skbs

[Roland Dreier]

On Tue, Sep 22, 2015 at 9:40 PM, Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
> +               err = fib_nl_fill_rule(skb, rule, NETLINK_CB(cb->skb).portid,
> +                                      cb->nlh->nlmsg_seq, RTM_NEWRULE,
> +                                      NLM_F_MULTI, ops);
> +               if (err)

FWIW I believe this breaks pre-4.0 stable kernels (unfortunately it
just showed up in 3.10.90).  In kernels without 053c095a82cf
("netlink: make nlmsg_end() and genlmsg_end() void") then
fib_nl_fill_rule() returns a positive value (skb->len) on success, so
we break out of the loop here immediately.  Symptom is "ip rule show"
loops forever printing the first rule.

After I finish testing a fix (as trivial as changing to "if (err <
0)") here, I'll send it to -stable guys.

 - R.

Re: [PATCH net] fib_rules: fix fib rule dumps across multiple skbs

[roopa]

On 10/2/15, 10:18 AM, Roland Dreier wrote:
> On Tue, Sep 22, 2015 at 9:40 PM, Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>> +               err = fib_nl_fill_rule(skb, rule, NETLINK_CB(cb->skb).portid,
>> +                                      cb->nlh->nlmsg_seq, RTM_NEWRULE,
>> +                                      NLM_F_MULTI, ops);
>> +               if (err)
> FWIW I believe this breaks pre-4.0 stable kernels (unfortunately it
> just showed up in 3.10.90).  In kernels without 053c095a82cf
> ("netlink: make nlmsg_end() and genlmsg_end() void") then
> fib_nl_fill_rule() returns a positive value (skb->len) on success, so
> we break out of the loop here immediately.  Symptom is "ip rule show"
> loops forever printing the first rule.
>
> After I finish testing a fix (as trivial as changing to "if (err <
> 0)") here, I'll send it to -stable guys.
>
Thanks for catching this.