netdev - Re: [PATCH] ipv4: fix a RCU-list lock in fib_triestat_seq

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <92C7474D-4592-44BF-B0ED-26253196511E@lca.pw>
Date:   Wed, 25 Mar 2020 13:34:46 -0400
From:   Qian Cai <cai@....pw>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     davem@...emloft.net, kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org,
        kuba@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipv4: fix a RCU-list lock in fib_triestat_seq_show



> On Mar 25, 2020, at 12:13 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> 
> I would prefer :
> 
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index ff0c24371e3309b3068980f46d1ed743337d2a3e..4b98ffb27136d3b43f179d6b1b42fe84586acc06 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -2581,6 +2581,7 @@ static int fib_triestat_seq_show(struct seq_file *seq, void *v)
>                struct hlist_head *head = &net->ipv4.fib_table_hash[h];
>                struct fib_table *tb;
> 
> +               rcu_read_lock();
>                hlist_for_each_entry_rcu(tb, head, tb_hlist) {
>                        struct trie *t = (struct trie *) tb->tb_data;
>                        struct trie_stat stat;
> @@ -2596,6 +2597,7 @@ static int fib_triestat_seq_show(struct seq_file *seq, void *v)
>                        trie_show_usage(seq, t->stats);
> #endif
>                }
> +               rcu_read_unlock();
>        }
> 
>        return 0;

I have no strong opinion either way. My initial thought was to save 255 extra lock/unlock with a single lock/unlock, but I am not sure how time-consuming for each iteration of the outer loop could be. If it could take a bit too long, it does make a lot of sense to reduce the critical section.