netdev - Re: [PATCH net] net/core/dev.c: Ensure pfmemalloc skbs are correctly handled when receiving

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANn89iLtsPHgaOdXxzeC61MEE96puObqOtuwCYmCAy3A79h7Yw@mail.gmail.com>
Date:   Sat, 17 Apr 2021 06:52:08 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Xie He <xie.he.0141@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andriin@...com>, Wei Wang <weiwan@...gle.com>,
        Cong Wang <cong.wang@...edance.com>,
        Taehee Yoo <ap420073@...il.com>,
        Björn Töpel <bjorn@...nel.org>,
        Mel Gorman <mgorman@...e.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, Neil Brown <neilb@...e.de>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Jiri Slaby <jslaby@...e.cz>,
        Mike Christie <michaelc@...wisc.edu>,
        Eric B Munson <emunson@...bm.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Sebastian Andrzej Siewior <sebastian@...akpoint.cc>,
        Christoph Lameter <cl@...ux.com>
Subject: Re: [PATCH net] net/core/dev.c: Ensure pfmemalloc skbs are correctly
 handled when receiving

On Sat, Apr 17, 2021 at 2:08 AM Xie He <xie.he.0141@...il.com> wrote:
>
> When an skb is allocated by "__netdev_alloc_skb" in "net/core/skbuff.c",
> if "sk_memalloc_socks()" is true, and if there's not sufficient memory,
> the skb would be allocated using emergency memory reserves. This kind of
> skbs are called pfmemalloc skbs.
>
> pfmemalloc skbs must be specially handled in "net/core/dev.c" when
> receiving. They must NOT be delivered to the target protocol if
> "skb_pfmemalloc_protocol(skb)" is false.
>
> However, if, after a pfmemalloc skb is allocated and before it reaches
> the code in "__netif_receive_skb", "sk_memalloc_socks()" becomes false,
> then the skb will be handled by "__netif_receive_skb" as a normal skb.
> This causes the skb to be delivered to the target protocol even if
> "skb_pfmemalloc_protocol(skb)" is false.
>
> This patch fixes this problem by ensuring all pfmemalloc skbs are handled
> by "__netif_receive_skb" as pfmemalloc skbs.
>
> "__netif_receive_skb_list" has the same problem as "__netif_receive_skb".
> This patch also fixes it.
>
> Fixes: b4b9e3558508 ("netvm: set PF_MEMALLOC as appropriate during SKB processing")
> Cc: Mel Gorman <mgorman@...e.de>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: Neil Brown <neilb@...e.de>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Jiri Slaby <jslaby@...e.cz>
> Cc: Mike Christie <michaelc@...wisc.edu>
> Cc: Eric B Munson <emunson@...bm.net>
> Cc: Eric Dumazet <eric.dumazet@...il.com>
> Cc: Sebastian Andrzej Siewior <sebastian@...akpoint.cc>
> Cc: Christoph Lameter <cl@...ux.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Signed-off-by: Xie He <xie.he.0141@...il.com>
> ---
>  net/core/dev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1f79b9aa9a3f..3e6b7879daef 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5479,7 +5479,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
>  {
>         int ret;
>
> -       if (sk_memalloc_socks() && skb_pfmemalloc(skb)) {
> +       if (skb_pfmemalloc(skb)) {
>                 unsigned int noreclaim_flag;
>
>                 /*
> @@ -5507,7 +5507,7 @@ static void __netif_receive_skb_list(struct list_head *head)
>         bool pfmemalloc = false; /* Is current sublist PF_MEMALLOC? */
>
>         list_for_each_entry_safe(skb, next, head, list) {
> -               if ((sk_memalloc_socks() && skb_pfmemalloc(skb)) != pfmemalloc) {
> +               if (skb_pfmemalloc(skb) != pfmemalloc) {
>                         struct list_head sublist;
>
>                         /* Handle the previous sublist */
> --
> 2.27.0
>

The race window has been considered to be small that we prefer the
code as it is.

The reason why we prefer current code is that we use a static key for
the implementation
of sk_memalloc_socks()

Trading some minor condition (race) with extra cycles for each
received packet is a serious concern.

What matters is a persistent condition that would _deplete_ memory,
not for a dozen of packets,
but thousands. Can you demonstrate such an issue ?