netdev - Re: [PATCH net] ipv4: make fib_info

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 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]

Date:   Fri, 14 Jan 2022 08:25:04 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        syzbot <syzkaller@...glegroups.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net] ipv4: make fib_info_cnt atomic

On Fri, Jan 14, 2022 at 7:50 AM David Laight <David.Laight@...lab.com> wrote:
>
> From: Eric Dumazet
> > Sent: 14 January 2022 15:39
> >
> > Instead of making sure all free_fib_info() callers
> > hold rtnl, it seems better to convert fib_info_cnt
> > to an atomic_t.
>
> Since fib_info_cnt is only used to control the size of the hash table
> could it be incremented when a fid is added to the hash table and
> decremented when it is removed.
>
> This is all inside the fib_info_lock.

Sure, this will need some READ_ONCE()/WRITE_ONCE() annotations
because the resize would read fib_info_cnt without this lock held.

I am not sure this is a stable candidate though, patch looks a bit more risky.

This seems to suggest another issue...

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 828de171708f599b56f63715514c0259c7cb08a2..45619c005b8dddd7ccd5c7029efa4ed69b6ce1de
100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -249,7 +249,6 @@ void free_fib_info(struct fib_info *fi)
                pr_warn("Freeing alive fib_info %p\n", fi);
                return;
        }
-       fib_info_cnt--;

        call_rcu(&fi->rcu, free_fib_info_rcu);
 }
@@ -260,6 +259,10 @@ void fib_release_info(struct fib_info *fi)
        spin_lock_bh(&fib_info_lock);
        if (fi && refcount_dec_and_test(&fi->fib_treeref)) {
                hlist_del(&fi->fib_hash);
+
+               /* Paired with READ_ONCE() in fib_create_info(). */
+               WRITE_ONCE(fib_info_cnt, fib_info_cnt - 1);
+
                if (fi->fib_prefsrc)
                        hlist_del(&fi->fib_lhash);
                if (fi->nh) {
@@ -1430,7 +1433,9 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 #endif

        err = -ENOBUFS;
-       if (fib_info_cnt >= fib_info_hash_size) {
+
+       /* Paired with WRITE_ONCE() in fib_release_info() */
+       if (READ_ONCE(fib_info_cnt) >= fib_info_hash_size) {
                unsigned int new_size = fib_info_hash_size << 1;
                struct hlist_head *new_info_hash;
                struct hlist_head *new_laddrhash;
@@ -1462,7 +1467,6 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
                return ERR_PTR(err);
        }

-       fib_info_cnt++;
        fi->fib_net = net;
        fi->fib_protocol = cfg->fc_protocol;
        fi->fib_scope = cfg->fc_scope;
@@ -1591,6 +1595,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
        refcount_set(&fi->fib_treeref, 1);
        refcount_set(&fi->fib_clntref, 1);
        spin_lock_bh(&fib_info_lock);
+       fib_info_cnt++;
        hlist_add_head(&fi->fib_hash,
                       &fib_info_hash[fib_info_hashfn(fi)]);
        if (fi->fib_prefsrc) {