| Message ID | 20181108061053.137720-1-edumazet@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] inet: frags: better deal with smp races | expand |
From: Eric Dumazet <edumazet@google.com> Date: Wed, 7 Nov 2018 22:10:53 -0800 > @@ -204,22 +205,22 @@ static struct inet_frag_queue *inet_frag_create(struct netns_frags *nf, > /* TODO : call from rcu_read_lock() and no longer use refcount_inc_not_zero() */ > struct inet_frag_queue *inet_frag_find(struct netns_frags *nf, void *key) > { > - struct inet_frag_queue *fq; > + struct inet_frag_queue *fq, *prev; > > if (!nf->high_thresh || frag_mem_limit(nf) > nf->high_thresh) > return NULL; > > rcu_read_lock(); > > - fq = rhashtable_lookup(&nf->rhashtable, key, nf->f->rhash_params); > - if (fq) { > + prev = rhashtable_lookup(&nf->rhashtable, key, nf->f->rhash_params); > + if (!prev) > + fq = inet_frag_create(nf, key, &prev); > + if (prev && !IS_ERR(prev)) { > + fq = prev; > if (!refcount_inc_not_zero(&fq->refcnt)) > fq = NULL; > - rcu_read_unlock(); > - return fq; > } > rcu_read_unlock(); > - > - return inet_frag_create(nf, key); > + return fq; GCC is unwilling to see that all paths leading to that final return statement do in fact set 'fq' one way or another: net/ipv4/inet_fragment.c: In function ‘inet_frag_find’: net/ipv4/inet_fragment.c:224:9: warning: ‘fq’ may be used uninitialized in this function [-Wmaybe-uninitialized] This is with: gcc (GCC) 8.2.1 20181011 (Red Hat 8.2.1-4) Please adjust your patch so that the warning is eliminated. Thanks.
On 11/08/2018 05:02 PM, David Miller wrote: > GCC is unwilling to see that all paths leading to that final return > statement do in fact set 'fq' one way or another: > > net/ipv4/inet_fragment.c: In function ‘inet_frag_find’: > net/ipv4/inet_fragment.c:224:9: warning: ‘fq’ may be used uninitialized in this function [-Wmaybe-uninitialized] > > This is with: > > gcc (GCC) 8.2.1 20181011 (Red Hat 8.2.1-4) > > Please adjust your patch so that the warning is eliminated. > Interesting, I will init *fq to NULL in v2, thanks.
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c index bcb11f3a27c0c34115af05034a5a20f57842eb0a..ced9abd4bec6cd494e352c1d6a97da8f67cf6073 100644 --- a/net/ipv4/inet_fragment.c +++ b/net/ipv4/inet_fragment.c @@ -178,21 +178,22 @@ static struct inet_frag_queue *inet_frag_alloc(struct netns_frags *nf, } static struct inet_frag_queue *inet_frag_create(struct netns_frags *nf, - void *arg) + void *arg, + struct inet_frag_queue **prev) { struct inet_frags *f = nf->f; struct inet_frag_queue *q; - int err; q = inet_frag_alloc(nf, f, arg); - if (!q) + if (!q) { + *prev = ERR_PTR(-ENOMEM); return NULL; - + } mod_timer(&q->timer, jiffies + nf->timeout); - err = rhashtable_insert_fast(&nf->rhashtable, &q->node, - f->rhash_params); - if (err < 0) { + *prev = rhashtable_lookup_get_insert_key(&nf->rhashtable, &q->key, + &q->node, f->rhash_params); + if (*prev) { q->flags |= INET_FRAG_COMPLETE; inet_frag_kill(q); inet_frag_destroy(q); @@ -204,22 +205,22 @@ static struct inet_frag_queue *inet_frag_create(struct netns_frags *nf, /* TODO : call from rcu_read_lock() and no longer use refcount_inc_not_zero() */ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf, void *key) { - struct inet_frag_queue *fq; + struct inet_frag_queue *fq, *prev; if (!nf->high_thresh || frag_mem_limit(nf) > nf->high_thresh) return NULL; rcu_read_lock(); - fq = rhashtable_lookup(&nf->rhashtable, key, nf->f->rhash_params); - if (fq) { + prev = rhashtable_lookup(&nf->rhashtable, key, nf->f->rhash_params); + if (!prev) + fq = inet_frag_create(nf, key, &prev); + if (prev && !IS_ERR(prev)) { + fq = prev; if (!refcount_inc_not_zero(&fq->refcnt)) fq = NULL; - rcu_read_unlock(); - return fq; } rcu_read_unlock(); - - return inet_frag_create(nf, key); + return fq; } EXPORT_SYMBOL(inet_frag_find);
Multiple cpus might attempt to insert a new fragment in rhashtable, if for example RPS is buggy, as reported by 배석진in https://patchwork.ozlabs.org/patch/994601/ We use rhashtable_lookup_get_insert_key() instead of rhashtable_insert_fast() to let cpus losing the race free their own inet_frag_queue and use the one that was inserted by another cpu. Fixes: 648700f76b03 ("inet: frags: use rhashtables for reassembly units") Signed-off-by: Eric Dumazet <edumazet@google.com> Reported-by: 배석진 <soukjin.bae@samsung.com> --- net/ipv4/inet_fragment.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)