Re: [net regression] "fib_rules: move common handling of newrule delrule msgs into fib_nl2rule" breaks suppress_prefixlength

2018-06-25 Thread Roopa Prabhu
On Mon, Jun 25, 2018 at 8:23 AM, Roopa Prabhu  wrote:
> On Sat, Jun 23, 2018 at 8:46 AM, Jason A. Donenfeld  wrote:
>> Hey Roopa,
>>
>> On a kernel with a minimal networking config,
>> CONFIG_IP_MULTIPLE_TABLES appears to be broken for certain rules after
>> f9d4b0c1e9695e3de7af3768205bacc27312320c.
>>
>> Try, for example, running:
>>
>> $ ip -4 rule add table main suppress_prefixlength 0
>>
>> It returns with EEXIST.
>>
>> Perhaps the reason is that the new rule_find function does not match
>> on suppress_prefixlength? However, rule_exist from before didn't do
>> that either. I'll keep playing and see if I can track it down myself,
>> but thought I should let you know first.
>
> I am surprised at that also. I cannot find prior rule_exist looking at
> suppress_prefixlength.
> I will dig deeper also today. But your patch LGTM with a small change
> I commented on it.
>

So the previous rule_exists code did not check for attribute matches correctly.
It would ignore a rule at the first non-existent attribute mis-match.
eg in your case, it would
return at pref mismatch.


$ip -4 rule add table main suppress_prefixlength 0
$ip -4 rule add table main suppress_prefixlength 0
$ip -4 rule add table main suppress_prefixlength 0

$ip rule show
0:  from all lookup local

32763:  from all lookup main suppress_prefixlength 0

32764:  from all lookup main suppress_prefixlength 0

32765:  from all lookup main suppress_prefixlength 0

32766:  from all lookup main

32767:  from all lookup default


With your patch (with my proposed fixes), you should get proper EXISTS check

$ git diff HEAD

diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c

index 126ffc5..de4c171 100644

--- a/net/core/fib_rules.c

+++ b/net/core/fib_rules.c

@@ -428,6 +428,14 @@ static struct fib_rule *rule_find(struct
fib_rules_ops *ops,

if (rule->l3mdev && r->l3mdev != rule->l3mdev)

continue;



+   if (rule->suppress_ifgroup != -1 &&

+   (r->suppress_ifgroup != rule->suppress_ifgroup))

+   continue;

+

+   if (rule->suppress_prefixlen != -1 &&

+   (r->suppress_prefixlen != rule->suppress_prefixlen))

+   continue;

+

if (uid_range_set(>uid_range) &&

(!uid_eq(r->uid_range.start, rule->uid_range.start) ||

!uid_eq(r->uid_range.end, rule->uid_range.end)))



$ ip -4 rule add table main suppress_prefixlength 0

$ ip -4 rule add table main suppress_prefixlength 0

RTNETLINK answers: File exists


Re: [net regression] "fib_rules: move common handling of newrule delrule msgs into fib_nl2rule" breaks suppress_prefixlength

2018-06-25 Thread Roopa Prabhu
On Sat, Jun 23, 2018 at 8:46 AM, Jason A. Donenfeld  wrote:
> Hey Roopa,
>
> On a kernel with a minimal networking config,
> CONFIG_IP_MULTIPLE_TABLES appears to be broken for certain rules after
> f9d4b0c1e9695e3de7af3768205bacc27312320c.
>
> Try, for example, running:
>
> $ ip -4 rule add table main suppress_prefixlength 0
>
> It returns with EEXIST.
>
> Perhaps the reason is that the new rule_find function does not match
> on suppress_prefixlength? However, rule_exist from before didn't do
> that either. I'll keep playing and see if I can track it down myself,
> but thought I should let you know first.

I am surprised at that also. I cannot find prior rule_exist looking at
suppress_prefixlength.
I will dig deeper also today. But your patch LGTM with a small change
I commented on it.

>
> A relevant .config can be found at https://א.cc/iq5HoUY0
>

thanks.


[net regression] "fib_rules: move common handling of newrule delrule msgs into fib_nl2rule" breaks suppress_prefixlength

2018-06-23 Thread Jason A. Donenfeld
Hey Roopa,

On a kernel with a minimal networking config,
CONFIG_IP_MULTIPLE_TABLES appears to be broken for certain rules after
f9d4b0c1e9695e3de7af3768205bacc27312320c.

Try, for example, running:

$ ip -4 rule add table main suppress_prefixlength 0

It returns with EEXIST.

Perhaps the reason is that the new rule_find function does not match
on suppress_prefixlength? However, rule_exist from before didn't do
that either. I'll keep playing and see if I can track it down myself,
but thought I should let you know first.

A relevant .config can be found at https://א.cc/iq5HoUY0

Jason