| Message ID | 20180205224145.9420-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net,v2] xt_RATEEST: acquire xt_rateest_mutex for hash insert | expand |
Cong Wang <xiyou.wangcong@gmail.com> wrote: > rateest_hash is supposed to be protected by xt_rateest_mutex, > and, as suggested by Eric, lookup and insert should be atomic, > so we should acquire the xt_rateest_mutex once for both. > > So introduce a non-locking helper for internal use and keep the > locking one for external. Looks good, thanks Cong. Reviewed-by: Florian Westphal <fw@strlen.de> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2018-02-05 at 14:41 -0800, Cong Wang wrote: > rateest_hash is supposed to be protected by xt_rateest_mutex, > and, as suggested by Eric, lookup and insert should be atomic, > so we should acquire the xt_rateest_mutex once for both. > > So introduce a non-locking helper for internal use and keep the > locking one for external. > > Reported-by: <syzbot+5cb189720978275e4c75@syzkaller.appspotmail.com> > Fixes: 5859034d7eb8 ("[NETFILTER]: x_tables: add RATEEST target") > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- Reviewed-by: Eric Dumazet <edumazet@google.com> Thanks ! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 05, 2018 at 02:41:45PM -0800, Cong Wang wrote: > rateest_hash is supposed to be protected by xt_rateest_mutex, > and, as suggested by Eric, lookup and insert should be atomic, > so we should acquire the xt_rateest_mutex once for both. > > So introduce a non-locking helper for internal use and keep the > locking one for external. Applied, thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/xt_RATEEST.c b/net/netfilter/xt_RATEEST.c index 498b54fd04d7..141c295191f6 100644 --- a/net/netfilter/xt_RATEEST.c +++ b/net/netfilter/xt_RATEEST.c @@ -39,23 +39,31 @@ static void xt_rateest_hash_insert(struct xt_rateest *est) hlist_add_head(&est->list, &rateest_hash[h]); } -struct xt_rateest *xt_rateest_lookup(const char *name) +static struct xt_rateest *__xt_rateest_lookup(const char *name) { struct xt_rateest *est; unsigned int h; h = xt_rateest_hash(name); - mutex_lock(&xt_rateest_mutex); hlist_for_each_entry(est, &rateest_hash[h], list) { if (strcmp(est->name, name) == 0) { est->refcnt++; - mutex_unlock(&xt_rateest_mutex); return est; } } - mutex_unlock(&xt_rateest_mutex); + return NULL; } + +struct xt_rateest *xt_rateest_lookup(const char *name) +{ + struct xt_rateest *est; + + mutex_lock(&xt_rateest_mutex); + est = __xt_rateest_lookup(name); + mutex_unlock(&xt_rateest_mutex); + return est; +} EXPORT_SYMBOL_GPL(xt_rateest_lookup); void xt_rateest_put(struct xt_rateest *est) @@ -100,8 +108,10 @@ static int xt_rateest_tg_checkentry(const struct xt_tgchk_param *par) net_get_random_once(&jhash_rnd, sizeof(jhash_rnd)); - est = xt_rateest_lookup(info->name); + mutex_lock(&xt_rateest_mutex); + est = __xt_rateest_lookup(info->name); if (est) { + mutex_unlock(&xt_rateest_mutex); /* * If estimator parameters are specified, they must match the * existing estimator. @@ -139,11 +149,13 @@ static int xt_rateest_tg_checkentry(const struct xt_tgchk_param *par) info->est = est; xt_rateest_hash_insert(est); + mutex_unlock(&xt_rateest_mutex); return 0; err2: kfree(est); err1: + mutex_unlock(&xt_rateest_mutex); return ret; }
rateest_hash is supposed to be protected by xt_rateest_mutex, and, as suggested by Eric, lookup and insert should be atomic, so we should acquire the xt_rateest_mutex once for both. So introduce a non-locking helper for internal use and keep the locking one for external. Reported-by: <syzbot+5cb189720978275e4c75@syzkaller.appspotmail.com> Fixes: 5859034d7eb8 ("[NETFILTER]: x_tables: add RATEEST target") Cc: Pablo Neira Ayuso <pablo@netfilter.org> Cc: Eric Dumazet <eric.dumazet@gmail.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/netfilter/xt_RATEEST.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)