Re: [PATCH net] ipv4: make fib_info_cnt atomic

[Ido Schimmel <idosch@idosch.org>]

On Fri, Jan 14, 2022 at 08:25:04AM -0800, 'Eric Dumazet' via syzkaller wrote:
> On Fri, Jan 14, 2022 at 7:50 AM David Laight <David.Laight@aculab.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) {

Thanks for the fix. Looks OK to me. The counter is incremented under the
lock when adding to the hash table(s) and decremented under the lock
upon removal. Do you intend to submit this version instead of the first
one?

> 
> -- 
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/CANn89iKA32qt8X6VzFsissZwxHpar6pDEJT_dgYhnxfROcaqRA%40mail.gmail.com.