netdev - Re: [PATCH 5/9] lib/ref_tracker: improve allocation flags

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+nCZ6LV_1E2OnJ4qWE0XkO2FGW+A6_tkmQpdxiiEh=LQ@mail.gmail.com>
Date:   Thu, 17 Feb 2022 07:13:13 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Andrzej Hajda <andrzej.hajda@...el.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        netdev <netdev@...r.kernel.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Lucas De Marchi <lucas.demarchi@...el.com>,
        Chris Wilson <chris.p.wilson@...el.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH 5/9] lib/ref_tracker: improve allocation flags

On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <andrzej.hajda@...el.com> wrote:
>
> Library can be called in non-sleeping context, so it should not use
> __GFP_NOFAIL. Instead it should calmly handle allocation fails, for
> this __GFP_NOWARN has been added as well.

Your commit changelog is misleading .

The GFP_NOFAIL issue has been fixed already in
commit c12837d1bb31032bead9060dec99ef310d5b9fb7 ("ref_tracker: use
__GFP_NOFAIL more carefully")


>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@...el.com>
> ---
>  lib/ref_tracker.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
> index 7b00bca300043..c8441ffbb058a 100644
> --- a/lib/ref_tracker.c
> +++ b/lib/ref_tracker.c
> @@ -59,7 +59,7 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir,
>         if (list_empty(&dir->list))
>                 return;
>
> -       sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT);
> +       sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT | __GFP_NOWARN);

This belongs to patch 3 in your series.

>
>         list_for_each_entry(tracker, &dir->list, head)
>                 ++total;
> @@ -154,11 +154,11 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir,
>         unsigned long entries[REF_TRACKER_STACK_ENTRIES];
>         struct ref_tracker *tracker;
>         unsigned int nr_entries;
> -       gfp_t gfp_mask = gfp;

Simply change this line to : gfp_t gfp_mask = gfp | __GFP_NOFAIL;

> +       gfp_t gfp_mask;
>         unsigned long flags;
>

Then leave all this code as is ? I find current code more readable.

> -       if (gfp & __GFP_DIRECT_RECLAIM)
> -               gfp_mask |= __GFP_NOFAIL;
> +       gfp |= __GFP_NOWARN;
> +       gfp_mask = (gfp & __GFP_DIRECT_RECLAIM) ? (gfp | __GFP_NOFAIL) : gfp;
>         *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask);
>         if (unlikely(!tracker)) {
>                 pr_err_once("memory allocation failure, unreliable refcount tracker.\n");
> @@ -191,7 +191,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir,
>         }
>         nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1);
>         nr_entries = filter_irq_stacks(entries, nr_entries);

lib/ref_tracker.c got patches in net-next, your patch series is going
to add conflicts.

git log --oneline 5740d0689096..4d449bdc5b26 --no-merges -- lib/ref_tracker.c
c2d1e3df4af59261777b39c2e47476acd4d1cbeb ref_tracker: remove
filter_irq_stacks() call
8fd5522f44dcd7f05454ddc4f16d0f821b676cd9 ref_tracker: add a count of
untracked references
e3ececfe668facd87d920b608349a32607060e66 ref_tracker: implement
use-after-free detection


> -       stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC);
> +       stack_handle = stack_depot_save(entries, nr_entries,
> +                                       GFP_NOWAIT | __GFP_NOWARN);

This is fine.

>
>         spin_lock_irqsave(&dir->lock, flags);
>         if (tracker->dead) {
> --
> 2.25.1
>