| Message ID | 20220114153902.1989393-1-eric.dumazet@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ipv4: make fib_info_cnt atomic | expand |
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. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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) {
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.
On Sun, Jan 16, 2022 at 12:46 AM Ido Schimmel <idosch@idosch.org> wrote: > > 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? Yes, I will send a V2, thanks !
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c index 828de171708f599b56f63715514c0259c7cb08a2..302373acf232205c812d10a041a87f22a64b1017 100644 --- a/net/ipv4/fib_semantics.c +++ b/net/ipv4/fib_semantics.c @@ -51,7 +51,7 @@ static DEFINE_SPINLOCK(fib_info_lock); static struct hlist_head *fib_info_hash; static struct hlist_head *fib_info_laddrhash; static unsigned int fib_info_hash_size; -static unsigned int fib_info_cnt; +static atomic_t fib_info_cnt; #define DEVINDEX_HASHBITS 8 #define DEVINDEX_HASHSIZE (1U << DEVINDEX_HASHBITS) @@ -249,7 +249,7 @@ void free_fib_info(struct fib_info *fi) pr_warn("Freeing alive fib_info %p\n", fi); return; } - fib_info_cnt--; + atomic_dec(&fib_info_cnt); call_rcu(&fi->rcu, free_fib_info_rcu); } @@ -1430,7 +1430,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg, #endif err = -ENOBUFS; - if (fib_info_cnt >= fib_info_hash_size) { + if (atomic_read(&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 +1462,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg, return ERR_PTR(err); } - fib_info_cnt++; + atomic_inc(&fib_info_cnt); fi->fib_net = net; fi->fib_protocol = cfg->fc_protocol; fi->fib_scope = cfg->fc_scope;