lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 29 Apr 2019 08:48:04 -0600
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org,
        Mateusz Bajorski <mateusz.bajorski@...ia.com>,
        Thomas Haller <thaller@...hat.com>
Subject: Re: Why should we add duplicate rules without NLM_F_EXCL?

On 4/28/19 12:21 AM, Hangbin Liu wrote:
> Hi David, Mateusz,
> 
> Kernel commit 153380ec4b9b ("fib_rules: Added NLM_F_EXCL support to
> fib_nl_newrule") added a check and return -EEXIST if the rule is already
> exist. With it the ip rule works as expected now.
> 
> But without NLM_F_EXCL people still could add duplicate rules. the result
> looks like:
> 
> # ip rule
> 0:      from all lookup local
> 32766:  from all lookup main
> 32767:  from all lookup default
> 100000: from 192.168.7.5 lookup 5
> 100000: from 192.168.7.5 lookup 5
> 
> The two same rules looks unreasonable. Do you know if there is a use case
> that need this?

It does not make sense to me to allow multiple entries with the same
config; it only adds to the overhead of fib lookups.

> 
> So how about just return directly if user add a exactally same rule, as if
> we did an update, like:
> 
> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
> index ffbb827723a2..c49b752ea7eb 100644
> --- a/net/core/fib_rules.c
> +++ b/net/core/fib_rules.c
> @@ -756,9 +756,9 @@ int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr *nlh,
>         if (err)
>                 goto errout;
> 
> -       if ((nlh->nlmsg_flags & NLM_F_EXCL) &&
> -           rule_exists(ops, frh, tb, rule)) {
> -               err = -EEXIST;
> +       if (rule_exists(ops, frh, tb, rule)) {
> +               if (nlh->nlmsg_flags & NLM_F_EXCL)
> +                       err = -EEXIST;
>                 goto errout_free;
>         }
> 
> 
> What do you think?

I'm not so sure about the failure and more from a consistency with other
RTM_NEW commands which cover updates to an existing entry. In the case
of rules if there is nothing to update and the rule already exists then
- for consistency - 0 is the right return code.


Powered by blists - more mailing lists