From: msizanoen <msizanoen@qtmlabs.xyz>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>,
netdev@vger.kernel.org, David Ahern <dsahern@gmail.com>,
Wei Wang <weiwan@google.com>, David Miller <davem@davemloft.net>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
Subject: Re: [PATCH net] ipv6: fix memory leak in fib6_rule_suppress
Date: Tue, 23 Nov 2021 19:57:31 +0700 [thread overview]
Message-ID: <e15bcc8a-4113-bdd9-30e7-9c1352003791@qtmlabs.xyz> (raw)
In-Reply-To: <20211123124832.15419-1-Jason@zx2c4.com>
[-- Attachment #1: Type: text/plain, Size: 5760 bytes --]
On 11/23/21 19:48, Jason A. Donenfeld wrote:
> From: msizanoen1 <msizanoen@qtmlabs.xyz>
>
> The kernel leaks memory when a `fib` rule is present in IPv6 nftables
> firewall rules and a suppress_prefix rule is present in the IPv6 routing
> rules (used by certain tools such as wg-quick). In such scenarios, every
> incoming packet will leak an allocation in `ip6_dst_cache` slab cache.
>
> After some hours of `bpftrace`-ing and source code reading, I tracked
> down the issue to ca7a03c41753 ("ipv6: do not free rt if
> FIB_LOOKUP_NOREF is set on suppress rule").
>
> The problem with that change is that the generic `args->flags` always have
> `FIB_LOOKUP_NOREF` set[1][2] but the IPv6-specific flag
> `RT6_LOOKUP_F_DST_NOREF` might not be, leading to `fib6_rule_suppress` not
> decreasing the refcount when needed.
>
> How to reproduce:
> - Add the following nftables rule to a prerouting chain:
> meta nfproto ipv6 fib saddr . mark . iif oif missing drop
> This can be done with:
> sudo nft create table inet test
> sudo nft create chain inet test test_chain '{ type filter hook prerouting priority filter + 10; policy accept; }'
> sudo nft add rule inet test test_chain meta nfproto ipv6 fib saddr . mark . iif oif missing drop
> - Run:
> sudo ip -6 rule add table main suppress_prefixlength 0
> - Watch `sudo slabtop -o | grep ip6_dst_cache` to see memory usage increase
> with every incoming ipv6 packet.
>
> This patch exposes the protocol-specific flags to the protocol
> specific `suppress` function, and check the protocol-specific `flags`
> argument for RT6_LOOKUP_F_DST_NOREF instead of the generic
> FIB_LOOKUP_NOREF when decreasing the refcount, like this.
>
> [1]: https://github.com/torvalds/linux/blob/ca7a03c4175366a92cee0ccc4fec0038c3266e26/net/ipv6/fib6_rules.c#L71
> [2]: https://github.com/torvalds/linux/blob/ca7a03c4175366a92cee0ccc4fec0038c3266e26/net/ipv6/fib6_rules.c#L99
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215105
> Fixes: ca7a03c41753 ("ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> The original author of this commit and commit message is anonymous and
> is therefore unable to sign off on it. Greg suggested that I do the sign
> off, extracting it from the bugzilla entry above, and post it properly.
> The patch "seems to work" on first glance, but I haven't looked deeply
> at it yet and therefore it doesn't have my Reviewed-by, even though I'm
> submitting this patch on the author's behalf. And it should probably get
> a good look from the v6 fib folks. The original author should be on this
> thread to address issues that come off, and I'll shephard additional
> versions that he has.
This patch has been running on my personal laptop since I debugged the
issue, so you can also add a `Tested-by: <msizanoen@qtmlabs.xyz>`.
>
> include/net/fib_rules.h | 4 +++-
> net/core/fib_rules.c | 2 +-
> net/ipv4/fib_rules.c | 1 +
> net/ipv6/fib6_rules.c | 4 ++--
> 4 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/fib_rules.h b/include/net/fib_rules.h
> index 4b10676c69d1..bd07484ab9dd 100644
> --- a/include/net/fib_rules.h
> +++ b/include/net/fib_rules.h
> @@ -69,7 +69,7 @@ struct fib_rules_ops {
> int (*action)(struct fib_rule *,
> struct flowi *, int,
> struct fib_lookup_arg *);
> - bool (*suppress)(struct fib_rule *,
> + bool (*suppress)(struct fib_rule *, int,
> struct fib_lookup_arg *);
> int (*match)(struct fib_rule *,
> struct flowi *, int);
> @@ -218,7 +218,9 @@ INDIRECT_CALLABLE_DECLARE(int fib4_rule_action(struct fib_rule *rule,
> struct fib_lookup_arg *arg));
>
> INDIRECT_CALLABLE_DECLARE(bool fib6_rule_suppress(struct fib_rule *rule,
> + int flags,
> struct fib_lookup_arg *arg));
> INDIRECT_CALLABLE_DECLARE(bool fib4_rule_suppress(struct fib_rule *rule,
> + int flags,
> struct fib_lookup_arg *arg));
> #endif
> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
> index 79df7cd9dbc1..1bb567a3b329 100644
> --- a/net/core/fib_rules.c
> +++ b/net/core/fib_rules.c
> @@ -323,7 +323,7 @@ int fib_rules_lookup(struct fib_rules_ops *ops, struct flowi *fl,
> if (!err && ops->suppress && INDIRECT_CALL_MT(ops->suppress,
> fib6_rule_suppress,
> fib4_rule_suppress,
> - rule, arg))
> + rule, flags, arg))
> continue;
>
> if (err != -EAGAIN) {
> diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
> index ce54a30c2ef1..364ad3446b2f 100644
> --- a/net/ipv4/fib_rules.c
> +++ b/net/ipv4/fib_rules.c
> @@ -141,6 +141,7 @@ INDIRECT_CALLABLE_SCOPE int fib4_rule_action(struct fib_rule *rule,
> }
>
> INDIRECT_CALLABLE_SCOPE bool fib4_rule_suppress(struct fib_rule *rule,
> + int flags,
> struct fib_lookup_arg *arg)
> {
> struct fib_result *result = (struct fib_result *) arg->result;
> diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> index 40f3e4f9f33a..dcedfe29d9d9 100644
> --- a/net/ipv6/fib6_rules.c
> +++ b/net/ipv6/fib6_rules.c
> @@ -267,6 +267,7 @@ INDIRECT_CALLABLE_SCOPE int fib6_rule_action(struct fib_rule *rule,
> }
>
> INDIRECT_CALLABLE_SCOPE bool fib6_rule_suppress(struct fib_rule *rule,
> + int flags,
> struct fib_lookup_arg *arg)
> {
> struct fib6_result *res = arg->result;
> @@ -294,8 +295,7 @@ INDIRECT_CALLABLE_SCOPE bool fib6_rule_suppress(struct fib_rule *rule,
> return false;
>
> suppress_route:
> - if (!(arg->flags & FIB_LOOKUP_NOREF))
> - ip6_rt_put(rt);
> + ip6_rt_put_flags(rt, flags);
> return true;
> }
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4494 bytes --]
next prev parent reply other threads:[~2021-11-23 13:04 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-23 12:48 [PATCH net] ipv6: fix memory leak in fib6_rule_suppress Jason A. Donenfeld
2021-11-23 12:57 ` msizanoen [this message]
2021-11-24 4:03 ` Jakub Kicinski
2021-11-24 7:00 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e15bcc8a-4113-bdd9-30e7-9c1352003791@qtmlabs.xyz \
--to=msizanoen@qtmlabs.xyz \
--cc=Jason@zx2c4.com \
--cc=davem@davemloft.net \
--cc=dsahern@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=netdev@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=weiwan@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).