From patchwork Thu Oct 6 06:00:22 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chengen Du X-Patchwork-Id: 1686664 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=canonical.com header.i=@canonical.com header.a=rsa-sha256 header.s=20210705 header.b=NsdDiZ4t; dkim-atps=neutral Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4MjjWH4lmSz1yqk for ; Thu, 6 Oct 2022 18:19:54 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1ogLAO-0005u0-F7; Thu, 06 Oct 2022 07:19:36 +0000 Received: from smtp-relay-internal-1.internal ([10.131.114.114] helo=smtp-relay-internal-1.canonical.com) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1ogJvr-0000sX-3E for kernel-team@lists.ubuntu.com; Thu, 06 Oct 2022 06:00:31 +0000 Received: from mail-pj1-f70.google.com (mail-pj1-f70.google.com [209.85.216.70]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-1.canonical.com (Postfix) with ESMTPS id 50D1E3F116 for ; Thu, 6 Oct 2022 06:00:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1665036029; bh=74lW27rL+f+vQXJdjWxJ/lToq+lsik6a9U+ok2HiPvw=; h=From:To:Subject:Date:Message-Id:MIME-Version; b=NsdDiZ4tiwookmYuWUNqjZ0D4u+xuf+wHha4qa5ikq7Isnmd7f/xwMP5ers8n+bZq xLexHBnE1kyPjht1j9UDgxwePUQpFFxyyjdzEzc1GU2DNsxulIygdqUIMw9Pr9OZPa Iyp8oQSEZmLGuwbN1h2Uc6IHT+1n7WGN2NyqU/XIgP9h3ehnlFZav4QtHHGLt1zt2U ckRQ/qVXckKk7JlegVK8ruU0dftr5ZMsVHXCoEmcv1S0HV8GdnOJMdo62tsKA8/UNy 8aK0DzFg0yh3O+Ph7i1ctFYH6k5gPwJqOwh1+lHv51WnVlfKwrp6xo88xjtb6O+2b4 gwDRYuksJVMUw== Received: by mail-pj1-f70.google.com with SMTP id pa16-20020a17090b265000b0020a71040b4cso523664pjb.6 for ; Wed, 05 Oct 2022 23:00:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=74lW27rL+f+vQXJdjWxJ/lToq+lsik6a9U+ok2HiPvw=; b=Wd9QC8pv4LVItfVbf7v5wiqChuDD24k8nGkG5+pzoBAV+Hnc4+A7AxOADyhSwY3MUM +7xA8irJoVAJ2mbIvKjrJ4LYuJREF89PCoahO17M6Dc5HcA5f5rjBy/B7qQ5Iq7FRXVO vQdS2ZJM2WCDrO47pL/ACAEdr2qZDVdpUp7anlVDPfyA++/31faUj1dMaq5muJ5UPAm7 V6r71gxrbZxLkL+jSJCUkAk0UQhIOhQq/5t9A0cMiTjCRgWuXWop0EFXV7Lg01FZvYza KX0KfQEV5q1lgOCe7Giq8mgVKlfEtY78g4wAalHyNsjc9Z0osjtd7mVdLDQXX2vDc+Ex zfcA== X-Gm-Message-State: ACrzQf0ou7vPKbDJjBVDdSq/pBFkyyFgck8EYHQTMj1N7JJcIxjACoZX kR+mugQxgGwcz6Zf9DVOl0yC+IwhH9L5VGfp0un1mtD3W/oaCZdfnQHXs4I6BbWer4V8JGmO+YK 9OWO3ZKX/7FBM1RCyoU1yy8HCQ3KFxChx9aZ5ICeRhA== X-Received: by 2002:a17:90b:48c9:b0:20a:f2f5:45c8 with SMTP id li9-20020a17090b48c900b0020af2f545c8mr5847173pjb.18.1665036027305; Wed, 05 Oct 2022 23:00:27 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6WYFBAPmpYxU5IoR8hQGBrM36X8QvhbHvsLj5+DgIsM+WEowTCiPQPAiErsiFPPKOo6HE17A== X-Received: by 2002:a17:90b:48c9:b0:20a:f2f5:45c8 with SMTP id li9-20020a17090b48c900b0020af2f545c8mr5847114pjb.18.1665036026636; Wed, 05 Oct 2022 23:00:26 -0700 (PDT) Received: from chengendu-Inspiron-3910.. (111-248-165-186.dynamic-ip.hinet.net. [111.248.165.186]) by smtp.gmail.com with ESMTPSA id f33-20020a17090a702400b0020647f279fbsm2080272pjk.29.2022.10.05.23.00.25 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Oct 2022 23:00:26 -0700 (PDT) From: Chengen Du To: kernel-team@lists.ubuntu.com Subject: [SRU][Bionic][PATCH 0/1] (upstream) netfilter: nf_queue: Fix memory leak in nf_queue_entry_get_refs Date: Thu, 6 Oct 2022 14:00:22 +0800 Message-Id: <20221006060023.27373-1-chengen.du@canonical.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-Mailman-Approved-At: Thu, 06 Oct 2022 07:19:35 +0000 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: "Chengen Du" [Impact] Environment: v4.15.0-177-generic Xenial ESM Using NFQUEUE to delegate the decision on TCP packets to userspace processes will cause memory leak. The symptom is that TCP slab objects will accumulate and eventually cause OOM. [Fix] There is a discrepancy between backport and upstream commit. [Upstream Commit c3873070247d9e3c7a6b0cf9bf9b45e8018427b1] [Difference between Commits] 50,57c57,62 < dev_hold(state->in); < dev_hold(state->out); < - if (state->sk) < - sock_hold(state->sk); < < #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) < dev_hold(entry->physin); < dev_hold(entry->physout); Acked-by: Tim Gardner --- > if (state->in) > dev_hold(state->in); > if (state->out) > @@ -102,6 +105,7 @@ void nf_queue_entry_get_refs(struct nf_queue_entry *entry) > dev_hold(physdev); > } The sock_hold() logic still remains in the backport commit, which will affect the reference count and result in memory leak. The fix aligns the logic with the upstream commit. [Test Plan] 1. Prepare a VM and run a TCP server on host. 2. Enter into the VM and set up a iptables rule. iptables -I OUTPUT -p tcp --dst <-TCP_SERVER_IP-> -j NFQUEUE --queue-num=1 --queue-bypass 3. Run a nfnetlink client (should listen on NF queue 1) on VM. 4. Keep connecting the TCP server from VM. while true; do netcat <-TCP_SERVER_IP-> 8080; done 5. The VM's TCP slab objects will accumulate and eventually encounter OOM situation. cat /proc/slabinfo | grep TCP [Where problems could occur] The fix just aligns the logic with the upstream commit, so the regression can be considered as low. Chengen Du (1): (upstream) netfilter: nf_queue: Fix memory leak in nf_queue_entry_get_refs net/netfilter/nf_queue.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h index 9eed51e920e8..980daa6e1e3a 100644 --- a/include/net/netfilter/nf_queue.h +++ b/include/net/netfilter/nf_queue.h @@ -37,7 +37,7 @@ void nf_register_queue_handler(const struct nf_queue_handler *qh); void nf_unregister_queue_handler(void); void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict); -void nf_queue_entry_get_refs(struct nf_queue_entry *entry); +bool nf_queue_entry_get_refs(struct nf_queue_entry *entry); void nf_queue_entry_free(struct nf_queue_entry *entry); static inline void init_hashrandom(u32 *jhash_initval) diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c index 5ab0680db445..e39549c55945 100644 --- a/net/netfilter/nf_queue.c +++ b/net/netfilter/nf_queue.c @@ -96,19 +96,21 @@ static void __nf_queue_entry_init_physdevs(struct nf_queue_entry *entry) } /* Bump dev refs so they don't vanish while packet is out */ -void nf_queue_entry_get_refs(struct nf_queue_entry *entry) +bool nf_queue_entry_get_refs(struct nf_queue_entry *entry) { struct nf_hook_state *state = &entry->state; + if (state->sk && !refcount_inc_not_zero(&state->sk->sk_refcnt)) + return false; + dev_hold(state->in); dev_hold(state->out); - if (state->sk) - sock_hold(state->sk); #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) dev_hold(entry->physin); dev_hold(entry->physout); #endif + return true; } EXPORT_SYMBOL_GPL(nf_queue_entry_get_refs); @@ -196,7 +198,10 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state, __nf_queue_entry_init_physdevs(entry); - nf_queue_entry_get_refs(entry); + if (!nf_queue_entry_get_refs(entry)) { + kfree(entry); + return -ENOTCONN; + } switch (entry->state.pf) { case AF_INET: diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c index ea2d9c2a44cf..64a6acb6aeae 100644 --- a/net/netfilter/nfnetlink_queue.c +++ b/net/netfilter/nfnetlink_queue.c @@ -710,9 +710,15 @@ static struct nf_queue_entry * nf_queue_entry_dup(struct nf_queue_entry *e) { struct nf_queue_entry *entry = kmemdup(e, e->size, GFP_ATOMIC); - if (entry) - nf_queue_entry_get_refs(entry); - return entry; + + if (!entry) + return NULL; + + if (nf_queue_entry_get_refs(entry)) + return entry; + + kfree(entry); + return NULL; } #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) [Backport Commit 4d032d60432327f068f03b516f5b6c51ceb17d15] diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h index 814058d0f167..f38cc6092c5a 100644 --- a/include/net/netfilter/nf_queue.h +++ b/include/net/netfilter/nf_queue.h @@ -32,7 +32,7 @@ void nf_register_queue_handler(struct net *net, const struct nf_queue_handler *q void nf_unregister_queue_handler(struct net *net); void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict); -void nf_queue_entry_get_refs(struct nf_queue_entry *entry); +bool nf_queue_entry_get_refs(struct nf_queue_entry *entry); void nf_queue_entry_release_refs(struct nf_queue_entry *entry); static inline void init_hashrandom(u32 *jhash_initval) diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c index 59340b3ef7ef..dbc45165c533 100644 --- a/net/netfilter/nf_queue.c +++ b/net/netfilter/nf_queue.c @@ -80,10 +80,13 @@ void nf_queue_entry_release_refs(struct nf_queue_entry *entry) EXPORT_SYMBOL_GPL(nf_queue_entry_release_refs); /* Bump dev refs so they don't vanish while packet is out */ -void nf_queue_entry_get_refs(struct nf_queue_entry *entry) +bool nf_queue_entry_get_refs(struct nf_queue_entry *entry) { struct nf_hook_state *state = &entry->state; + if (state->sk && !refcount_inc_not_zero(&state->sk->sk_refcnt)) + return false; + if (state->in) dev_hold(state->in); if (state->out) @@ -102,6 +105,7 @@ void nf_queue_entry_get_refs(struct nf_queue_entry *entry) dev_hold(physdev); } #endif + return true; } EXPORT_SYMBOL_GPL(nf_queue_entry_get_refs); @@ -159,7 +163,11 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state, .size = sizeof(*entry) + afinfo->route_key_size, }; - nf_queue_entry_get_refs(entry); + if (!nf_queue_entry_get_refs(entry)) { + kfree(entry); + return -ENOTCONN; + } + afinfo->saveroute(skb, entry); status = qh->outfn(entry, queuenum); diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c index 48ed30e1b405..abad8bf6fa38 100644 --- a/net/netfilter/nfnetlink_queue.c +++ b/net/netfilter/nfnetlink_queue.c @@ -693,9 +693,15 @@ static struct nf_queue_entry * nf_queue_entry_dup(struct nf_queue_entry *e) { struct nf_queue_entry *entry = kmemdup(e, e->size, GFP_ATOMIC); - if (entry) - nf_queue_entry_get_refs(entry); - return entry; + + if (!entry) + return NULL; + + if (nf_queue_entry_get_refs(entry)) + return entry; + + kfree(entry); + return NULL; } #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)