netdev - Re: Why should we add duplicate rules without NLM_F

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]

Message-ID: <d2a89996-2c73-be8c-a890-6d8543eda6ba@cumulusnetworks.com>
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.